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@xxxxxxxxxxxx>
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
>
