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

[netdev-bot+sashiko@kernel.org]

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?

>  		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.

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