2019-10-08 22:27:03

by Valentin Vidić

Subject: [PATCH] usb: iowarrior: fix access to freed data structure

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: [email protected]
Signed-off-by: Valentin Vidic <[email protected]>
---
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);
- }
+ 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)
--
2.20.1

2019-10-09 07:39:22

by Johan Hovold

Subject: Re: [PATCH] usb: iowarrior: fix access to freed data structure

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: [email protected]
> Signed-off-by: Valentin Vidic <[email protected]>
> ---
> 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