| Message ID | 20200917103427.15740-1-oneukum@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | usblp: fix race between disconnect() and read() | expand |
On Thu, Sep 17, 2020 at 12:34:27PM +0200, Oliver Neukum wrote: > read() needs to check whether the device has been > disconnected before it tries to talk to the device. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > Reported-by: syzbot+be5b5f86a162a6c281e6@syzkaller.appspotmail.com > --- > drivers/usb/class/usblp.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c > index 084c48c5848f..67cbd42421be 100644 > --- a/drivers/usb/class/usblp.c > +++ b/drivers/usb/class/usblp.c > @@ -827,6 +827,11 @@ static ssize_t usblp_read(struct file *file, char __user *buffer, size_t len, lo > if (rv < 0) > return rv; > > + if (!usblp->present) { > + count = -ENODEV; > + goto done; > + } > + What prevents ->present from not being changed right after this test? thanks, greg k-h
Am Donnerstag, den 17.09.2020, 13:43 +0200 schrieb Greg KH: > On Thu, Sep 17, 2020 at 12:34:27PM +0200, Oliver Neukum wrote: > > read() needs to check whether the device has been > > disconnected before it tries to talk to the device. > > > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > > Reported-by: syzbot+be5b5f86a162a6c281e6@syzkaller.appspotmail.com > > --- > > drivers/usb/class/usblp.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c > > index 084c48c5848f..67cbd42421be 100644 > > --- a/drivers/usb/class/usblp.c > > +++ b/drivers/usb/class/usblp.c > > @@ -827,6 +827,11 @@ static ssize_t usblp_read(struct file *file, char __user *buffer, size_t len, lo > > if (rv < 0) > > return rv; > > > > + if (!usblp->present) { > > + count = -ENODEV; > > + goto done; > > + } > > + > > What prevents ->present from not being changed right after this test? Hi, the mutex taken in rv = usblp_rwait_and_lock(usblp, !!(file->f_flags & O_NONBLOCK)); right above it. Yes, this driver is a mess. But short of adding a ton of comments or rewriting the whole thing I have no idea what to do about that. Regards Oliver
On Thu, Sep 17, 2020 at 02:03:11PM +0200, Oliver Neukum wrote: > Am Donnerstag, den 17.09.2020, 13:43 +0200 schrieb Greg KH: > > On Thu, Sep 17, 2020 at 12:34:27PM +0200, Oliver Neukum wrote: > > > read() needs to check whether the device has been > > > disconnected before it tries to talk to the device. > > > > > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > > > Reported-by: syzbot+be5b5f86a162a6c281e6@syzkaller.appspotmail.com > > > --- > > > drivers/usb/class/usblp.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c > > > index 084c48c5848f..67cbd42421be 100644 > > > --- a/drivers/usb/class/usblp.c > > > +++ b/drivers/usb/class/usblp.c > > > @@ -827,6 +827,11 @@ static ssize_t usblp_read(struct file *file, char __user *buffer, size_t len, lo > > > if (rv < 0) > > > return rv; > > > > > > + if (!usblp->present) { > > > + count = -ENODEV; > > > + goto done; > > > + } > > > + > > > > What prevents ->present from not being changed right after this test? > > Hi, > > the mutex taken in > > rv = usblp_rwait_and_lock(usblp, !!(file->f_flags & O_NONBLOCK)); Ah, missed that, thanks. greg k-h
diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c index 084c48c5848f..67cbd42421be 100644 --- a/drivers/usb/class/usblp.c +++ b/drivers/usb/class/usblp.c @@ -827,6 +827,11 @@ static ssize_t usblp_read(struct file *file, char __user *buffer, size_t len, lo if (rv < 0) return rv; + if (!usblp->present) { + count = -ENODEV; + goto done; + } + if ((avail = usblp->rstatus) < 0) { printk(KERN_ERR "usblp%d: error %d reading from printer\n", usblp->minor, (int)avail);
read() needs to check whether the device has been disconnected before it tries to talk to the device. Signed-off-by: Oliver Neukum <oneukum@suse.com> Reported-by: syzbot+be5b5f86a162a6c281e6@syzkaller.appspotmail.com --- drivers/usb/class/usblp.c | 5 +++++ 1 file changed, 5 insertions(+)