drivers/net/usb/ch9200.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
The successful register writes, done via the USB control requests in
control_write(), seem vital for the normal functioning of the device,
however the driver's bind() method ignores error codes returned from
control_write(). Do bail out of ch9200_bind() on failed writes...
Found by Linux Verification Center (linuxtesting.org) with the Svace
static analysis tool.
Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>
---
Changes in version 2:
- switched to checking for the negative error values instead of non-zero;
- added the Fixes tag, retargeting the patch to the net.git repo;
- dropped [RFT] from the subject.
drivers/net/usb/ch9200.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index a206ffa76f1b..ab3cd3902ed7 100644
--- a/drivers/net/usb/ch9200.c
+++ b/drivers/net/usb/ch9200.c
@@ -360,32 +360,44 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
data[1] = 0x0F;
retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_THRESHOLD, data,
0x02, CONTROL_TIMEOUT_MS);
+ if (retval < 0)
+ return retval;
data[0] = 0xA0;
data[1] = 0x90;
retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_FIFO_DEPTH, data,
0x02, CONTROL_TIMEOUT_MS);
+ if (retval < 0)
+ return retval;
data[0] = 0x30;
data[1] = 0x00;
retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_PAUSE, data,
0x02, CONTROL_TIMEOUT_MS);
+ if (retval < 0)
+ return retval;
data[0] = 0x17;
data[1] = 0xD8;
retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_FLOW_CONTROL,
data, 0x02, CONTROL_TIMEOUT_MS);
+ if (retval < 0)
+ return retval;
/* Undocumented register */
data[0] = 0x01;
data[1] = 0x00;
retval = control_write(dev, REQUEST_WRITE, 0, 254, data, 0x02,
CONTROL_TIMEOUT_MS);
+ if (retval < 0)
+ return retval;
data[0] = 0x5F;
data[1] = 0x0D;
retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_CTRL, data, 0x02,
CONTROL_TIMEOUT_MS);
+ if (retval < 0)
+ return retval;
retval = get_mac_address(dev, addr);
eth_hw_addr_set(dev->net, addr);
--
2.55.0Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 0 · Low: 0
- [High] Ineffective fix: the six newly added `if (retval < 0) return
retval;` checks in ch9200_bind() (drivers/net/usb/ch9200.c) cannot…
--
ch9200: return error on failed register writes in ch9200_bind()
The patch adds error checks after the six control_write() calls in
ch9200_bind() so that a failed vendor register write aborts the bind
instead of being ignored. It carries a Fixes tag for the original driver
commit 4a476bd6d1d9.
> diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
> index a206ffa76f1b9..ab3cd3902ed72 100644
> --- a/drivers/net/usb/ch9200.c
> +++ b/drivers/net/usb/ch9200.c
> @@ -360,32 +360,44 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
> data[1] = 0x0F;
> retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_THRESHOLD, data,
> 0x02, CONTROL_TIMEOUT_MS);
> + if (retval < 0)
> + return retval;
[Severity: High]
Can these new checks ever observe a failed register write? control_write()
in the same file discards the USB transfer status on its normal path:
drivers/net/usb/ch9200.c:control_write() {
...
err = usb_control_msg(dev->udev,
usb_sndctrlpipe(dev->udev, 0),
request, request_type, value, index, buf, size,
timeout);
if (err >= 0 && err < size)
err = -EINVAL;
kfree(buf);
return 0;
err_out:
return err;
}
The only return err; is at err_out:, which is reached solely from the
kmemdup() -ENOMEM branch. All six patched call sites pass a non-NULL
2-byte data buffer, so the only negative value the new checks can see is
-ENOMEM from that allocation.
A STALL of the vendor OUT request (-EPIPE), a timeout (-ETIMEDOUT), a
disconnect during bind (-ENODEV/-ESHUTDOWN), a protocol error (-EPROTO),
or a short control transfer (normalized to -EINVAL just above) are all
turned into 0, so ch9200_bind() still falls through to
get_mac_address()/eth_hw_addr_set() and reports success with
MAC_REG_THRESHOLD, MAC_REG_FIFO_DEPTH, MAC_REG_PAUSE,
MAC_REG_FLOW_CONTROL, register 254 and MAC_REG_CTRL (which arms TX/RX)
left unprogrammed.
The sibling helper control_read() already returns err on its normal path,
and ch9200_mdio_read() checks it with if (ret < 0), so the convention
control_write() is expected to follow already exists in this file.
Would it make more sense to change control_write() to return err after
kfree(buf) instead of (or in addition to) adding the caller-side checks?
As it stands the commit message claim that this makes the driver "bail out
of ch9200_bind() on failed writes" does not seem to hold for actual
transfer failures.
>
> data[0] = 0xA0;
> data[1] = 0x90;
> retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_FIFO_DEPTH, data,
> 0x02, CONTROL_TIMEOUT_MS);
> + if (retval < 0)
> + return retval;
[ ... remaining four checks have the same question ... ]
> retval = get_mac_address(dev, addr);
> eth_hw_addr_set(dev->net, addr);
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914202503.52920-1-s.shtylyov%40auroraos.dev
On 9/16/26 2:20 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
>
> - [High] Ineffective fix: the six newly added `if (retval < 0) return
> retval;` checks in ch9200_bind() (drivers/net/usb/ch9200.c) cannot…
>
> --
>
> ch9200: return error on failed register writes in ch9200_bind()
>
> The patch adds error checks after the six control_write() calls in
> ch9200_bind() so that a failed vendor register write aborts the bind
> instead of being ignored. It carries a Fixes tag for the original driver
> commit 4a476bd6d1d9.
>
>> diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
>> index a206ffa76f1b9..ab3cd3902ed72 100644
>> --- a/drivers/net/usb/ch9200.c
>> +++ b/drivers/net/usb/ch9200.c
>> @@ -360,32 +360,44 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
>> data[1] = 0x0F;
>> retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_THRESHOLD, data,
>> 0x02, CONTROL_TIMEOUT_MS);
>> + if (retval < 0)
>> + return retval;
>
> [Severity: High]
But why? 8-)
> Can these new checks ever observe a failed register write? control_write()
> in the same file discards the USB transfer status on its normal path:
By "failed register write" I meant any error in control_write(), including
kmemdup() failure (which precludes the USB request from being sent at all)...
> drivers/net/usb/ch9200.c:control_write() {
> ...
> err = usb_control_msg(dev->udev,
> usb_sndctrlpipe(dev->udev, 0),
> request, request_type, value, index, buf, size,
> timeout);
> if (err >= 0 && err < size)
> err = -EINVAL;
> kfree(buf);
>
> return 0;
>
> err_out:
> return err;
> }
>
> The only return err; is at err_out:, which is reached solely from the
> kmemdup() -ENOMEM branch. All six patched call sites pass a non-NULL
> 2-byte data buffer, so the only negative value the new checks can see is
> -ENOMEM from that allocation.
I kinda noticed. :-)
[...]
> The sibling helper control_read() already returns err on its normal path,
> and ch9200_mdio_read() checks it with if (ret < 0), so the convention
> control_write() is expected to follow already exists in this file.
And I'm extending it to control_write(), but in another patch...
> Would it make more sense to change control_write() to return err after
> kfree(buf) instead of (or in addition to) adding the caller-side checks?
> As it stands the commit message claim that this makes the driver "bail out
> of ch9200_bind() on failed writes" does not seem to hold for actual
> transfer failures.
I'm getting bored...
If any *human* maintainer insists that I merge the 2 patches, I surely
will... Or maybe I should resubmit both against net.git (net-next.git?) as
a series?
[...]
MBR, Sergey
On 9/16/26 18:58, Sergey Shtylyov wrote:
> On 9/16/26 2:20 AM, netdev-bot+sashiko@kernel.org wrote:
>
>> Thank you for your contribution! Sashiko AI review found 1 potential
>> issue(s) to consider:
>>
>> Critical: 0 · High: 1 · Medium: 0 · Low: 0
>>
>> - [High] Ineffective fix: the six newly added `if (retval < 0) return
>> retval;` checks in ch9200_bind() (drivers/net/usb/ch9200.c) cannot…
>>
>> --
>>
>> ch9200: return error on failed register writes in ch9200_bind()
>>
>> The patch adds error checks after the six control_write() calls in
>> ch9200_bind() so that a failed vendor register write aborts the bind
>> instead of being ignored. It carries a Fixes tag for the original driver
>> commit 4a476bd6d1d9.
>>
>>> diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
>>> index a206ffa76f1b9..ab3cd3902ed72 100644
>>> --- a/drivers/net/usb/ch9200.c
>>> +++ b/drivers/net/usb/ch9200.c
>>> @@ -360,32 +360,44 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
>>> data[1] = 0x0F;
>>> retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_THRESHOLD, data,
>>> 0x02, CONTROL_TIMEOUT_MS);
>>> + if (retval < 0)
>>> + return retval;
>>
>> [Severity: High]
>
> But why? 8-)
>
>> Can these new checks ever observe a failed register write? control_write()
>> in the same file discards the USB transfer status on its normal path:
>
> By "failed register write" I meant any error in control_write(), including
> kmemdup() failure (which precludes the USB request from being sent at all)...
>
>> drivers/net/usb/ch9200.c:control_write() {
>> ...
>> err = usb_control_msg(dev->udev,
>> usb_sndctrlpipe(dev->udev, 0),
>> request, request_type, value, index, buf, size,
>> timeout);
>> if (err >= 0 && err < size)
>> err = -EINVAL;
>> kfree(buf);
>>
>> return 0;
>>
>> err_out:
>> return err;
>> }
>>
>> The only return err; is at err_out:, which is reached solely from the
>> kmemdup() -ENOMEM branch. All six patched call sites pass a non-NULL
>> 2-byte data buffer, so the only negative value the new checks can see is
>> -ENOMEM from that allocation.
>
> I kinda noticed. :-)
>
> [...]
>
>> The sibling helper control_read() already returns err on its normal path,
>> and ch9200_mdio_read() checks it with if (ret < 0), so the convention
>> control_write() is expected to follow already exists in this file.
>
> And I'm extending it to control_write(), but in another patch...
>
>> Would it make more sense to change control_write() to return err after
>> kfree(buf) instead of (or in addition to) adding the caller-side checks?
>> As it stands the commit message claim that this makes the driver "bail out
>> of ch9200_bind() on failed writes" does not seem to hold for actual
>> transfer failures.
>
> I'm getting bored...
> If any *human* maintainer insists that I merge the 2 patches, I surely
> will... Or maybe I should resubmit both against net.git (net-next.git?) as
> a series?
IMHO the patches are so strictly interconnected that should land into
the same (net-next) series. No fixes tags in net-next patches.
Thanks,
Paolo
On Mon, Sep 14, 2026 at 11:25:02PM +0300, Sergey Shtylyov wrote:
> The successful register writes, done via the USB control requests in
> control_write(), seem vital for the normal functioning of the device,
> however the driver's bind() method ignores error codes returned from
> control_write(). Do bail out of ch9200_bind() on failed writes...
>
> Found by Linux Verification Center (linuxtesting.org) with the Svace
> static analysis tool.
>
> Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
> Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
© 2016 - 2026 Red Hat, Inc.