| 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 |
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 >
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
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 --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 */
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(-)