[PATCH] usblp: poison URBs upon disconnect

[PATCH] usblp: poison URBs upon disconnect

[Oliver Neukum]

syzkaller reported an URB that should have been killed to be active.
We do not understand it, but this should fix the issue if it is real.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Reported-by: syzbot+be5b5f86a162a6c281e6@syzkaller.appspotmail.com
---
 drivers/usb/class/usblp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index 0d8e3f3804a3..084c48c5848f 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -468,7 +468,8 @@ static int usblp_release(struct inode *inode, struct file *file)
 	usb_autopm_put_interface(usblp->intf);
 
 	if (!usblp->present)		/* finish cleanup from disconnect */
-		usblp_cleanup(usblp);
+		usblp_cleanup(usblp);	/* any URBs must be dead */
+
 	mutex_unlock(&usblp_mutex);
 	return 0;
 }
@@ -1375,9 +1376,11 @@ static void usblp_disconnect(struct usb_interface *intf)
 
 	usblp_unlink_urbs(usblp);
 	mutex_unlock(&usblp->mut);
+	usb_poison_anchored_urbs(&usblp->urbs);
 
 	if (!usblp->used)
 		usblp_cleanup(usblp);
+
 	mutex_unlock(&usblp_mutex);
 }
 
-- 
2.16.4

Re: [PATCH] usblp: poison URBs upon disconnect

[Pete Zaitcev]

On Thu,  7 May 2020 10:58:06 +0200
Oliver Neukum <oneukum@suse.com> wrote:

> syzkaller reported an URB that should have been killed to be active.
> We do not understand it, but this should fix the issue if it is real.

> @@ -1375,9 +1376,11 @@ static void usblp_disconnect(struct usb_interface *intf)
>  
>  	usblp_unlink_urbs(usblp);
>  	mutex_unlock(&usblp->mut);
> +	usb_poison_anchored_urbs(&usblp->urbs);
>  
>  	if (!usblp->used)
>  		usblp_cleanup(usblp);
>  	mutex_unlock(&usblp_mutex);
>  }

Sorry, it took me this long to think about it. I am against the duplication
of effort between usblp_unlink_urbs, which is exactly usb_kill_anchored_urbs,
and usb_poison_anchored_urbs. If you think that poisoning may help against
what the bot identified, we may try this instead:

diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index 0d8e3f3804a3..eae5eaf5d4f1 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -1373,7 +1373,7 @@ static void usblp_disconnect(struct usb_interface *intf)
        wake_up(&usblp->rwait);
        usb_set_intfdata(intf, NULL);
 
-       usblp_unlink_urbs(usblp);
+       usb_poison_anchored_urbs(&usblp->urbs);
        mutex_unlock(&usblp->mut);
 
        if (!usblp->used)

I'm still concerned that we didn't identify the scenario tht led to bot's
findings.

The usblp->present was supposed to play a role of the poison pill, at the
driver level. The difference with poisoning the anchor is that ->present
is protected by the most outlying mutex, and therefore cannot be meaninfully
checked in URB callbacks. But the anchor's poison flag is protected by a
spinlock, so callbacks check it. But what does it matter for us? This driver
does not re-submit URBs from callbacks.

So, I'm suspicious of attempts to hit at the problem in the dark and hope
for a miracle.

-- Pete

Re: [PATCH] usblp: poison URBs upon disconnect

[Oliver Neukum]

Am Freitag, den 15.05.2020, 15:31 -0500 schrieb Pete Zaitcev:
> 
> and usb_poison_anchored_urbs. If you think that poisoning may help against
> what the bot identified, we may try this instead:

Sure. Would you send in a patch?

> I'm still concerned that we didn't identify the scenario tht led to bot's
> findings.

So am I. Yet as far as I can tell the code of usblp is correct. Even
worse, it is casting doubt on our testing framework.

> The usblp->present was supposed to play a role of the poison pill, at the
> driver level. The difference with poisoning the anchor is that ->present
> is protected by the most outlying mutex, and therefore cannot be meaninfully
> checked in URB callbacks. But the anchor's poison flag is protected by a
> spinlock, so callbacks check it. But what does it matter for us? This driver
> does not re-submit URBs from callbacks.

It also makes resubmission impossible. In fact, considering that, we
might better poison before we go for usblp->present.

> So, I'm suspicious of attempts to hit at the problem in the dark and hope
> for a miracle.

Right. The only thing worse would be doing nothing. At the risk of
repeating myself, usblp looks correct to me.

	Regards
		Oliver