* [pve-devel] [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces @ 2024-09-25 11:39 Daniel Kral 2024-09-25 11:39 ` [pve-devel] [PATCH common 2/2] net: add name checks when creating bridge and veth interfaces Daniel Kral 2024-11-11 18:36 ` [pve-devel] applied: [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces Thomas Lamprecht 0 siblings, 2 replies; 3+ messages in thread From: Daniel Kral @ 2024-09-25 11:39 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [pve-devel] [PATCH common 2/2] net: add name checks when creating bridge and veth interfaces 2024-09-25 11:39 [pve-devel] [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces Daniel Kral @ 2024-09-25 11:39 ` Daniel Kral 2024-11-11 18:36 ` [pve-devel] applied: [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces Thomas Lamprecht 1 sibling, 0 replies; 3+ messages in thread From: Daniel Kral @ 2024-09-25 11:39 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [pve-devel] applied: [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces 2024-09-25 11:39 [pve-devel] [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces Daniel Kral 2024-09-25 11:39 ` [pve-devel] [PATCH common 2/2] net: add name checks when creating bridge and veth interfaces Daniel Kral @ 2024-11-11 18:36 ` Thomas Lamprecht 1 sibling, 0 replies; 3+ messages in thread From: Thomas Lamprecht @ 2024-11-11 18:36 UTC (permalink / raw) To: Proxmox VE development discussion, Daniel Kral 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-11-11 18:37 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-09-25 11:39 [pve-devel] [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces Daniel Kral 2024-09-25 11:39 ` [pve-devel] [PATCH common 2/2] net: add name checks when creating bridge and veth interfaces Daniel Kral 2024-11-11 18:36 ` [pve-devel] applied: [PATCH common 1/2] fix #5454: net: check names for vlan bridge slave interfaces Thomas Lamprecht
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.