[PATCH net 1/2] net: Catch invalid index in XPS mapping

[PATCH net 1/2] net: Catch invalid index in XPS mapping

[Nick Child]

When setting the XPS value of a TX queue, add a conditional to ensure
that the index of the queue is less than the number of allocated TX
queues.

Previously, this scenario went uncaught. In the best case, it resulted
in unnecessary allocations. In the worst case, it resulted in
out-of-bounds memory references through calls to `netdev_get_tx_queue(
dev, index)`.

Fixes: 537c00de1c9b ("net: Add functions netif_reset_xps_queue and netif_set_xps_queue")
Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
This is a result of my own foolish mistake of giving an invalid
index to __netif_set_xps_queue [1]. While the function adds the queue to
the cpu's XPS queue map, the queue is never used due to a conditional
in __get_xps_queue_idx. But there is a risk of random memory reading
and writing that should be prevented.

1. https://lore.kernel.org/netdev/20230224183659.2a7bfeea@kernel.org/

 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index c7853192563d..cd3878043846 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2535,6 +2535,9 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
 	struct xps_map *map, *new_map;
 	unsigned int nr_ids;
 
+	if (index >= dev->num_tx_queues)
+		return -EINVAL;
+
 	if (dev->num_tc) {
 		/* Do not allow XPS on subordinate device directly */
 		num_tc = dev->num_tc;
-- 
2.31.1

[PATCH net 2/2] netdev: Enforce index cap in netdev_get_tx_queue

[Nick Child]

When requesting a TX queue at a given index, prevent out-of-bounds
referencing by ensuring that the index is within the allocated number
of queues.

If there is an out-of-bounds reference then inform the user and return
a reference to the first tx queue instead.

Fixes: e8a0464cc950 ("netdev: Allocate multiple queues for TX.")
Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 include/linux/netdevice.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 23b0d7eaaadd..fe88b1a7393d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2482,6 +2482,13 @@ static inline
 struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev,
 					 unsigned int index)
 {
+	if (unlikely(index >= dev->num_tx_queues)) {
+		net_warn_ratelimited("%s selects TX queue %d, but number of TX queues is %d\n",
+				     dev->name, index,
+				     dev->num_tx_queues);
+		return &dev->_tx[0];
+	}
+
 	return &dev->_tx[index];
 }
 
-- 
2.31.1

Re: [PATCH net 2/2] netdev: Enforce index cap in netdev_get_tx_queue

[Piotr Raczynski]

On Fri, Mar 17, 2023 at 01:19:41PM -0500, Nick Child wrote:
> When requesting a TX queue at a given index, prevent out-of-bounds
> referencing by ensuring that the index is within the allocated number
> of queues.
> 
> If there is an out-of-bounds reference then inform the user and return
> a reference to the first tx queue instead.
> 
> Fixes: e8a0464cc950 ("netdev: Allocate multiple queues for TX.")
> Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> ---
>  include/linux/netdevice.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 23b0d7eaaadd..fe88b1a7393d 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2482,6 +2482,13 @@ static inline
>  struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev,
>  					 unsigned int index)
>  {
> +	if (unlikely(index >= dev->num_tx_queues)) {
> +		net_warn_ratelimited("%s selects TX queue %d, but number of TX queues is %d\n",
> +				     dev->name, index,
> +				     dev->num_tx_queues);
> +		return &dev->_tx[0];

Why return first queue here instead of NULL, wouldn't that confuse the
caller instead of return proper (NULL) value?

Piotr.
> +	}
> +
>  	return &dev->_tx[index];
>  }
>  
> -- 
> 2.31.1
> 

Re: [PATCH net 2/2] netdev: Enforce index cap in netdev_get_tx_queue

[Nick Child]

On 3/17/23 13:45, Piotr Raczynski wrote:
> On Fri, Mar 17, 2023 at 01:19:41PM -0500, Nick Child wrote:
>> When requesting a TX queue at a given index, prevent out-of-bounds
>> referencing by ensuring that the index is within the allocated number
>> of queues.
>>
>> If there is an out-of-bounds reference then inform the user and return
>> a reference to the first tx queue instead.
>>
>> Fixes: e8a0464cc950 ("netdev: Allocate multiple queues for TX.")
>> Signed-off-by: Nick Child <nnac123@linux.ibm.com>
>> ---
>>   include/linux/netdevice.h | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 23b0d7eaaadd..fe88b1a7393d 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -2482,6 +2482,13 @@ static inline
>>   struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev,
>>   					 unsigned int index)
>>   {
>> +	if (unlikely(index >= dev->num_tx_queues)) {
>> +		net_warn_ratelimited("%s selects TX queue %d, but number of TX queues is %d\n",
>> +				     dev->name, index,
>> +				     dev->num_tx_queues);
>> +		return &dev->_tx[0];
> 
> Why return first queue here instead of NULL, wouldn't that confuse the
> caller instead of return proper (NULL) value?
> 

Thanks for reviewing Piotr.
netdev_get_tx_queue has over 300 callers, most of these calls
use the returned queue immediately without any checking on
the returned value. I don't expect all of these callers
to go and add conditionals to handle this case either.

So I opted for the warning message and a valid return value.
That being said, I am open to more opinions.


> Piotr.
>> +	}
>> +
>>   	return &dev->_tx[index];
>>   }
>>   
>> -- 
>> 2.31.1
>>

Re: [PATCH net 2/2] netdev: Enforce index cap in netdev_get_tx_queue

[Jakub Kicinski]

On Fri, 17 Mar 2023 13:19:41 -0500 Nick Child wrote:
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 23b0d7eaaadd..fe88b1a7393d 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2482,6 +2482,13 @@ static inline
>  struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev,
>  					 unsigned int index)
>  {
> +	if (unlikely(index >= dev->num_tx_queues)) {
> +		net_warn_ratelimited("%s selects TX queue %d, but number of TX queues is %d\n",
> +				     dev->name, index,
> +				     dev->num_tx_queues);
> +		return &dev->_tx[0];
> +	}
> +

Should we maybe do DEBUG_NET_WARN_ON_ONCE() instead?
It will likely run multiple times per each Tx packet,
so I wonder if we really want to add a branch for what's
effectively defensive programming...

Re: [PATCH net 1/2] net: Catch invalid index in XPS mapping

[Piotr Raczynski]

On Fri, Mar 17, 2023 at 01:19:40PM -0500, Nick Child wrote:
> When setting the XPS value of a TX queue, add a conditional to ensure
> that the index of the queue is less than the number of allocated TX
> queues.
> 
> Previously, this scenario went uncaught. In the best case, it resulted
> in unnecessary allocations. In the worst case, it resulted in
> out-of-bounds memory references through calls to `netdev_get_tx_queue(
> dev, index)`.
> 
> Fixes: 537c00de1c9b ("net: Add functions netif_reset_xps_queue and netif_set_xps_queue")
> Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> ---
> This is a result of my own foolish mistake of giving an invalid
> index to __netif_set_xps_queue [1]. While the function adds the queue to
> the cpu's XPS queue map, the queue is never used due to a conditional
> in __get_xps_queue_idx. But there is a risk of random memory reading
> and writing that should be prevented.
> 
> 1. https://lore.kernel.org/netdev/20230224183659.2a7bfeea@kernel.org/
> 
>  net/core/dev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c7853192563d..cd3878043846 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2535,6 +2535,9 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
>  	struct xps_map *map, *new_map;
>  	unsigned int nr_ids;
>  
> +	if (index >= dev->num_tx_queues)
> +		return -EINVAL;
> +
>  	if (dev->num_tc) {
>  		/* Do not allow XPS on subordinate device directly */
>  		num_tc = dev->num_tc;
> -- 
> 2.31.1
> 
Reasonable check added, thanks.
Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com>

Re: [PATCH net 1/2] net: Catch invalid index in XPS mapping

[Jakub Kicinski]

On Fri, 17 Mar 2023 13:19:40 -0500 Nick Child wrote:
> +	if (index >= dev->num_tx_queues)
> +		return -EINVAL;

WARN_ON_ONCE()? On a quick grep virtio does not check return value 
for example. Others may assume this never fails and not print any
warning, and users will have "fun time" figuring out why their machine
fell of the network / where is the probe error coming from..

Also seems like net-next material? Why do we consider this a fix?
It's defensive / debug check, ain't no bug to assume callers are sane..