Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jernej =?utf-8?B?xaBrcmFiZWM=?= <jernej.skrabec@gmail.com>
To:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Larry Finger <Larry.Finger@lwfinger.net>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>
Subject: Re: Driver for rtw8723ds
Date:   Sat, 13 May 2023 22:13:39 +0200
Message-ID: <21872829.EfDdHjke4D@jernej-laptop>
In-Reply-To: <f3368bf9-c6e6-f418-41da-b9de185acd34@lwfinger.net>
References: <72a8eeb1-c91c-80a7-5a09-1b7963e0996b@lwfinger.net>
 <13262218.uLZWGnKmhe@jernej-laptop>
 <f3368bf9-c6e6-f418-41da-b9de185acd34@lwfinger.net>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nextPart7520481.EvYhyI6sBW"
Content-Transfer-Encoding: 7Bit
Precedence: bulk

This is a multi-part message in MIME format.

--nextPart7520481.EvYhyI6sBW
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"

Dne sobota, 13. maj 2023 ob 21:55:51 CEST je Larry Finger napisal(a):
> On 5/13/23 05:23, Jernej =C5=A0krabec wrote:
> > Larry,
> >=20
> > Dne sreda, 10. maj 2023 ob 23:47:24 CEST je Larry Finger napisal(a):
> >> On 5/10/23 16:07, Martin Blumenstingl wrote:
> >>> On Wed, May 10, 2023 at 12:02=E2=80=AFAM Larry Finger <Larry.Finger@l=
wfinger.net> wrote:
> >>> [...]
> >>>> I added that patch to the driver. The user reports that he was able =
to do a ping
> >>>> and an nslookup before it crashed with the following in the log:
> >>> That's some positive news alongside the crash log: it seems that a
> >>> part of the driver works! :-)
> >>>
> >>>> [    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:334=
1 put:3341
> >>>> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev=
:<NULL>
> >>> [...]
> >>>> Somehow skb->tail was greater than skb->end. Unfortunately I do not =
have access
> >>>> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on =
the MangoPi
> >>>> MQ Quad.
> >>> I need to have a closer look at the pkg_offset and struct
> >>> rtw_rx_pkt_stat which we receive.
> >>> Recently my own MangoPI MQ-Quad arrived but I did not have the time to
> >>> set it up yet. I'll try to do so during the weekend so I can debug
> >>> this on my own.
> >>>
> >>> Please ping me next week in case I haven't provided any update until =
then.
> >>
> >> I have some test prints in to check for skb overrun. My initial indica=
tion is
> >> that the problem was in the c2h branch of rtw_sdio_rx_skb(), but my ne=
xt run
> >> should verify that. My changes will do a pr_warn_once() when the probl=
em
> >> happens, and then drop the skb.
> >>
> >> My contact reported that he had one run of 3 minutes before the problem
> >> happened, which is good news for most of the driver.
> >=20
> > I may have discovered something interesting. rtl8723ds vendor driver has
> > following checks in RX data parsing code:
> > https://github.com/lwfinger/rtl8723ds/blob/master/hal/rtl8723d/sdio/rtl=
8723ds_recv.c#L83-L99
> >=20
> > Those checks are absent in rtl8822bs vendor driver, which was my origin=
al
> > development platform for SDIO. This may indicate some kind of bug in FW
> > and/or HW.
> >=20
> > I think that at least second check, which checks for exactly the case y=
our
> > client experience, can be easily added and tested.
>=20
> Thanks for this update. I added the following to the start of rtw_sdio_rx=
_skb():
>         /* fix Hardware RX data error, drop whole recv_buffer */
>         if (!(rtwdev->hal.rcr & BIT_ACRC32) && pkt_stat->crc_err) {
>                 kfree_skb(skb);
>                 return;
>         }
> I think that duplicates the code in the vendor driver.
>=20
> I have not heard from my user as to whether it helps. My communications w=
ith him=20
> are at https://github.com/lwfinger/rtl8723ds/issues/37.

I had second part in mind (see attachment), but this is IMO only sanity che=
ck
and it will mask the issue. At this point I'm not sure if this is something=
 that
can happen occasionally or is there additional bug in rtw88 code. I'll check
rtl8723ds c2h code in greater detail.

In any case, I would argue that all 3 patches in this thread are valid and
should be submitted upstream.

Best regards,
Jernej


--nextPart7520481.EvYhyI6sBW
Content-Disposition: attachment; filename="sdio-size-check.patch"
Content-Transfer-Encoding: 7Bit
Content-Type: text/x-patch; charset="utf-8"; name="sdio-size-check.patch"

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index af0459a79899..d69ce76ad0f4 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -975,7 +975,13 @@ static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
 		curr_pkt_len = ALIGN(pkt_offset + pkt_stat.pkt_len,
 				     RTW_SDIO_DATA_PTR_ALIGN);
 
-		if ((curr_pkt_len + pkt_desc_sz) >= rx_len) {
+		if ((curr_pkt_len + pkt_desc_sz) > rx_len) {
+			dev_warn(rtwdev->dev, "Invalid RX packet size!");
+			dev_kfree_skb_any(skb);
+			return;
+		}
+
+		if ((curr_pkt_len + pkt_desc_sz) == rx_len) {
 			/* Use the original skb (with it's adjusted offset)
 			 * when processing the last (or even the only) entry to
 			 * have it's memory freed automatically.

--nextPart7520481.EvYhyI6sBW--