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;
Date:   Wed, 9 Aug 2023 13:32:45 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     Dmitry Antipov <dmantipov@yandex.ru>,
        Polaris Pi <pinkperfect2021@gmail.com>,
        Matthew Wang <matthewmwang@chromium.org>
Cc:     Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org,
        lvc-project@linuxtesting.org,
        Johannes Berg <johannes@sipsolutions.net>
Subject: Bug in commit 119585281617 wifi: mwifiex: Fix OOB and integer
 underflow when rx packets
Message-ID: <ZNP37WfcNfCWB1yX@google.com>
References: <20230808084431.43548-1-dmantipov@yandex.ru>
 <ZNKhb2lgSmfEqqsW@google.com>
 <9ad63828-3c85-fcc4-a91d-58e1d16b60b7@yandex.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9ad63828-3c85-fcc4-a91d-58e1d16b60b7@yandex.ru>
Precedence: bulk

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