linux-kernel - Re: [RESEND PATCH V3 1/3] USB: serial: f81232: clear overrun flag

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 1 Apr 2019 17:01:29 +0800
From:   "Ji-Ze Hong (Peter Hong)" <hpeter@...il.com>
To:     Oliver Neukum <oneukum@...e.com>, peter_hong@...tek.com.tw,
        johan@...nel.org, gregkh@...uxfoundation.org
Cc:     "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@...il.com>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RESEND PATCH V3 1/3] USB: serial: f81232: clear overrun flag

Oliver Neukum 於 2019/4/1 下午 04:34 寫道:
> On Mo, 2019-04-01 at 14:00 +0800,  Ji-Ze Hong (Peter Hong)  wrote:
> 
> Hi,
> I am afraid there is a race condiion in this code.
> 
> 
>> @@ -315,6 +318,7 @@ static void f81232_process_read_urb(struct urb *urb)
>>   
>>   			if (lsr & UART_LSR_OE) {
>>   				port->icount.overrun++;
>> +				schedule_work(&priv->lsr_work);
> 
> Unconditionally scheduled
> 
>>   				tty_insert_flip_char(&port->port, 0,
>>   						TTY_OVERRUN);
>>   			}
> 
> [..]
>> +static int f81232_suspend(struct usb_serial *serial, pm_message_t message)
>> +{
>> +	struct f81232_private *port_priv;
>> +
>> +	port_priv = usb_get_serial_port_data(serial->port[0]);
>> +	flush_work(&port_priv->lsr_work);
>> +
>> +	return 0;
>> +}
>> +
>>   static struct usb_serial_driver f81232_device = {
>>   	.driver = {
>>   		.owner =	THIS_MODULE,
>> @@ -655,6 +688,7 @@ static struct usb_serial_driver f81232_device = {
>>   	.read_int_callback =	f81232_read_int_callback,
>>   	.port_probe =		f81232_port_probe,
>>   	.port_remove =		f81232_port_remove,
>> +	.suspend =		f81232_suspend,
> 
> Please have a look at:
> int usb_serial_suspend(struct usb_interface *intf, pm_message_t message)
> {
>          struct usb_serial *serial = usb_get_intfdata(intf);
>          int i, r = 0;
> 
> 
>          serial->suspending = 1;
> 
> 
>          /*
>           * serial->type->suspend() MUST return 0 in system sleep context,
>           * otherwise, the resume callback has to recover device from
>           * previous suspend failure.
>           */
>          if (serial->type->suspend) {
>                  r = serial->type->suspend(serial, message);
>                  if (r < 0) {
>                          serial->suspending = 0;
>                          goto err_out;
>                  }
>          }
> 
> 
>          for (i = 0; i < serial->num_ports; ++i)
>                  usb_serial_port_poison_urbs(serial->port[i]);
> err_out:
>          return r;
> }
> EXPORT_SYMBOL(usb_serial_suspend);
> 
> As you can see, the suspend method is called first and then the URBs
> are poisoned. That means that after you have flushed the work, it may
> be submitted again. The fix would be to test the 'suspending' flag
> before you schedule work (and recheck the need to schedule it
> during resume)

Thanks for report the race condition issue. It's seems the same bug
in f81534.c. I'll try to fix it on f81232.c then fix f81534.c too.

-- 
With Best Regards,
Peter Hong