netdev - Re: [PATCH] nfc/nci: fix task hung in nfc_targets

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: <ddb92cce-911b-48d9-a780-7b319f7491eb@linaro.org>
Date: Mon, 15 Jan 2024 12:15:52 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
 Edward Adam Davis <eadavis@...com>,
 syzbot+2b131f51bb4af224ab40@...kaller.appspotmail.com
Cc: davem@...emloft.net, edumazet@...gle.com, gregkh@...uxfoundation.org,
 hdanton@...a.com, kuba@...nel.org, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, pabeni@...hat.com, stern@...land.harvard.edu,
 syzkaller-bugs@...glegroups.com, torvalds@...ux-foundation.org
Subject: Re: [PATCH] nfc/nci: fix task hung in nfc_targets_found

On 15/01/2024 12:08, Tetsuo Handa wrote:
> On 2024/01/15 18:36, Krzysztof Kozlowski wrote:
>>> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
>>> index 6c9592d05120..9a277228a875 100644
>>> --- a/net/nfc/nci/core.c
>>> +++ b/net/nfc/nci/core.c
>>> @@ -145,6 +145,8 @@ inline int nci_request(struct nci_dev *ndev,
>>>  {
>>>  	int rc;
>>>  
>>> +	if (test_bit(NCI_UNREG, &ndev->flags))
>>> +		return -ENODEV;
>>
>> nci_close_device() clears the NCI_UP, which is tested here, just after
>> acquiring mutex. And there is explicit comment about it just below your
>> code. Why it is not relevant?
> 
> Because the deadlock happens at mutex_lock(&ndev->req_lock), which is
> before test_bit(NCI_UP, &ndev->flags) is called. Please see
> https://lkml.kernel.org/r/d314e471-0251-461e-988d-70add0c6ebf6@I-love.SAKURA.ne.jp .

I see, yet still this code looks like moving or copying existing
test_bit(NCI_UP) outside of mutex, which is usually not the correct
solution for deadlocks. First of all, flags are supposed to be
manipulated under lock, so the code here can be quite re-ordered. What
stops the CPU to test negative in above (so: !NCI_UNREG) and then
execute nci_close_device before waiting on mutex here? Nothing.

The problem seems to be locking and solution is not to add one if()
outside of locking.

Best regards,
Krzysztof