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 for Android: free password hash cracker in your pocket

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

Message-ID: <1554107689.6310.17.camel@suse.com>
Date:   Mon, 01 Apr 2019 10:34:49 +0200
From:   Oliver Neukum <oneukum@...e.com>
To:     "Ji-Ze Hong (Peter Hong)" <hpeter@...il.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

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)

	Regards
		Oliver