| Message ID | 20191008222307.18587-1-vvidic@valentin-vidic.from.hr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: iowarrior: fix access to freed data structure | expand |
On Wed, Oct 09, 2019 at 12:23:07AM +0200, Valentin Vidic wrote: > struct iowarrior gets freed prematurely in iowarrior_release while > it is still being referenced from usb_interface, so let only > iowarrior_disconnect call iowarrior_delete. The proposed fix is broken since release() may happen long after disconnect(), in which case you'd now leak memory. > Fixes: KMSAN: uninit-value in iowarrior_disconnect Fixes tags are supposed to refer to the commit introducing the issue. > Reported-by: syzbot+0761012cebf7bdb38137@syzkaller.appspotmail.com > Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr> > --- > drivers/usb/misc/iowarrior.c | 35 +++++++++++++++-------------------- > 1 file changed, 15 insertions(+), 20 deletions(-) > > diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c > index f5bed9f29e56..0492ea76c4bf 100644 > --- a/drivers/usb/misc/iowarrior.c > +++ b/drivers/usb/misc/iowarrior.c > @@ -638,7 +638,6 @@ static int iowarrior_open(struct inode *inode, struct file *file) > static int iowarrior_release(struct inode *inode, struct file *file) > { > struct iowarrior *dev; > - int retval = 0; > > dev = file->private_data; > if (!dev) > @@ -650,27 +649,23 @@ static int iowarrior_release(struct inode *inode, struct file *file) > mutex_lock(&dev->mutex); > > if (dev->opened <= 0) { > - retval = -ENODEV; /* close called more than once */ > mutex_unlock(&dev->mutex); > - } else { > - dev->opened = 0; /* we're closing now */ > - retval = 0; > - if (dev->present) { > - /* > - The device is still connected so we only shutdown > - pending read-/write-ops. > - */ > - usb_kill_urb(dev->int_in_urb); > - wake_up_interruptible(&dev->read_wait); > - wake_up_interruptible(&dev->write_wait); > - mutex_unlock(&dev->mutex); > - } else { > - /* The device was unplugged, cleanup resources */ > - mutex_unlock(&dev->mutex); > - iowarrior_delete(dev); So you cannot just remove this bit. > - } > + return -ENODEV; /* close called more than once */ > } > - return retval; > + > + dev->opened = 0; /* we're closing now */ > + if (dev->present) { > + /* > + * The device is still connected so we only shutdown > + * pending read/write ops. > + */ > + usb_kill_urb(dev->int_in_urb); > + wake_up_interruptible(&dev->read_wait); > + wake_up_interruptible(&dev->write_wait); > + } > + > + mutex_unlock(&dev->mutex); > + return 0; > } > > static __poll_t iowarrior_poll(struct file *file, poll_table * wait) Johan
diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c index f5bed9f29e56..0492ea76c4bf 100644 --- a/drivers/usb/misc/iowarrior.c +++ b/drivers/usb/misc/iowarrior.c @@ -638,7 +638,6 @@ static int iowarrior_open(struct inode *inode, struct file *file) static int iowarrior_release(struct inode *inode, struct file *file) { struct iowarrior *dev; - int retval = 0; dev = file->private_data; if (!dev) @@ -650,27 +649,23 @@ static int iowarrior_release(struct inode *inode, struct file *file) mutex_lock(&dev->mutex); if (dev->opened <= 0) { - retval = -ENODEV; /* close called more than once */ mutex_unlock(&dev->mutex); - } else { - dev->opened = 0; /* we're closing now */ - retval = 0; - if (dev->present) { - /* - The device is still connected so we only shutdown - pending read-/write-ops. - */ - usb_kill_urb(dev->int_in_urb); - wake_up_interruptible(&dev->read_wait); - wake_up_interruptible(&dev->write_wait); - mutex_unlock(&dev->mutex); - } else { - /* The device was unplugged, cleanup resources */ - mutex_unlock(&dev->mutex); - iowarrior_delete(dev); - } + return -ENODEV; /* close called more than once */ } - return retval; + + dev->opened = 0; /* we're closing now */ + if (dev->present) { + /* + * The device is still connected so we only shutdown + * pending read/write ops. + */ + usb_kill_urb(dev->int_in_urb); + wake_up_interruptible(&dev->read_wait); + wake_up_interruptible(&dev->write_wait); + } + + mutex_unlock(&dev->mutex); + return 0; } static __poll_t iowarrior_poll(struct file *file, poll_table * wait)
struct iowarrior gets freed prematurely in iowarrior_release while it is still being referenced from usb_interface, so let only iowarrior_disconnect call iowarrior_delete. Fixes: KMSAN: uninit-value in iowarrior_disconnect Reported-by: syzbot+0761012cebf7bdb38137@syzkaller.appspotmail.com Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr> --- drivers/usb/misc/iowarrior.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-)