RE: [PATCH net-next v5 2/9] r8152: split RTL_VER_17 into QFN68 and QFN100 package variants

[Chih Kai Hsu]

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 (1) · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each issue of
> Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork know
> by including "pw-bot: cr" as a separate line at the end of your reply (one such
> reply per series is enough).
> 
> - [Medium] The commit message says the split is needed because QFN68 and
>   QFN100 "need different firmware and a package-specific u2phy backup-…
> - [Medium] In __rtl_get_pkg_det() (drivers/net/usb/r8152.c), the retry
>   loop `for (i = 0, ret = 0; i < 3 && ret != 4; i++) ret =…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but in
>   __rtl_get_hw_ver() (drivers/net/usb/r8152.c) the buffer `tmp` is…
> 
> --
> 
> r8152: split RTL_VER_17 into QFN68 and QFN100 package variants
> 
> This commit splits RTL_VER_17 into RTL_VER_17_QFN68 and
> RTL_VER_17_QFN100.
> The package is detected by a new __rtl_get_pkg_det() helper that reads
> PLA_PKG_DET. Every RTL_VER_17 switch case now handles both variants, and
> r8159_init() gains a u2phy backup restore write that only applies to QFN68.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 4d8f0815d79d1..30af6ed3a638f 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -10172,7 +10194,8 @@ static int rtl_fw_init(struct r8152 *tp)
> >       case RTL_VER_16:
> >               rtl_fw->fw_name         = FIRMWARE_8157_1;
> >               break;
> > -     case RTL_VER_17:
> > +     case RTL_VER_17_QFN68:
> > +     case RTL_VER_17_QFN100:
> >               rtl_fw->fw_name         = FIRMWARE_8159_1;
> 
> [Severity: Medium]
> The commit message says the two packages "need different firmware and a
> package-specific u2phy backup-restore patch". However, rtl_fw_init() still gives
> both RTL_VER_17_QFN68 and RTL_VER_17_QFN100 the same firmware,
> FIRMWARE_8159_1 ("rtl_nic/rtl8159-1.fw").
> 
> The driver still has only one 8159 firmware define and one
> MODULE_FIRMWARE(FIRMWARE_8159_1). The later patches in the series, up
> to
> "r8152: enable UPS for RTL8157 and RTL8159 QFN68", do not change this
> either.
> 
> Is per-package firmware selection missing from this patch? Or should the
> commit message drop the claim about different firmware?

I will reword the commit message to drop the claim.

> 
> >               break;
> >       default:
> > @@ -10182,9 +10205,38 @@ static int rtl_fw_init(struct r8152 *tp)
> >       return 0;
> >  }
> >
> > +static u32 __rtl_get_pkg_det(struct usb_device *udev) {
> > +     u32 pkg_det = 0;
> > +     __le32 *tmp;
> > +     int ret, i;
> > +
> > +     tmp = kmalloc_obj(*tmp);
> > +     if (!tmp)
> > +             return 0;
> > +
> > +     for (i = 0, ret = 0; i < 3 && ret != 4; i++)
> > +             ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> > +                                   RTL8152_REQ_GET_REGS,
> RTL8152_REQT_READ,
> > +                                   PLA_PKG_DET, MCU_TYPE_PLA,
> tmp,
> > +                                   sizeof(*tmp),
> > + USB_CTRL_GET_TIMEOUT);
> > +
> > +     if (ret == sizeof(*tmp)) {
> > +             if (i != 0)
> > +                     dev_warn(&udev->dev,
> > +                              "Needed %d retries to read
> package\n",
> > + i);
> 
> [Severity: Medium]
> Won't this warning fire on every successful read? The loop increments i after
> the iteration that succeeds. If the first usb_control_msg() returns 4, i is already
> 1 when the loop exits, so the i != 0 check is always true.

Actually, I will fix it.

> 
> The printed count is also one too high: 1 for a first-try success, 3 for a success
> on the third attempt.
> 
> The version read loop in __rtl_get_hw_ver() avoids this by breaking out on
> success:
> 
>         for (i = 0; i < 3; i++) {
>                 ret = usb_control_msg(...);
>                 if (ret > 0) {
>                         ocp_data = (__le32_to_cpu(*tmp) >> 16) &
> VERSION_MASK;
>                         break;
>                 }
>         }
> 
>         if (i != 0 && ret > 0)
>                 dev_warn(&udev->dev, "Needed %d retries to read
> version\n", i);
> 
> __rtl_get_hw_ver() runs from both the cfgselector and the probe paths, so the
> false warning can appear more than once per RTL8159 plug-in. The code is still
> the same at the end of the series.
> 
> > +
> > +             pkg_det = __le32_to_cpu(*tmp) & PKG_MASK;
> > +     }
> > +
> > +     kfree(tmp);
> > +     return pkg_det;
> > +}
> > +
> >  static u8 __rtl_get_hw_ver(struct usb_device *udev)  {
> >       u32 ocp_data = 0;
> > +     u32 pkg_det = 0;
> >       __le32 *tmp;
> >       u8 version;
> >       int ret;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the PLA_TCR0 read in
> __rtl_get_hw_ver() accepts a reply of any positive length:
> 
>         tmp = kmalloc_obj(*tmp);
>         ...
>                 if (ret > 0) {
>                         ocp_data = (__le32_to_cpu(*tmp) >> 16) &
> VERSION_MASK;
>                         break;
>                 }
> 
> kmalloc_obj() does not zero the buffer. If a faulty or malicious device sends
> only 1 to 3 bytes, can the upper bytes of *tmp still hold leftover heap data that
> then gets decoded as the chip version?
> 
> That could misidentify the chip, or leak uninitialized heap bytes to dmesg
> through the "Unknown version 0x%04x" message.
> 
> The new __rtl_get_pkg_det() helper already requires ret == sizeof(*tmp).
> Would the same check make sense here?
> 
> [ ... ]
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001091144.28191-1
> -nic_swsd%40realtek.com

Best Regards,
Chih-Kai

---
pw-bot: cr