[PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet

[PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet

[Pin-yen Lin]

Only skip the code path trying to access the rfc1042 headers when the
buffer is too small, so the driver can still process packets without
rfc1042 headers.

Fixes: 119585281617 ("wifi: mwifiex: Fix OOB and integer underflow when rx packets")
Signed-off-by: Pin-yen Lin <treapking@chromium.org>

---

Changes in v3:
- Really apply the sizeof call fix as it was missed in the previous patch

Changes in v2:
- Fix sizeof call (sizeof(rx_pkt_hdr) --> sizeof(*rx_pkt_hdr))

 drivers/net/wireless/marvell/mwifiex/sta_rx.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sta_rx.c b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
index 65420ad67416..257737137cd7 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_rx.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
@@ -86,7 +86,8 @@ int mwifiex_process_rx_packet(struct mwifiex_private *priv,
 	rx_pkt_len = le16_to_cpu(local_rx_pd->rx_pkt_length);
 	rx_pkt_hdr = (void *)local_rx_pd + rx_pkt_off;
 
-	if (sizeof(*rx_pkt_hdr) + rx_pkt_off > skb->len) {
+	if (sizeof(rx_pkt_hdr->eth803_hdr) + sizeof(rfc1042_header) +
+	    rx_pkt_off > skb->len) {
 		mwifiex_dbg(priv->adapter, ERROR,
 			    "wrong rx packet offset: len=%d, rx_pkt_off=%d\n",
 			    skb->len, rx_pkt_off);
@@ -95,12 +96,13 @@ int mwifiex_process_rx_packet(struct mwifiex_private *priv,
 		return -1;
 	}
 
-	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 &&
+	    ((!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))) {
 		/*
 		 *  Replace the 803 header and rfc1042 header (llc/snap) with an
 		 *    EthernetII header, keep the src/dst and snap_type
-- 
2.42.0.283.g2d96d420d3-goog

Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet

[Brian Norris]

On Fri, Sep 08, 2023 at 06:41:12PM +0800, Pin-yen Lin wrote:
> Only skip the code path trying to access the rfc1042 headers when the
> buffer is too small, so the driver can still process packets without
> rfc1042 headers.
> 
> Fixes: 119585281617 ("wifi: mwifiex: Fix OOB and integer underflow when rx packets")
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>

I'd appreciate another review/test from one of the others here
(Matthew?), even though I know y'all are already working together.

> ---
> 
> Changes in v3:
> - Really apply the sizeof call fix as it was missed in the previous patch
> 
> Changes in v2:
> - Fix sizeof call (sizeof(rx_pkt_hdr) --> sizeof(*rx_pkt_hdr))
> 
>  drivers/net/wireless/marvell/mwifiex/sta_rx.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_rx.c b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
> index 65420ad67416..257737137cd7 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_rx.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
> @@ -86,7 +86,8 @@ int mwifiex_process_rx_packet(struct mwifiex_private *priv,
>  	rx_pkt_len = le16_to_cpu(local_rx_pd->rx_pkt_length);
>  	rx_pkt_hdr = (void *)local_rx_pd + rx_pkt_off;
>  
> -	if (sizeof(*rx_pkt_hdr) + rx_pkt_off > skb->len) {
> +	if (sizeof(rx_pkt_hdr->eth803_hdr) + sizeof(rfc1042_header) +
> +	    rx_pkt_off > skb->len) {
>  		mwifiex_dbg(priv->adapter, ERROR,
>  			    "wrong rx packet offset: len=%d, rx_pkt_off=%d\n",
>  			    skb->len, rx_pkt_off);
> @@ -95,12 +96,13 @@ int mwifiex_process_rx_packet(struct mwifiex_private *priv,
>  		return -1;
>  	}
>  
> -	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.

If I'm correct, then this check should move inside the 'if' branch of
this if/else.

Brian

> +	    ((!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))) {
>  		/*
>  		 *  Replace the 803 header and rfc1042 header (llc/snap) with an
>  		 *    EthernetII header, keep the src/dst and snap_type
> -- 
> 2.42.0.283.g2d96d420d3-goog
> 

Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet

[Pin-yen Lin]

Hi Brian,

Thanks for the review.

On Thu, Sep 14, 2023 at 4:31 AM Brian Norris <briannorris@chromium.org> wrote:
>
> On Fri, Sep 08, 2023 at 06:41:12PM +0800, Pin-yen Lin wrote:
> > Only skip the code path trying to access the rfc1042 headers when the
> > buffer is too small, so the driver can still process packets without
> > rfc1042 headers.
> >
> > Fixes: 119585281617 ("wifi: mwifiex: Fix OOB and integer underflow when rx packets")
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
>
> I'd appreciate another review/test from one of the others here
> (Matthew?), even though I know y'all are already working together.
>
> > ---
> >
> > Changes in v3:
> > - Really apply the sizeof call fix as it was missed in the previous patch
> >
> > Changes in v2:
> > - Fix sizeof call (sizeof(rx_pkt_hdr) --> sizeof(*rx_pkt_hdr))
> >
> >  drivers/net/wireless/marvell/mwifiex/sta_rx.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sta_rx.c b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
> > index 65420ad67416..257737137cd7 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sta_rx.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
> > @@ -86,7 +86,8 @@ int mwifiex_process_rx_packet(struct mwifiex_private *priv,
> >       rx_pkt_len = le16_to_cpu(local_rx_pd->rx_pkt_length);
> >       rx_pkt_hdr = (void *)local_rx_pd + rx_pkt_off;
> >
> > -     if (sizeof(*rx_pkt_hdr) + rx_pkt_off > skb->len) {
> > +     if (sizeof(rx_pkt_hdr->eth803_hdr) + sizeof(rfc1042_header) +
> > +         rx_pkt_off > skb->len) {
> >               mwifiex_dbg(priv->adapter, ERROR,
> >                           "wrong rx packet offset: len=%d, rx_pkt_off=%d\n",
> >                           skb->len, rx_pkt_off);
> > @@ -95,12 +96,13 @@ int mwifiex_process_rx_packet(struct mwifiex_private *priv,
> >               return -1;
> >       }
> >
> > -     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
>
> 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.
>
> Brian
>

Regards,
Pin-yen

> > +         ((!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))) {
> >               /*
> >                *  Replace the 803 header and rfc1042 header (llc/snap) with an
> >                *    EthernetII header, keep the src/dst and snap_type
> > --
> > 2.42.0.283.g2d96d420d3-goog
> >

Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet

[Brian Norris]

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@chromium.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@chromium.org>

Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet

[Matthew Wang]

lgtm

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

On Fri, Sep 15, 2023 at 1:38 AM 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 AM Brian Norris <briannorris@chromium.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@chromium.org>

Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet

[Kalle Valo]

Matthew Wang <matthewmwang@chromium.org> writes:

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

Please don't top post, it's just bad in so many levels. This has been
discussed and explained in our docs so many times that I'm not going to
repeat those anymore. If you are too busy to edit your quotes and reply
properly then it's better not to reply at all.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet

[Kalle Valo]

Pin-yen Lin <treapking@chromium.org> wrote:

> Only skip the code path trying to access the rfc1042 headers when the
> buffer is too small, so the driver can still process packets without
> rfc1042 headers.
> 
> Fixes: 119585281617 ("wifi: mwifiex: Fix OOB and integer underflow when rx packets")
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> Acked-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Matthew Wang <matthewmwang@chromium.org>

Patch applied to wireless.git, thanks.

aef7a0300047 wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230908104308.1546501-1-treapking@chromium.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet

[Matthew Wang]

Sorry, my mistake, I was unaware.