Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9ad63828-3c85-fcc4-a91d-58e1d16b60b7@yandex.ru>
Date:   Wed, 9 Aug 2023 12:35:37 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Content-Language: en-US
To:     Brian Norris <briannorris@chromium.org>
Cc:     Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org,
        lvc-project@linuxtesting.org
References: <20230808084431.43548-1-dmantipov@yandex.ru>
 <ZNKhb2lgSmfEqqsW@google.com>
From:   Dmitry Antipov <dmantipov@yandex.ru>
Subject: Re: [PATCH] wifi: mwifiex: avoid possible NULL skb pointer
 dereference
In-Reply-To: <ZNKhb2lgSmfEqqsW@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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