linux-kernel - Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZQOZZZgHP2EeDNix@google.com>
Date:   Thu, 14 Sep 2023 16:38:13 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Pin-yen Lin <treapking@...omium.org>
Cc:     linux-wireless@...r.kernel.org, Kalle Valo <kvalo@...nel.org>,
        Polaris Pi <pinkperfect2021@...il.com>,
        Matthew Wang <matthewmwang@...omium.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in
 mwifiex_process_rx_packet

On Thu, Sep 14, 2023 at 03:09:47PM +0800, Pin-yen Lin wrote:
> On Thu, Sep 14, 2023 at 4:31 AM Brian Norris <briannorris@...omium.org> wrote:
> > I'd appreciate another review/test from one of the others here
> > (Matthew?), even though I know y'all are already working together.

I'd still appreciate some comment here.

> > > -     if ((!memcmp(&rx_pkt_hdr->rfc1042_hdr, bridge_tunnel_header,
> > > -                  sizeof(bridge_tunnel_header))) ||
> > > -         (!memcmp(&rx_pkt_hdr->rfc1042_hdr, rfc1042_header,
> > > -                  sizeof(rfc1042_header)) &&
> > > -          ntohs(rx_pkt_hdr->rfc1042_hdr.snap_type) != ETH_P_AARP &&
> > > -          ntohs(rx_pkt_hdr->rfc1042_hdr.snap_type) != ETH_P_IPX)) {
> > > +     if (sizeof(*rx_pkt_hdr) + rx_pkt_off <= skb->len &&
> >
> > Are you sure you want this length check to fall back to the non-802.3
> > codepath? Isn't it an error to look like an 802.3 frame but to be too
> > small? I'd think we want to drop such packets, not process them as-is.
> 
> I did that because I saw other drivers (e.g., [1], [2]) use similar
> approaches, and I assumed that the rest of the pipeline will
> eventually drop it if the packet cannot be recognized. But, yes, we
> can just drop the packet here if it doesn't look good.
> 
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/intersil/hostap/hostap_80211_rx.c#L1035
> [2]: https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/intel/ipw2x00/libipw_rx.c#L735

Hmm, I suppose. I'm frankly not sure how exactly all upper layers handle
this, but at least in a non-raw mode, we'll drop them. (We might be
delivering awfully weird packets to tcpdump though, but this is already
a weird situation, if it's such a weird-looking packet.)

> > If I'm correct, then this check should move inside the 'if' branch of
> > this if/else.
> 
> We can't simply move the check inside the if branch because the
> condition also checks rx_pkt_hdr->rfc1042_hdr.snap_type. Though, of
> course, it is doable by adding another `if` conditions.

Right.

I guess this is probably OK as-is:

Acked-by: Brian Norris <briannorris@...omium.org>