* [PATCH] wifi: mwifiex: avoid possible NULL skb pointer dereference
@ 2023-08-08 8:44 Dmitry Antipov
2023-08-08 20:11 ` Brian Norris
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Antipov @ 2023-08-08 8:44 UTC (permalink / raw)
To: Brian Norris; +Cc: Kalle Valo, linux-wireless, lvc-project, Dmitry Antipov
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++;
+ dev_kfree_skb_any(skb);
+ return -1;
+ }
} else {
if (mwifiex_get_sta_entry(priv, ra)) {
/* Requeue Intra-BSS packet */
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] wifi: mwifiex: avoid possible NULL skb pointer dereference
2023-08-08 8:44 [PATCH] wifi: mwifiex: avoid possible NULL skb pointer dereference Dmitry Antipov
@ 2023-08-08 20:11 ` Brian Norris
2023-08-09 9:35 ` Dmitry Antipov
2023-08-14 9:49 ` [PATCH] [v2] wifi: mwifiex: avoid possible NULL skb pointer dereference Dmitry Antipov
0 siblings, 2 replies; 6+ messages in thread
From: Brian Norris @ 2023-08-08 20:11 UTC (permalink / raw)
To: Dmitry Antipov; +Cc: Kalle Valo, linux-wireless, lvc-project
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] wifi: mwifiex: avoid possible NULL skb pointer dereference
2023-08-08 20:11 ` Brian Norris
@ 2023-08-09 9:35 ` Dmitry Antipov
2023-08-09 20:32 ` Bug in commit 119585281617 wifi: mwifiex: Fix OOB and integer underflow when rx packets Brian Norris
2023-08-14 9:49 ` [PATCH] [v2] wifi: mwifiex: avoid possible NULL skb pointer dereference Dmitry Antipov
1 sibling, 1 reply; 6+ messages in thread
From: Dmitry Antipov @ 2023-08-09 9:35 UTC (permalink / raw)
To: Brian Norris; +Cc: Kalle Valo, linux-wireless, lvc-project
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Bug in commit 119585281617 wifi: mwifiex: Fix OOB and integer underflow when rx packets
2023-08-09 9:35 ` Dmitry Antipov
@ 2023-08-09 20:32 ` Brian Norris
0 siblings, 0 replies; 6+ messages in thread
From: Brian Norris @ 2023-08-09 20:32 UTC (permalink / raw)
To: Dmitry Antipov, Polaris Pi, Matthew Wang
Cc: Kalle Valo, linux-wireless, lvc-project, Johannes Berg
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] [v2] wifi: mwifiex: avoid possible NULL skb pointer dereference
2023-08-08 20:11 ` Brian Norris
2023-08-09 9:35 ` Dmitry Antipov
@ 2023-08-14 9:49 ` Dmitry Antipov
2023-08-23 11:11 ` Kalle Valo
1 sibling, 1 reply; 6+ messages in thread
From: Dmitry Antipov @ 2023-08-14 9:49 UTC (permalink / raw)
To: Brian Norris; +Cc: Kalle Valo, linux-wireless, lvc-project, Dmitry Antipov
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 <briannorris@chromium.org>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-23 11:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-08 8:44 [PATCH] wifi: mwifiex: avoid possible NULL skb pointer dereference Dmitry Antipov
2023-08-08 20:11 ` Brian Norris
2023-08-09 9:35 ` Dmitry Antipov
2023-08-09 20:32 ` Bug in commit 119585281617 wifi: mwifiex: Fix OOB and integer underflow when rx packets Brian Norris
2023-08-14 9:49 ` [PATCH] [v2] wifi: mwifiex: avoid possible NULL skb pointer dereference Dmitry Antipov
2023-08-23 11:11 ` Kalle Valo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.