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: <10fa514a-7fa0-449f-a7fd-cd3bfb0180d7@linaro.org>
Date: Mon, 15 Jan 2024 10:36:39 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: 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,
 penguin-kernel@...ove.sakura.ne.jp, 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 14/01/2024 09:20, Edward Adam Davis wrote:
> nci_start_poll() holds the dev->mutex required by the kworker of nci_close_device(),
> and the related tasks are as follows:
> |cpu0                          |cpu1                                           |cpu2                      |
> |nci_close_device()            |                                               |                          |
> |mutex_lock(&ndev->req_lock);  |                                               |                          |
> |...                           |nfc_genl_start_poll()                          |                          |
> |flush_workqueue(ndev->rx_wq)  |mutex_lock(&dev->genl_data.genl_data_mutex);   |                          |
> |                              |nfc_start_poll()                               |                          |
> |              	               |device_lock(&dev->dev);                        |process_one_work()        |
> |                              |nci_start_poll()                               |nfc_targets_found()       |
> |                              |nci_request()                                  |device_lock(&dev->dev);   |
> |                              |mutex_lock(&ndev->req_lock);                   |                          |
> 
> Therefore, before applying for req_lock in nci_request(), it should be determined
> whether the execution of nci_close_device() has already begun.
> 
> Reported-and-tested-by: syzbot+2b131f51bb4af224ab40@...kaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@...com>
> ---
>  net/nfc/nci/core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> 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?

Your code looks really unnecessary, at least with that code flow from
commit msg. Especially considering you do it outside of mutex, so how
does it solve anything?

Best regards,
Krzysztof