linux-kernel - Re: [PATCH v5 3/3] w1: add UART w1 bus driver

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]

Date: Thu, 1 Feb 2024 10:35:32 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Christoph Winklhofer <cj.winklhofer@...il.com>,
 Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Rob Herring <robh@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Jiri Slaby <jirislaby@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Jonathan Corbet <corbet@....net>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH v5 3/3] w1: add UART w1 bus driver

On 01/02/2024 08:29, Christoph Winklhofer wrote:
>>> +
>>> +static void w1_uart_remove(struct serdev_device *serdev)
>>> +{
>>> +	struct w1_uart_device *w1dev = serdev_device_get_drvdata(serdev);
>>> +
>>> +	mutex_lock(&w1dev->mutex);
>>> +
>>> +	w1_remove_master_device(&w1dev->bus);
>>> +
>>> +	mutex_unlock(&w1dev->mutex);
>>
>> This is still suspicious. You do not have serdev_device_close and you
>> want to protect from concurrent access but it looks insufficient.
>>
>> This code assumes that:
>>
>> w1_uart_remove()
>>   <-- here concurrent read/write might start
>>   mutex_lock()
>>   w1_remove_master_device()
>>   mutex_unlock()
>>   <-- now w1_uart_serdev_tx_rx() or w1_uart_serdev_receive_buf() can be
>> executed, but device is removed. So what's the point of the mutex here?
>>
>> What exactly is protected by the mutex? So far it looks like only some
>> contents of w1dev, but it does not matter, because it that memory is
>> still valid at this point.
>>
>> After describing what is protected we can think whether it is really
>> protected...
>>
>>
>>>
>>
>> Best regards,
>> Krzysztof
>>
> 
> Yes, it is still suspicious, sorry..
> 
> After w1_uart_remove, serdev is closed and w1dev is released. Therefore
> the w1-callback (w1_uart_serdev_tx_rx) must be finished before returning

I did not even go to end of w1_uart_remove(). In my code above, that
thread is still in w1_uart_remove(), just after unlocking mutex.

> from w1_uart_remove. That was the intention with the lock and trylock.

Then it does not look really protected. To be honest, w1-gpio and other
drivers also might have a race here. You can test it by adding long
sleeps in read/write paths and then trying to unbind device. Maybe this
should be fixed everywhere, but this mutex here brings more questions.


Best regards,
Krzysztof