Re: [PATCH] Input: yealink - stop URB resubmission on completion error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The URB completion callbacks return early on any non-zero URB status, preventing resubmission on transient errors (like -EPROTO or -EILSEQ) and permanently breaking the device's polling loop.

Pre-existing issues:
- [High] Concurrent accesses to `yld->ctl_data` from sysfs and the asynchronous URB polling loop cause DMA buffer corruption, and passing a coherent DMA buffer to a synchronous USB API violates DMA mappings.
- [High] Ping-pong URBs can escape `usb_kill_urb()` during `input_close()`, leading to continuous background execution and use-after-free.
--

commit 02a6ac0b5f7fd710af199370b924a27f34ca574c
Author: Jie Wang <jie.wang@intel.com>

Input: yealink - stop URB resubmission on completion error

urb_irq_callback() and urb_ctl_callback() resubmit each other's URBs
regardless of completion status. When the device returns a persistent
error (-EPROTO), this creates an unthrottled resubmission loop that
starves the CPU and triggers RCU stalls.

Stop resubmitting on any non-zero URB status, following the standard
completion pattern used by other USB input drivers.

Reported-by: syzbot+039eab266c6321a174bd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=039eab266c6321a174bd
Signed-off-by: Jie Wang <jie.wang@intel.com>

> diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
> index 560f895a00cda..fcf66912f86b6 100644
> --- a/drivers/input/misc/yealink.c
> +++ b/drivers/input/misc/yealink.c
> @@ -405,9 +405,20 @@ static void urb_irq_callback(struct urb *urb)
>  	struct yealink_dev *yld = urb->context;
>  	int ret, status = urb->status;
>  
> -	if (status)
> +	switch (status) {
> +	case 0:
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +		dev_dbg(&yld->intf->dev, "%s - urb shutting down with status %d\n",
> +			__func__, status);
> +		return;
> +	default:
>  		dev_err(&yld->intf->dev, "%s - urb status %d\n",
>  			__func__, status);
> +		return;
> +	}

[Severity: High]
Does returning early on any non-zero status prevent resubmission on transient
errors?

If an error like -EPROTO or -EILSEQ is encountered due to electrical noise,
this early return completely drops the URB. Because the driver has no backoff
timer or error retry mechanism, a single transient error will permanently stop
the polling state machine. Sysfs updates and key presses will silently be
ignored until the device is physically replugged.

[ ... ]

> @@ -443,9 +454,20 @@ static void urb_ctl_callback(struct urb *urb)
>  	struct yealink_dev *yld = urb->context;
>  	int ret = 0, status = urb->status;
>  
> -	if (status)
> +	switch (status) {
> +	case 0:
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +		dev_dbg(&yld->intf->dev, "%s - urb shutting down with status %d\n",
> +			__func__, status);
> +		return;
> +	default:
>  		dev_err(&yld->intf->dev, "%s - urb status %d\n",
>  			__func__, status);
> +		return;
> +	}

[Severity: High]
Will the same issue happen here, breaking the ping-pong URB loop on transient
USB errors and permanently freezing the device state machine?

[Severity: High]
This is a pre-existing issue, but does concurrent access to the shared control
buffer from sysfs cause DMA corruption?

Looking at yealink_set_ringtone(), it modifies the shared control buffer
without locking:

drivers/input/misc/yealink.c:yealink_set_ringtone() {
    ...
    memset(yld->ctl_data, 0, sizeof(*(yld->ctl_data)));
    yld->ctl_data->cmd = CMD_RING_VOLUME;
    ...
}

Since yld->ctl_data is used as the dedicated DMA transfer buffer for the
asynchronous control URB which runs continuously in a background loop, modifying
it from sysfs without locking against the async loop could corrupt an active DMA
transfer.

Additionally, yealink_set_ringtone() synchronously sends this buffer:

drivers/input/misc/yealink.c:yealink_cmd() {
    ...
    return usb_control_msg(yld->udev,
            usb_sndctrlpipe(yld->udev, 0),
            USB_REQ_SET_CONFIGURATION,
            USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_OUT,
            0x200, 3,
            p, sizeof(*p),
            USB_CTRL_SET_TIMEOUT);
}

Because yld->ctl_data is allocated with usb_alloc_coherent(), does this violate
the DMA API by attempting to dma_map_single() already-coherent memory?

[Severity: High]
This is a pre-existing issue, but can ping-pong URBs escape the shutdown
sequence and cause a use-after-free?

The driver attempts to stop the ping-pong URBs in input_close() by setting a
flag and killing them:

drivers/input/misc/yealink.c:input_close() {
    ...
    yld->shutdown = 1;
    usb_kill_urb(yld->urb_ctl);
    usb_kill_urb(yld->urb_irq);
    ...
}

If urb_irq_callback() evaluates !yld->shutdown as true but is preempted:

drivers/input/misc/yealink.c:urb_irq_callback() {
    ...
    if (!yld->shutdown) {
        ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
    ...
}

And input_close() then sets shutdown = 1, kills urb_ctl (which isn't in flight
yet), and blocks killing urb_irq. When urb_irq_callback() resumes, it submits
urb_ctl. Because usb_kill_urb(urb_ctl) already returned and cleared the reject
count, the submission succeeds.

If the device is disconnected, usb_cleanup() frees the yealink_dev structure,
but the runaway URB will dereference it upon completion. Should this use
usb_poison_urb() instead to reliably stop the loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616110440.1073059-1-jie.wang@intel.com?part=1