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