Re: [PATCH] wifi: mwifiex: avoid possible NULL skb pointer dereference

[Brian Norris <briannorris@chromium.org>]

On Tue, Aug 08, 2023 at 11:44:27AM +0300, Dmitry Antipov wrote:
> In 'mwifiex_handle_uap_rx_forward()', always check the value
> returned by 'skb_copy()' to avoid potential NULL pointer
> dereference in 'mwifiex_uap_queue_bridged_pkt()', and drop
> original skb in case of copying failure.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 838e4f449297 ("mwifiex: improve uAP RX handling")
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> index 04ff051f5d18..454d1c11d39b 100644
> --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> @@ -252,7 +252,15 @@ int mwifiex_handle_uap_rx_forward(struct mwifiex_private *priv,
>  
>  	if (is_multicast_ether_addr(ra)) {
>  		skb_uap = skb_copy(skb, GFP_ATOMIC);
> -		mwifiex_uap_queue_bridged_pkt(priv, skb_uap);
> +		if (likely(skb_uap)) {
> +			mwifiex_uap_queue_bridged_pkt(priv, skb_uap);
> +		} else {
> +			mwifiex_dbg(adapter, ERROR,
> +				    "failed to copy skb for uAP\n");
> +			priv->stats.tx_dropped++;

This feels like it should be 'rx_dropped', since we're dropping it
before we done any real "RX" (let alone getting to any forward/outbound
operation). I doubt it makes a big difference overall, but it seems like
the right thing to do.

Otherwise, this looks good; feel free to carry this to a next revision
if you're just changing tx_dropped to rx_dropped:

Acked-by: Brian Norris <briannorris@chromium.org>

> +			dev_kfree_skb_any(skb);
> +			return -1;
> +		}
>  	} else {
>  		if (mwifiex_get_sta_entry(priv, ra)) {
>  			/* Requeue Intra-BSS packet */
> -- 
> 2.41.0
>