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 <[email protected]>
---
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
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 <[email protected]>
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
>
Matthew Wang <[email protected]> writes:
> lgtm
>
> Reviewed-by: Matthew Wang <[email protected]>
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