Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
MIME-Version: 1.0
References: <20230908104308.1546501-1-treapking@chromium.org>
 <ZQIcDWKrmgoPkwlN@google.com> <CAEXTbpc=QC6wC-W2VZCaRCp6rSpyNSsq5M6cxNcqAQxciNj0vg@mail.gmail.com>
 <ZQOZZZgHP2EeDNix@google.com>
In-Reply-To: <ZQOZZZgHP2EeDNix@google.com>
From:   Matthew Wang <matthewmwang@chromium.org>
Date:   Mon, 18 Sep 2023 09:50:37 +0200
Message-ID: <CABRiz0ph56X48Y0VjS1yDEUW3=kihE4+WSHPuFVQv-CCdR=0cw@mail.gmail.com>
Subject: Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet
To:     Brian Norris <briannorris@chromium.org>
Cc:     Pin-yen Lin <treapking@chromium.org>,
        linux-wireless@vger.kernel.org, Kalle Valo <kvalo@kernel.org>,
        Polaris Pi <pinkperfect2021@gmail.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

lgtm

Reviewed-by: Matthew Wang <matthewmwang@chromium.org>

On Fri, Sep 15, 2023 at 1:38=E2=80=AFAM Brian Norris <briannorris@chromium.=
org> wrote:
>
> On Thu, Sep 14, 2023 at 03:09:47PM +0800, Pin-yen Lin wrote:
> > On Thu, Sep 14, 2023 at 4:31=E2=80=AFAM Brian Norris <briannorris@chrom=
ium.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) !=3D ETH_P_AARP=
 &&
> > > > -          ntohs(rx_pkt_hdr->rfc1042_hdr.snap_type) !=3D ETH_P_IPX)=
) {
> > > > +     if (sizeof(*rx_pkt_hdr) + rx_pkt_off <=3D 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/wireles=
s/intersil/hostap/hostap_80211_rx.c#L1035
> > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/net/wireles=
s/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@chromium.org>