diff mbox series

wifi: mwifiex: avoid possible NULL skb pointer dereference

Message ID 20230808084431.43548-1-dmantipov@yandex.ru (mailing list archive)
State Superseded
Delegated to: Kalle Valo
Headers show
Series wifi: mwifiex: avoid possible NULL skb pointer dereference | expand

Commit Message

Dmitry Antipov Aug. 8, 2023, 8:44 a.m. UTC
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(-)

Comments

Brian Norris Aug. 8, 2023, 8:11 p.m. UTC | #1
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
>
Dmitry Antipov Aug. 9, 2023, 9:35 a.m. UTC | #2
On 8/8/23 23:11, Brian Norris wrote:

> 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.

This is somewhat confusing for me indeed. In 'mwifiex_uap_queue_bridged_pkt()',
both 'rx_dropped' and 'tx_dropped' may be incremented, for a different reasons
(unexpected skb layout and error (re)allocating new skb, respectively).

And I have some doubts on 119585281617 ("wifi: mwifiex: Fix OOB and integer
underflow when rx packets"). Looking through 'mwifiex_uap_queue_bridged_pkt()'
again, it seems that 'return' is missing:

	if (sizeof(*rx_pkt_hdr) +
	    le16_to_cpu(uap_rx_pd->rx_pkt_offset) > skb->len) {
		mwifiex_dbg(adapter, ERROR,
			    "wrong rx packet offset: len=%d,rx_pkt_offset=%d\n",
			    skb->len, le16_to_cpu(uap_rx_pd->rx_pkt_offset));
		priv->stats.rx_dropped++;
		dev_kfree_skb_any(skb);
                /* HERE */
	}

	if ((!memcmp(&rx_pkt_hdr->rfc1042_hdr, bridge_tunnel_header,

because 'rx_pkt_hdr' points to 'skb->data' plus some offset (see above),
so reading freed memory with 'memcmp()' causes an undefined behavior.
And likewise for 'mwifiex_process_rx_packet()' (but not for
'mwifiex_process_uap_rx_packet()' where 'return 0' looks correct).

Dmitry
Brian Norris Aug. 9, 2023, 8:32 p.m. UTC | #3
On Wed, Aug 09, 2023 at 12:35:37PM +0300, Dmitry Antipov wrote:
> And I have some doubts on 119585281617 ("wifi: mwifiex: Fix OOB and integer
> underflow when rx packets"). Looking through 'mwifiex_uap_queue_bridged_pkt()'
> again, it seems that 'return' is missing:
> 
> 	if (sizeof(*rx_pkt_hdr) +
> 	    le16_to_cpu(uap_rx_pd->rx_pkt_offset) > skb->len) {
> 		mwifiex_dbg(adapter, ERROR,
> 			    "wrong rx packet offset: len=%d,rx_pkt_offset=%d\n",
> 			    skb->len, le16_to_cpu(uap_rx_pd->rx_pkt_offset));
> 		priv->stats.rx_dropped++;
> 		dev_kfree_skb_any(skb);
>                /* HERE */
> 	}
> 
> 	if ((!memcmp(&rx_pkt_hdr->rfc1042_hdr, bridge_tunnel_header,
> 
> because 'rx_pkt_hdr' points to 'skb->data' plus some offset (see above),
> so reading freed memory with 'memcmp()' causes an undefined behavior.
> And likewise for 'mwifiex_process_rx_packet()' (but not for
> 'mwifiex_process_uap_rx_packet()' where 'return 0' looks correct).

That's...completely unrelated to the post in question, so changing the
subject. But it's also an excellent (and terrible) catch.

Polars or Matthew, can you fix that up in a new patch ASAP?

CC Johannes, in case this patch is going places any time soon.

Brian
diff mbox series

Patch

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++;
+			dev_kfree_skb_any(skb);
+			return -1;
+		}
 	} else {
 		if (mwifiex_get_sta_entry(priv, ra)) {
 			/* Requeue Intra-BSS packet */