* [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.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal