Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 8 Aug 2023 13:11:27 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     Dmitry Antipov <dmantipov@yandex.ru>
Cc:     Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org,
        lvc-project@linuxtesting.org
Subject: Re: [PATCH] wifi: mwifiex: avoid possible NULL skb pointer
 dereference
Message-ID: <ZNKhb2lgSmfEqqsW@google.com>
References: <20230808084431.43548-1-dmantipov@yandex.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230808084431.43548-1-dmantipov@yandex.ru>
Precedence: bulk

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
>