2023-08-08 18:08:16

by Dmitry Antipov

[permalink] [raw]
Subject: [PATCH] wifi: mwifiex: avoid possible NULL skb pointer dereference

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



2023-08-08 21:14:26

by Brian Norris

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

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 <[email protected]>
> ---
> 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 <[email protected]>

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

2023-08-09 10:14:40

by Dmitry Antipov

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

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


2023-08-09 20:39:03

by Brian Norris

[permalink] [raw]
Subject: Bug in commit 119585281617 wifi: mwifiex: Fix OOB and integer underflow when rx packets

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

2023-08-14 10:12:18

by Dmitry Antipov

[permalink] [raw]
Subject: [PATCH] [v2] wifi: mwifiex: avoid possible NULL skb pointer dereference

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")
Acked-by: Brian Norris <[email protected]>
Signed-off-by: Dmitry Antipov <[email protected]>
---
v2: increment RX drop count rather than TX one (Brian Norris)
---
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..a8a9986102a2 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.rx_dropped++;
+ dev_kfree_skb_any(skb);
+ return -1;
+ }
} else {
if (mwifiex_get_sta_entry(priv, ra)) {
/* Requeue Intra-BSS packet */
--
2.41.0


2023-08-23 13:29:57

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] [v2] wifi: mwifiex: avoid possible NULL skb pointer dereference

Dmitry Antipov <[email protected]> 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")
> Acked-by: Brian Norris <[email protected]>
> Signed-off-by: Dmitry Antipov <[email protected]>

Patch applied to wireless-next.git, thanks.

35a7a1ce7c7d wifi: mwifiex: avoid possible NULL skb pointer dereference

--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches