linux-kernel - Re: Possible bug in cypress

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:   Tue, 22 Aug 2017 16:18:43 +0200
From:   Oliver Neukum <oneukum@...e.com>
To:     Anton Volkov <avolkov@...ras.ru>, koyama@...stlight.net,
        dignome@...il.com, johan@...nel.org
Cc:     Alexey Khoroshilov <khoroshilov@...ras.ru>,
        gregkh@...uxfoundation.org, ldv-project@...uxtesting.org,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: Possible bug in cypress_m8.ko

Am Dienstag, den 22.08.2017, 15:11 +0300 schrieb Anton Volkov:
> Hello.
> 
> Judging by the code of cypress_m8.c some functions are considered to be 
> capable of working concurrently with other functions, e.g. cypress_open.
> There are, however, entities that are protected by the locks at one 
> place and not protected in another. Lines are given using the info from 
> Linux kernel v4.12. Example:
> 
> cypress_send
>    spin_lock_irqsave
>    priv->write_urb_in_use = 1;
>    spin_lock_irqrestore
>    (cypress_m8.c: lines 761-763)
>    ...
>    if (result) {
>       priv->write_urb_in_use = 0; //without lock protection
>       (cypress_m8.c: line 783)
>    }
> 
> Is it a bug?

Yes, but not of the kind you describe.
The transition from "not in use" to "in use" must be guarded by
a lock, because it may be contended.
But if that transition is properly guarded, you already know
that there can be only user. He can theoretically give up
the resource without locking.

Yet there is a bug:

^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  702)       spin_lock_irqsave(&priv->lock, flags);
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  703)       if (priv->write_urb_in_use) {
441b62c1edb98 (Harvey Harrison    2008-03-03 16:08:34 -0800  704)               dbg("%s - can't write, urb in use", __func__);
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  705)               spin_unlock_irqrestore(&priv->lock, flags);
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  706)               return;
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  707)       }
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  708)       spin_unlock_irqrestore(&priv->lock, flags);

The flag is checked is checked under a lock. But then the lock is dropped.
And here:

^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  759)       spin_lock_irqsave(&priv->lock, flags);
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  760)       priv->write_urb_in_use = 1;
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  761)       spin_unlock_irqrestore(&priv->lock, flags);

The flag is set under lock, but unconditionally.
The code just makes no sense.


In addition, when you drop the flag without a lock you need to worry
about memory ordering.

	HTH
		Oliver