Re: Re: [PATCH] USB: serial: console: Fix potential use-after-free in usb_console_setup()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 16, 2022 at 11:20:23PM +0800, Liang He wrote:
> 
> 
> At 2022-09-16 23:04:02, "Alan Stern" <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> >On Fri, Sep 16, 2022 at 03:35:52PM +0800, Liang He wrote:
> >> In usb_console_setup(), if we goto error_get_interface and the
> >> usb_serial_put() may finally call kfree(serial). However, the next
> >> line will call 'mutex_unlock(&serial->disc_mutex)' which can cause
> >> a potential UAF bug.
> >
> >Why not just move the mutex_unlock() call up one line, before the 
> >usb_serial_put()?
> >
> >> Fixes: 7bd032dc2793 ("USB serial: update the console driver")
> >> Signed-off-by: Liang He <windhl@xxxxxxx>
> >> ---
> >> 
> >>  I don't know if the refcount can be zero here, so if it cannot be zero,
> >> this code is safe and please ignore my patch.
> >> 
> >>  drivers/usb/serial/console.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
> >> index b97aa40ca4d1..21ac2dd6baca 100644
> >> --- a/drivers/usb/serial/console.c
> >> +++ b/drivers/usb/serial/console.c
> >> @@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options)
> >>  	int cflag = CREAD | HUPCL | CLOCAL;
> >>  	char *s;
> >>  	struct usb_serial *serial;
> >> +	struct mutex *s_mutex;
> >>  	struct usb_serial_port *port;
> >>  	int retval;
> >>  	struct tty_struct *tty = NULL;
> >> @@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options)
> >>  		return -ENODEV;
> >>  	}
> >>  	serial = port->serial;
> >> -
> >> +	s_mutex = &serial->disc_mutex;
> >>  	retval = usb_autopm_get_interface(serial->interface);
> >>  	if (retval)
> >>  		goto error_get_interface;
> >> @@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options)
> >>  	usb_autopm_put_interface(serial->interface);
> >>   error_get_interface:
> >>  	usb_serial_put(serial);
> >> -	mutex_unlock(&serial->disc_mutex);
> >> +	mutex_unlock(s_mutex);
> >
> >If the old code was unsafe then so is this, because s_mutex points to a 
> >mutex that is embedded within the serial structure.  If the structure 
> >was deallocated by usb_serial_put() then so was the mutex.
> >
> >Alan Stern
> >
> >>  	return retval;
> >>  }
> >>  
> >> -- 
> >> 2.25.1
> >> 
> 
> Hi, Alan Stern,
> 
> Thanks for your review and this patch is indeed wrong!
> 
> But I am not sure if we can safely move the usb_serial_put()
> out of mutex_unlock().
> 
> If it is safe, I can give a new version of patch very soon.
> 
> Can you help me confirm if it is safe?

I cannot.  You need to ask Johan (the USB-serial maintainer).

Alan Stern



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux