linux-kernel - Re: [PATCH] usb: use mutex_lock in iowarrior

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <bf971924-9d91-40a3-a4c2-5b518e2ce2fd@suse.com>
Date: Mon, 16 Sep 2024 15:15:57 +0200
From: Oliver Neukum <oneukum@...e.com>
To: Jeongjun Park <aha310510@...il.com>, Oliver Neukum <oneukum@...e.com>
Cc: Greg KH <gregkh@...uxfoundation.org>, colin.i.king@...il.com,
 linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: use mutex_lock in iowarrior_read()

Hi,

On 16.09.24 14:44, Jeongjun Park wrote:
> Oliver Neukum <oneukum@...e.com> wrote:
>>
>>
>>
>> On 16.09.24 06:15, Greg KH wrote:
>>> On Mon, Sep 16, 2024 at 01:06:29PM +0900, Jeongjun Park wrote:

>>> Please use the guard() form here, it makes the change much simpler and
>>> easier to review and maintain.
>>
>> That would break the O_NONBLOCK case.
>>
>> Looking at the code it indeed looks like iowarrior_read() can race
>> with itself. Strictly speaking it always could happen if a task used
>> fork() after open(). The driver tries to restrict its usage to one
>> thread, but I doubt that the logic is functional.
>>
>> It seems to me the correct fix is something like this:
> 
> Well, I don't know why it's necessary to modify it like this.
> I think it would be more appropriate to patch it to make it
> more maintainable by using guard() as Greg suggested.

Allow me to explain detail.

guard() internally uses mutex_lock(). That means that

a) it will block
b) having blocked it will sleep in the state TASK_UNINTERRUPTIBLE

The driver itself uses TASK_INTERRUPTIBLE in iowarrior_read(),
when it waits for IO. That is entirely correct, as it waits for
an external device doing an operation that may never occur. You
must use TASK_INTERRUPTIBLE.

Now, if you use mutex_lock() to wait for a task waiting for IO
to occur in the state TASK_INTERRUPTIBLE, you are indirectlywaiting for
an event that you must wait for in TASK_INTERRUPTIBLE in the state
TASK_UNINTERRUPTIBLE.
That is a bug. You have created a task that cannot be killed (uid may not match),
but may have to be killed. Furthermore you block even in case the
device has been opened with O_NONBLOCK, which is a second bug.

These limitations are inherent in guard(). Therefore you cannot use
guard here.

	Regards
		Oliver