[pve-devel] [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces

[pve-devel] [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces

[Daniel Kral]

Adds a check for the name of VLAN bridge slave interfaces, which are
created on non VLAN-aware bridges. These checks mimics what is done when
parsing an interface name in iproute2 [0], which includes a name size
check, an empty string check and checking for invalid characters.

Without this check, creating a VLAN bridge slave interface, where the
length of the string "<iface>.<vlanid>" will be greater than or equal to
16 characters, resulted in the following error message from `ip` itself:

> Error: argument "<iface>.<vlanid>" is wrong: "name" not a valid ifname

[0] https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/lib/utils.c?h=v6.1.0#n825

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/Network.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/PVE/Network.pm b/src/PVE/Network.pm
index a4f5ba9..dd627f2 100644
--- a/src/PVE/Network.pm
+++ b/src/PVE/Network.pm
@@ -165,6 +165,22 @@ my $compute_fwbr_names = sub {
     return ($fwbr, $vethfw, $vethfwpeer, $ovsintport);
 };
 
+sub check_iface_name : prototype($) {
+    my ($name) = @_;
+
+    my $name_len = length($name);
+
+    # iproute2 / kernel have a strict interface name size limit
+    die "the interface name $name is too long"
+	if $name_len >= PVE::ProcFSTools::IFNAMSIZ;
+
+    # iproute2 checks with isspace(3), which includes vertical tabs (not catched with perl's '\s')
+    die "the interface name $name is empty or contains invalid characters"
+	if $name_len == 0 || $name =~ /\s|\v|\//;
+
+    return 1;
+}
+
 sub iface_delete :prototype($) {
     my ($iface) = @_;
     run_command(['/sbin/ip', 'link', 'delete', 'dev', $iface], noerr => 1)
@@ -561,6 +577,8 @@ sub activate_bridge_vlan_slave {
     # create vlan on $iface is not already exist
     if (! -d "/sys/class/net/$ifacevlan") {
 	eval {
+	    check_iface_name($ifacevlan);
+
 	    my $cmd = ['/sbin/ip', 'link', 'add'];
 	    push @$cmd, 'link', $iface;
 	    push @$cmd, 'name', $ifacevlan;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH common 2/2] net: add name checks when creating bridge and veth interfaces

[Daniel Kral]

Adds checks when creating interfaces with `veth_create`, which is used
when creating the veth interface for Linux firewall bridges, and
`iface_create`, which is used when creating Linux / OVS firewall bridges
and VLAN bridges.

There are no functional changes in `veth_create` except the added check.

Without these checks, the following cases:

- When creating more than 10 Linux firewall bridges on a VM with 9
  digits, e.g. 'fwbr999999999i10' is too long for an interface name
- When creating a VLAN bridge on a bridge that has already a long name,
  e.g. the bridge 'abcdefghjklm' will try to create 'abcdefghijklmv249'

will fail with a rather unhelpful error message from the kernel:

> Error: Attribute failed policy validation.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
This change was not part of the initial bug report #5454, which is why I
split it up. It is in no part essential for patch #1, but rather to add
preliminary checks at other places where similar errors could happen.

 src/PVE/Network.pm | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/src/PVE/Network.pm b/src/PVE/Network.pm
index dd627f2..cde7949 100644
--- a/src/PVE/Network.pm
+++ b/src/PVE/Network.pm
@@ -190,6 +190,10 @@ sub iface_delete :prototype($) {
 
 sub iface_create :prototype($$@) {
     my ($iface, $type, @args) = @_;
+
+    eval { check_iface_name($iface) };
+    die "failed to create interface '$iface' - $@" if $@;
+
     run_command(['/sbin/ip', 'link', 'add', $iface, 'type', $type, @args], noerr => 1)
 	== 0 or die "failed to create interface '$iface'\n";
     return;
@@ -376,17 +380,21 @@ sub veth_create {
 
     # create veth pair
     if (! -d "/sys/class/net/$veth") {
-	my $cmd = ['/sbin/ip', 'link', 'add'];
-	# veth device + MTU
-	push @$cmd, 'name', $veth;
-	push @$cmd, 'mtu', $bridgemtu;
-	push @$cmd, 'type', 'veth';
-	# peer device + MTU
-	push @$cmd, 'peer', 'name', $vethpeer, 'mtu', $bridgemtu;
+	eval {
+	    check_iface_name($veth);
 
-	push @$cmd, 'addr', $mac if $mac;
+	    my $cmd = ['/sbin/ip', 'link', 'add'];
+	    # veth device + MTU
+	    push @$cmd, 'name', $veth;
+	    push @$cmd, 'mtu', $bridgemtu;
+	    push @$cmd, 'type', 'veth';
+	    # peer device + MTU
+	    push @$cmd, 'peer', 'name', $vethpeer, 'mtu', $bridgemtu;
 
-	eval { run_command($cmd) };
+	    push @$cmd, 'addr', $mac if $mac;
+
+	    run_command($cmd);
+	};
 	die "can't create interface $veth - $@\n" if $@;
     }
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces

[Thomas Lamprecht]

Am 25.09.24 um 13:39 schrieb Daniel Kral:
> Adds a check for the name of VLAN bridge slave interfaces, which are
> created on non VLAN-aware bridges. These checks mimics what is done when
> parsing an interface name in iproute2 [0], which includes a name size
> check, an empty string check and checking for invalid characters.
> 
> Without this check, creating a VLAN bridge slave interface, where the
> length of the string "<iface>.<vlanid>" will be greater than or equal to
> 16 characters, resulted in the following error message from `ip` itself:
> 
>> Error: argument "<iface>.<vlanid>" is wrong: "name" not a valid ifname
> 
> [0] https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/lib/utils.c?h=v6.1.0#n825
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
>  src/PVE/Network.pm | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
>

applied both patches, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel