diff mbox series

[4,net] RDMA/core: fix UAF with ib_device_get_netdev()

Message ID 20240401100005.1799-1-dkirjanov@suse.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [4,net] RDMA/core: fix UAF with ib_device_get_netdev() | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 948 this patch: 948
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: benve@cisco.com; 7 maintainers not CCed: lishifeng@sangfor.com.cn wenglianfa@huawei.com chuck.lever@oracle.com xuhaoyue1@hisilicon.com linux-rdma@vger.kernel.org phaddad@nvidia.com benve@cisco.com
netdev/build_clang success Errors and warnings before: 954 this patch: 954
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 959 this patch: 959
netdev/checkpatch warning WARNING: From:/Signed-off-by: email address mismatch: 'From: Denis Kirjanov <kirjanov@gmail.com>' != 'Signed-off-by: Denis Kirjanov <dkirjanov@suse.de>' WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: d41861942fc5 ("IB/core: Add generic function to extract IB speed from netdev")'
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-01--18-00 (tests: 951)

Commit Message

Denis Kirjanov April 1, 2024, 10 a.m. UTC 
A call to ib_device_get_netdev may lead to a race condition
while accessing a netdevice instance since we don't hold
the rtnl lock while checking
the registration state:
	if (res && res->reg_state != NETREG_REGISTERED) {

v2: unlock rtnl on error path
v3: update remaining callers of ib_device_get_netdev
v4: don't call a cb with rtnl lock in ib_enum_roce_netdev

Reported-by: syzbot+5fe14f2ff4ccbace9a26@syzkaller.appspotmail.com
Fixes: d41861942fc55 ("IB/core: Add generic function to extract IB speed from netdev")
Signed-off-by: Denis Kirjanov <dkirjanov@suse.de>
---
 drivers/infiniband/core/cache.c  |  2 ++
 drivers/infiniband/core/device.c | 15 ++++++++++++---
 drivers/infiniband/core/nldev.c  |  3 +++
 drivers/infiniband/core/verbs.c  |  6 ++++--
 4 files changed, 21 insertions(+), 5 deletions(-)

Comments

Ratheesh Kannoth April 1, 2024, 1:35 p.m. UTC | #1 
On 2024-04-01 at 15:30:05, Denis Kirjanov (kirjanov@gmail.com) wrote:
> A call to ib_device_get_netdev may lead to a race condition
> while accessing a netdevice instance since we don't hold
> the rtnl lock while checking
> the registration state:
> 	if (res && res->reg_state != NETREG_REGISTERED) {
>
> v2: unlock rtnl on error path
> v3: update remaining callers of ib_device_get_netdev
> v4: don't call a cb with rtnl lock in ib_enum_roce_netdev
>
> Reported-by: syzbot+5fe14f2ff4ccbace9a26@syzkaller.appspotmail.com
> Fixes: d41861942fc55 ("IB/core: Add generic function to extract IB speed from netdev")
> Signed-off-by: Denis Kirjanov <dkirjanov@suse.de>
> ---
>  drivers/infiniband/core/cache.c  |  2 ++
>  drivers/infiniband/core/device.c | 15 ++++++++++++---
>  drivers/infiniband/core/nldev.c  |  3 +++
>  drivers/infiniband/core/verbs.c  |  6 ++++--
>  4 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> index c02a96d3572a..cf9c826cd520 100644
> --- a/drivers/infiniband/core/cache.c
> +++ b/drivers/infiniband/core/cache.c
> @@ -1461,7 +1461,9 @@ static int config_non_roce_gid_cache(struct ib_device *device,
>  		if (rdma_protocol_iwarp(device, port)) {
>  			struct net_device *ndev;
>
> +			rtnl_lock();
>  			ndev = ib_device_get_netdev(device, port);
> +			rtnl_unlock();
Why dont you move the rtnl_lock()/_unlock() inside ib_device_get_netdev().
ib_device_get_netdev() hold ref to dev, so can access ndev safely here.

>  			if (!ndev)
>  				continue;
>  			RCU_INIT_POINTER(gid_attr.ndev, ndev);
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 07cb6c5ffda0..25edb50d2b64 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -2026,9 +2026,12 @@ static int iw_query_port(struct ib_device *device,
>
>  	memset(port_attr, 0, sizeof(*port_attr));
>
> +	rtnl_lock();
>  	netdev = ib_device_get_netdev(device, port_num);
> -	if (!netdev)
> +	if (!netdev) {
> +		rtnl_unlock();
>  		return -ENODEV;
> +	}
>
>  	port_attr->max_mtu = IB_MTU_4096;
>  	port_attr->active_mtu = ib_mtu_int_to_enum(netdev->mtu);
> @@ -2052,6 +2055,7 @@ static int iw_query_port(struct ib_device *device,
>  		rcu_read_unlock();
>  	}
>
> +	rtnl_unlock();
>  	dev_put(netdev);
>  	return device->ops.query_port(device, port_num, port_attr);
>  }
> @@ -2220,6 +2224,8 @@ struct net_device *ib_device_get_netdev(struct ib_device *ib_dev,
>  	struct ib_port_data *pdata;
>  	struct net_device *res;
>
> +	ASSERT_RTNL();
> +
>  	if (!rdma_is_port_valid(ib_dev, port))
>  		return NULL;
>
> @@ -2306,8 +2312,11 @@ void ib_enum_roce_netdev(struct ib_device *ib_dev,
>
>  	rdma_for_each_port (ib_dev, port)
>  		if (rdma_protocol_roce(ib_dev, port)) {
> -			struct net_device *idev =
> -				ib_device_get_netdev(ib_dev, port);
> +			struct net_device *idev;
> +
> +			rtnl_lock();
> +			idev = ib_device_get_netdev(ib_dev, port);
> +			rtnl_unlock();
>
>  			if (filter(ib_dev, port, idev, filter_cookie))
>  				cb(ib_dev, port, idev, cookie);
> diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
> index 4900a0848124..5cf7cdae8925 100644
> --- a/drivers/infiniband/core/nldev.c
> +++ b/drivers/infiniband/core/nldev.c
> @@ -360,7 +360,9 @@ static int fill_port_info(struct sk_buff *msg,
>  	if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, attr.phys_state))
>  		return -EMSGSIZE;
>
> +	rtnl_lock();
>  	netdev = ib_device_get_netdev(device, port);
> +
>  	if (netdev && net_eq(dev_net(netdev), net)) {
>  		ret = nla_put_u32(msg,
>  				  RDMA_NLDEV_ATTR_NDEV_INDEX, netdev->ifindex);
> @@ -371,6 +373,7 @@ static int fill_port_info(struct sk_buff *msg,
>  	}
>
>  out:
> +	rtnl_unlock();
>  	dev_put(netdev);
>  	return ret;
>  }
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 94a7f3b0c71c..6a3757b00c93 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -1976,11 +1976,13 @@ int ib_get_eth_speed(struct ib_device *dev, u32 port_num, u16 *speed, u8 *width)
>  	if (rdma_port_get_link_layer(dev, port_num) != IB_LINK_LAYER_ETHERNET)
>  		return -EINVAL;
>
> +	rtnl_lock();
>  	netdev = ib_device_get_netdev(dev, port_num);
> -	if (!netdev)
> +	if (!netdev) {
> +		rtnl_unlock();
>  		return -ENODEV;
> +	}
>
> -	rtnl_lock();
>  	rc = __ethtool_get_link_ksettings(netdev, &lksettings);
>  	rtnl_unlock();
>
> --
> 2.30.2
>
Denis Kirjanov April 2, 2024, 1:05 p.m. UTC | #2 
On 4/1/24 16:35, Ratheesh Kannoth wrote:
> On 2024-04-01 at 15:30:05, Denis Kirjanov (kirjanov@gmail.com) wrote:
>> A call to ib_device_get_netdev may lead to a race condition
>> while accessing a netdevice instance since we don't hold
>> the rtnl lock while checking
>> the registration state:
>> 	if (res && res->reg_state != NETREG_REGISTERED) {
>>
>> v2: unlock rtnl on error path
>> v3: update remaining callers of ib_device_get_netdev
>> v4: don't call a cb with rtnl lock in ib_enum_roce_netdev
>>
>> Reported-by: syzbot+5fe14f2ff4ccbace9a26@syzkaller.appspotmail.com
>> Fixes: d41861942fc55 ("IB/core: Add generic function to extract IB speed from netdev")
>> Signed-off-by: Denis Kirjanov <dkirjanov@suse.de>
>> ---
>>  drivers/infiniband/core/cache.c  |  2 ++
>>  drivers/infiniband/core/device.c | 15 ++++++++++++---
>>  drivers/infiniband/core/nldev.c  |  3 +++
>>  drivers/infiniband/core/verbs.c  |  6 ++++--
>>  4 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
>> index c02a96d3572a..cf9c826cd520 100644
>> --- a/drivers/infiniband/core/cache.c
>> +++ b/drivers/infiniband/core/cache.c
>> @@ -1461,7 +1461,9 @@ static int config_non_roce_gid_cache(struct ib_device *device,
>>  		if (rdma_protocol_iwarp(device, port)) {
>>  			struct net_device *ndev;
>>
>> +			rtnl_lock();
>>  			ndev = ib_device_get_netdev(device, port);
>> +			rtnl_unlock();
> Why dont you move the rtnl_lock()/_unlock() inside ib_device_get_netdev().
> ib_device_get_netdev() hold ref to dev, so can access ndev safely here.

Makes sense. and it makes the whole change pretty small
> 
>>  			if (!ndev)
>>  				continue;
>>  			RCU_INIT_POINTER(gid_attr.ndev, ndev);
>> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
>> index 07cb6c5ffda0..25edb50d2b64 100644
>> --- a/drivers/infiniband/core/device.c
>> +++ b/drivers/infiniband/core/device.c
>> @@ -2026,9 +2026,12 @@ static int iw_query_port(struct ib_device *device,
>>
>>  	memset(port_attr, 0, sizeof(*port_attr));
>>
>> +	rtnl_lock();
>>  	netdev = ib_device_get_netdev(device, port_num);
>> -	if (!netdev)
>> +	if (!netdev) {
>> +		rtnl_unlock();
>>  		return -ENODEV;
>> +	}
>>
>>  	port_attr->max_mtu = IB_MTU_4096;
>>  	port_attr->active_mtu = ib_mtu_int_to_enum(netdev->mtu);
>> @@ -2052,6 +2055,7 @@ static int iw_query_port(struct ib_device *device,
>>  		rcu_read_unlock();
>>  	}
>>
>> +	rtnl_unlock();
>>  	dev_put(netdev);
>>  	return device->ops.query_port(device, port_num, port_attr);
>>  }
>> @@ -2220,6 +2224,8 @@ struct net_device *ib_device_get_netdev(struct ib_device *ib_dev,
>>  	struct ib_port_data *pdata;
>>  	struct net_device *res;
>>
>> +	ASSERT_RTNL();
>> +
>>  	if (!rdma_is_port_valid(ib_dev, port))
>>  		return NULL;
>>
>> @@ -2306,8 +2312,11 @@ void ib_enum_roce_netdev(struct ib_device *ib_dev,
>>
>>  	rdma_for_each_port (ib_dev, port)
>>  		if (rdma_protocol_roce(ib_dev, port)) {
>> -			struct net_device *idev =
>> -				ib_device_get_netdev(ib_dev, port);
>> +			struct net_device *idev;
>> +
>> +			rtnl_lock();
>> +			idev = ib_device_get_netdev(ib_dev, port);
>> +			rtnl_unlock();
>>
>>  			if (filter(ib_dev, port, idev, filter_cookie))
>>  				cb(ib_dev, port, idev, cookie);
>> diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
>> index 4900a0848124..5cf7cdae8925 100644
>> --- a/drivers/infiniband/core/nldev.c
>> +++ b/drivers/infiniband/core/nldev.c
>> @@ -360,7 +360,9 @@ static int fill_port_info(struct sk_buff *msg,
>>  	if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, attr.phys_state))
>>  		return -EMSGSIZE;
>>
>> +	rtnl_lock();
>>  	netdev = ib_device_get_netdev(device, port);
>> +
>>  	if (netdev && net_eq(dev_net(netdev), net)) {
>>  		ret = nla_put_u32(msg,
>>  				  RDMA_NLDEV_ATTR_NDEV_INDEX, netdev->ifindex);
>> @@ -371,6 +373,7 @@ static int fill_port_info(struct sk_buff *msg,
>>  	}
>>
>>  out:
>> +	rtnl_unlock();
>>  	dev_put(netdev);
>>  	return ret;
>>  }
>> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
>> index 94a7f3b0c71c..6a3757b00c93 100644
>> --- a/drivers/infiniband/core/verbs.c
>> +++ b/drivers/infiniband/core/verbs.c
>> @@ -1976,11 +1976,13 @@ int ib_get_eth_speed(struct ib_device *dev, u32 port_num, u16 *speed, u8 *width)
>>  	if (rdma_port_get_link_layer(dev, port_num) != IB_LINK_LAYER_ETHERNET)
>>  		return -EINVAL;
>>
>> +	rtnl_lock();
>>  	netdev = ib_device_get_netdev(dev, port_num);
>> -	if (!netdev)
>> +	if (!netdev) {
>> +		rtnl_unlock();
>>  		return -ENODEV;
>> +	}
>>
>> -	rtnl_lock();
>>  	rc = __ethtool_get_link_ksettings(netdev, &lksettings);
>>  	rtnl_unlock();
>>
>> --
>> 2.30.2
>>
diff mbox series

Patch

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index c02a96d3572a..cf9c826cd520 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -1461,7 +1461,9 @@  static int config_non_roce_gid_cache(struct ib_device *device,
 		if (rdma_protocol_iwarp(device, port)) {
 			struct net_device *ndev;
 
+			rtnl_lock();
 			ndev = ib_device_get_netdev(device, port);
+			rtnl_unlock();
 			if (!ndev)
 				continue;
 			RCU_INIT_POINTER(gid_attr.ndev, ndev);
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 07cb6c5ffda0..25edb50d2b64 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2026,9 +2026,12 @@  static int iw_query_port(struct ib_device *device,
 
 	memset(port_attr, 0, sizeof(*port_attr));
 
+	rtnl_lock();
 	netdev = ib_device_get_netdev(device, port_num);
-	if (!netdev)
+	if (!netdev) {
+		rtnl_unlock();
 		return -ENODEV;
+	}
 
 	port_attr->max_mtu = IB_MTU_4096;
 	port_attr->active_mtu = ib_mtu_int_to_enum(netdev->mtu);
@@ -2052,6 +2055,7 @@  static int iw_query_port(struct ib_device *device,
 		rcu_read_unlock();
 	}
 
+	rtnl_unlock();
 	dev_put(netdev);
 	return device->ops.query_port(device, port_num, port_attr);
 }
@@ -2220,6 +2224,8 @@  struct net_device *ib_device_get_netdev(struct ib_device *ib_dev,
 	struct ib_port_data *pdata;
 	struct net_device *res;
 
+	ASSERT_RTNL();
+
 	if (!rdma_is_port_valid(ib_dev, port))
 		return NULL;
 
@@ -2306,8 +2312,11 @@  void ib_enum_roce_netdev(struct ib_device *ib_dev,
 
 	rdma_for_each_port (ib_dev, port)
 		if (rdma_protocol_roce(ib_dev, port)) {
-			struct net_device *idev =
-				ib_device_get_netdev(ib_dev, port);
+			struct net_device *idev;
+
+			rtnl_lock();
+			idev = ib_device_get_netdev(ib_dev, port);
+			rtnl_unlock();
 
 			if (filter(ib_dev, port, idev, filter_cookie))
 				cb(ib_dev, port, idev, cookie);
diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
index 4900a0848124..5cf7cdae8925 100644
--- a/drivers/infiniband/core/nldev.c
+++ b/drivers/infiniband/core/nldev.c
@@ -360,7 +360,9 @@  static int fill_port_info(struct sk_buff *msg,
 	if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, attr.phys_state))
 		return -EMSGSIZE;
 
+	rtnl_lock();
 	netdev = ib_device_get_netdev(device, port);
+
 	if (netdev && net_eq(dev_net(netdev), net)) {
 		ret = nla_put_u32(msg,
 				  RDMA_NLDEV_ATTR_NDEV_INDEX, netdev->ifindex);
@@ -371,6 +373,7 @@  static int fill_port_info(struct sk_buff *msg,
 	}
 
 out:
+	rtnl_unlock();
 	dev_put(netdev);
 	return ret;
 }
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 94a7f3b0c71c..6a3757b00c93 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1976,11 +1976,13 @@  int ib_get_eth_speed(struct ib_device *dev, u32 port_num, u16 *speed, u8 *width)
 	if (rdma_port_get_link_layer(dev, port_num) != IB_LINK_LAYER_ETHERNET)
 		return -EINVAL;
 
+	rtnl_lock();
 	netdev = ib_device_get_netdev(dev, port_num);
-	if (!netdev)
+	if (!netdev) {
+		rtnl_unlock();
 		return -ENODEV;
+	}
 
-	rtnl_lock();
 	rc = __ethtool_get_link_ksettings(netdev, &lksettings);
 	rtnl_unlock();