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

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

[sashiko-bot]

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

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

[Jie Wang]

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>
---
 drivers/input/misc/yealink.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
index 8786ed8b3565..bbfa1a9c23d1 100644
--- a/drivers/input/misc/yealink.c
+++ b/drivers/input/misc/yealink.c
@@ -414,9 +414,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;
+	}

 	switch (yld->irq_data->cmd) {
 	case CMD_KEYPRESS:
@@ -452,9 +463,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;
+	}

 	switch (yld->ctl_data->cmd) {
 	case CMD_KEYPRESS:
--
2.34.1

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

[Dmitry Torokhov]

Hi Jie,

On Tue, Jun 16, 2026 at 11:04:40AM +0000, Jie Wang wrote:
> 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>

It would be good to have a helper like

bool usb_is_permanent_error(struct urb *urb, struct device *dev);

instead of having this "switch" in all the drivers. It also unclear
whether we should not attempt to re-submit the URB on errors other than
ECONNRESET, ENOENT, or ESHUTDOWN.

> ---
>  drivers/input/misc/yealink.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
> index 8786ed8b3565..bbfa1a9c23d1 100644
> --- a/drivers/input/misc/yealink.c
> +++ b/drivers/input/misc/yealink.c
> @@ -414,9 +414,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;
> +	}
> 
>  	switch (yld->irq_data->cmd) {
>  	case CMD_KEYPRESS:
> @@ -452,9 +463,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;
> +	}
> 
>  	switch (yld->ctl_data->cmd) {
>  	case CMD_KEYPRESS:
> --
> 2.34.1

Thanks.

-- 
Dmitry