| Message ID | 20230317181941.86151-1-nnac123@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] net: Catch invalid index in XPS mapping | expand |
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>
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..
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;
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(+)