Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [High] If `usb_pcwd_probe()` fails, the global `cards_found` counter is not decremented, permanently preventing any subsequent devices from being probed.
- [High] `usb_pcwd_temperature_read` copies exactly 1 byte from a 32-bit `int` to userspace, causing incorrect temperature readings on both little-endian and big-endian architectures.
- [High] Error checking in `usb_pcwd_get_temperature` and `usb_pcwd_get_timeleft` ignores a return value of `0` from `usb_pcwd_send_command`, falsely treating timeouts as success.
- [High] Concurrent `open()` and `disconnect()` can leave the hardware watchdog armed after the driver unbinds.
- [High] Unconditional busy-wait loop executes even if the USB control message fails, causing a 250ms kernel thread stall.
--
--- Patch [1]: [PATCH] watchdog: pcwd_usb: keep device alive for open files ---
commit 832556793ce76f6a3d61309293d57fe82b13b7f2
Author: Qing Ming <[email protected]>
Subject: watchdog: pcwd_usb: keep device alive for open files
This patch addresses a use-after-free issue when reading temperature files
across USB disconnects. It stores the device object in file->private_data
and manages its lifecycle via a kref reference count. The disconnect logic
is updated to serialize teardown and quiesce the device properly.
Link: Cc: [email protected]
> diff --git a/drivers/watchdog/pcwd_usb.c b/drivers/watchdog/pcwd_usb.c
> index d7c18c990649a..9ef7a4a472846 100644
> --- a/drivers/watchdog/pcwd_usb.c
> +++ b/drivers/watchdog/pcwd_usb.c
[ ... ]
> @@ -258,6 +270,8 @@ static int usb_pcwd_send_command(struct usb_pcwd_private *usb_pcwd,
>
> kfree(buf);
>
> +out_unlock:
> + mutex_unlock(&usb_pcwd->mtx);
> return got_response;
> }
[Severity: High]
This is a pre-existing issue, but does this function stall a kernel thread for
250ms if the control message fails?
Looking at usb_pcwd_send_command(), if usb_control_msg() fails (e.g., due to a
device unplug), the code proceeds directly into a polling loop:
for (count = 0; (count < USB_COMMAND_TIMEOUT) && (!got_response); count++) {
mdelay(1);
...
}
Since got_response will remain 0, wouldn't this busy-wait for the full timeout
(250 iterations of mdelay), causing unnecessary CPU spinning and delaying
other USB processing?
> @@ -327,8 +341,11 @@ static int usb_pcwd_get_temperature(struct usb_pcwd_private *usb_pcwd,
> {
> unsigned char msb = 0x00;
> unsigned char lsb = 0x00;
> + int ret;
>
> - usb_pcwd_send_command(usb_pcwd, CMD_READ_TEMP, &msb, &lsb);
> + ret = usb_pcwd_send_command(usb_pcwd, CMD_READ_TEMP, &msb, &lsb);
> + if (ret < 0)
> + return ret;
[Severity: High]
This isn't a bug introduced by this patch, but does checking ret < 0
miss the error case where usb_pcwd_send_command() returns 0?
usb_pcwd_send_command() returns 0 on timeout or memory allocation failure.
If a command times out, wouldn't this function proceed to calculate the result
using uninitialized or zero-initialized local buffers for msb and lsb,
reporting fake values to userspace instead of propagating the error?
> @@ -344,10 +361,14 @@ static int usb_pcwd_get_timeleft(struct usb_pcwd_private *usb_pcwd,
> {
> unsigned char msb = 0x00;
> unsigned char lsb = 0x00;
> + int ret;
>
> /* Read the time that's left before rebooting */
> /* Note: if the board is not yet armed then we will read 0xFFFF */
> - usb_pcwd_send_command(usb_pcwd, CMD_READ_WATCHDOG_TIMEOUT, &msb, &lsb);
> + ret = usb_pcwd_send_command(usb_pcwd, CMD_READ_WATCHDOG_TIMEOUT,
> + &msb, &lsb);
> + if (ret < 0)
> + return ret;
[Severity: High]
This is a pre-existing issue, but similar to the temperature function above,
does checking ret < 0 fail to catch the 0 return value on timeout?
If the timeout occurs, it looks like it will calculate time_left using
the zero-initialized msb and lsb buffers.
[ ... ]
> @@ -476,29 +504,55 @@ static long usb_pcwd_ioctl(struct file *file, unsigned int cmd,
>
> static int usb_pcwd_open(struct inode *inode, struct file *file)
> {
> + struct usb_pcwd_private *usb_pcwd;
> + int ret;
> +
> + mutex_lock(&disconnect_mutex);
> + usb_pcwd = usb_pcwd_device;
> + if (!usb_pcwd || !usb_pcwd->exists) {
> + ret = -ENODEV;
> + goto out_unlock;
> + }
> /* /dev/watchdog can only be opened once */
> - if (test_and_set_bit(0, &is_active))
> - return -EBUSY;
> + if (test_and_set_bit(0, &is_active)) {
> + ret = -EBUSY;
> + goto out_unlock;
> + }
> + kref_get(&usb_pcwd->kref);
> + file->private_data = usb_pcwd;
> + mutex_unlock(&disconnect_mutex);
>
> /* Activate */
> - usb_pcwd_start(usb_pcwd_device);
> - usb_pcwd_keepalive(usb_pcwd_device);
> - return stream_open(inode, file);
> + usb_pcwd_start(usb_pcwd);
[Severity: High]
This is a pre-existing issue, but can concurrent execution of usb_pcwd_open()
and usb_pcwd_disconnect() leave the hardware watchdog armed after the driver
unbinds?
If usb_pcwd_open() checks usb_pcwd->exists and then drops disconnect_mutex:
Thread 1 in usb_pcwd_open():
mutex_unlock(&disconnect_mutex);
Could Thread 2 in usb_pcwd_disconnect() acquire the mutex, clear
usb_pcwd_device, drop the mutex, and execute usb_pcwd_stop() before setting
exists to 0?
Thread 2 in usb_pcwd_disconnect():
usb_pcwd_stop(usb_pcwd);
mutex_lock(&usb_pcwd->mtx);
usb_pcwd->exists = 0;
mutex_unlock(&usb_pcwd->mtx);
If Thread 1 then executes usb_pcwd_start(usb_pcwd) right after Thread 2
completes its stop, wouldn't the hardware watchdog remain armed without a
bound driver to ping it, resulting in an unintended reboot?
[ ... ]
> @@ -509,10 +563,13 @@ static int usb_pcwd_release(struct inode *inode, struct file *file)
> static ssize_t usb_pcwd_temperature_read(struct file *file, char __user *data,
> size_t len, loff_t *ppos)
> {
> + struct usb_pcwd_private *usb_pcwd = file->private_data;
> int temperature;
> + int ret;
>
> - if (usb_pcwd_get_temperature(usb_pcwd_device, &temperature))
> - return -EFAULT;
> + ret = usb_pcwd_get_temperature(usb_pcwd, &temperature);
> + if (ret)
> + return ret;
>
> if (copy_to_user(data, &temperature, 1))
> return -EFAULT;
[Severity: High]
This isn't a bug introduced by this patch, but does copy_to_user() only copy
1 byte of the 4-byte int temperature variable to userspace?
On little-endian architectures, wouldn't this truncate any value over 255, and
on big-endian architectures, wouldn't it always copy the most significant byte
which is 0?
[ ... ]
> @@ -747,12 +835,20 @@ static int usb_pcwd_probe(struct usb_interface *interface,
> return 0;
>
> err_out_misc_deregister:
> + mutex_lock(&disconnect_mutex);
> + if (usb_pcwd_device == usb_pcwd)
> + usb_pcwd_device = NULL;
> + mutex_unlock(&disconnect_mutex);
> + mutex_lock(&usb_pcwd->mtx);
> + usb_pcwd->exists = 0;
> + mutex_unlock(&usb_pcwd->mtx);
> + usb_kill_urb(usb_pcwd->intr_urb);
> misc_deregister(&usb_pcwd_temperature_miscdev);
> err_out_unregister_reboot:
> unregister_reboot_notifier(&usb_pcwd_notifier);
> error:
> if (usb_pcwd)
> - usb_pcwd_delete(usb_pcwd);
> + kref_put(&usb_pcwd->kref, usb_pcwd_delete);
> usb_pcwd_device = NULL;
> return retval;
> }
[Severity: High]
This is a pre-existing issue, but does the error path fail to decrement the
global cards_found counter?
In usb_pcwd_probe(), cards_found is incremented early on:
cards_found++;
if (cards_found > 1) {
pr_err("This driver only supports 1 device\n");
return -ENODEV;
}
If the probe fails (e.g., due to out-of-memory or not being a HID device), it
jumps to the error path and returns without decrementing cards_found. Since
usb_pcwd_disconnect() won't be called for a failed probe, wouldn't this
permanently prevent any subsequent devices from being successfully probed
until the module is reloaded?
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.