linux-kernel - RE: [EXTERNAL] Re: [Patch net-next v2] hv_netvsc: Set device flags for properly indicating bonding in Hyper-V

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <LV8PR21MB4236726636CF9CA5E3EB6BE1CEEE2@LV8PR21MB4236.namprd21.prod.outlook.com>
Date: Wed, 29 Jan 2025 22:22:35 +0000
From: Long Li <longli@...rosoft.com>
To: Jakub Kicinski <kuba@...nel.org>, "longli@...uxonhyperv.com"
	<longli@...uxonhyperv.com>
CC: Jason Gunthorpe <jgg@...pe.ca>, Leon Romanovsky <leon@...nel.org>, Ajay
 Sharma <sharmaajay@...rosoft.com>, Konstantin Taranov
	<kotaranov@...rosoft.com>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Stephen
 Hemminger <stephen@...workplumber.org>, "linux-rdma@...r.kernel.org"
	<linux-rdma@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-hyperv@...r.kernel.org"
	<linux-hyperv@...r.kernel.org>
Subject: RE: [EXTERNAL] Re: [Patch net-next v2] hv_netvsc: Set device flags
 for properly indicating bonding in Hyper-V

> Subject: [EXTERNAL] Re: [Patch net-next v2] hv_netvsc: Set device flags for
> properly indicating bonding in Hyper-V
> 
> On Fri, 13 Dec 2024 12:06:01 -0800 longli@...uxonhyperv.com wrote:
> > Other kernel APIs (e.g those in "include/linux/netdevice.h") check for
> > IFF_MASTER, IFF_SLAVE and IFF_BONDING for determing if those are used
> > in a master/slave bonded setup. RDMA uses those APIs extensively when
> > looking for master/slave devices. Netvsc's bonding setup with its
> > slave device falls into this category.
> >
> > Make hv_netvsc properly indicate bonding with its slave and change the
> > API to reflect this bonding setup.
> 
> This is severely lacking in terms of safety analysis.

I'm sorry for the late reply. 

The usage of netif_is_bond_slave() and netif_is_bond_master() are mostly in device drivers that checks for bonding as configured from user-mode. The check is consistent with the netvsc bonding (bonding is done in kernel mode without any user-mode configuration). I don't see any security risk if netvsc is bonded and becomes a master device to the bonded slave device. Any those checks will return the same value as if the bonding is done from the user-mode. 

There are two places other than individual device driver that check for bonded slave/master.
1. ./net/tls/tls_device.c and ./net/tls/tls_device_fallback.c: it checks for sending packets over a netdev for an SKB if that netdev is within the context of TLS or the netdev is a bonded master. If netvsc uses this bonding flag, the check will pass for netvsc netdev as if it is the bonded master. This has the same effect of user-configured bonding devices. Please see possible security implications below.

2. drivers/infiniband/core/roce_gid_mgmt.c:: it checks of net device for caching their addresses for RoCE gid lookup. The code check for master/slave bonded devices and determined which of their addresses should be used in the GID cache. If netvsc uses this bonding flag, it will be consistent with all the checks on identifying the correct addresses (master's, not slave's) for GIDs. This has the same effect of bonding configuration from user-mode.

One possible security problem is that the user-mode can assign different permissions to different netdev (and expose to containers) and that the slave device and netvsc may have different permissions. I want to point out this is an invalid configuration when a slave device doesn't have the same permission of its parent netvsc. Because the slave device along can't function in the hyper-v environment when netvsc is present. It must bond to the netvsc for any outgoing/incoming packets to work. If a user wants to configure different permissions, it must assume the kernel will always bond netvsc with the slave device and they must use the same permissions (and for assigning to containers).

> 
> > @@ -2204,6 +2204,10 @@ static int netvsc_vf_join(struct net_device
> *vf_netdev,
> >  		goto rx_handler_failed;
> >  	}
> >
> > +	vf_netdev->permanent_bond = 1;
> > +	ndev->permanent_bond = 1;
> > +	ndev->flags |= IFF_MASTER;
> 
> > @@ -2484,7 +2488,15 @@ static int netvsc_unregister_vf(struct
> > net_device *vf_netdev)
> >
> >  	reinit_completion(&net_device_ctx->vf_add);
> >  	netdev_rx_handler_unregister(vf_netdev);
> > +
> > +	/* Unlink the slave device and clear flag */
> > +	vf_netdev->permanent_bond = 0;
> > +	ndev->permanent_bond = 0;
> 
> > + *	@permanent_bond: device is permanently bonded to another device
> 
> I think we have been taught a definition of the word "permanent"

Is it okay that it uses "kernel_bond" here? The reason is that netvsc is doing unconditional bonding without any user configuration.

IMHO, using IFF_BONDING is the least disruptive way to express this relationship without adding new flags, given the behavior of netvsc bonding is identical to that of the bonding driver, but without any configuration from user-mode.

Thanks,
Long