Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Wed, 13 Sep 2023 13:31:09 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     Pin-yen Lin <treapking@chromium.org>
Cc:     linux-wireless@vger.kernel.org, Kalle Valo <kvalo@kernel.org>,
        Polaris Pi <pinkperfect2021@gmail.com>,
        Matthew Wang <matthewmwang@chromium.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in
 mwifiex_process_rx_packet
Message-ID: <ZQIcDWKrmgoPkwlN@google.com>
References: <20230908104308.1546501-1-treapking@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230908104308.1546501-1-treapking@chromium.org>
Precedence: bulk

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
>