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

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Mar 2023 19:37:03 +0100
From:   Piotr Raczynski <piotr.raczynski@...el.com>
To:     Nick Child <nnac123@...ux.ibm.com>
CC:     <netdev@...r.kernel.org>
Subject: Re: [PATCH net 1/2] net: Catch invalid index in XPS mapping

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@...ux.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@...el.com>