linux-kernel - Re: [PATCH] nfc: pn533: Fix null-ptr-deref in pn533_recv

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]

Message-ID: <8a01c6b2-ca30-448c-8ba8-34720891b461@kernel.org>
Date: Wed, 24 Apr 2024 20:13:31 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Yuxuan-Hu <yuxuanhu@...a.edu.cn>, Yuxuan Hu <20373622@...a.edu.cn>,
 gregkh@...uxfoundation.org, johan@...nel.org, davem@...emloft.net,
 u.kleine-koenig@...gutronix.de, Jonathan.Cameron@...wei.com,
 francesco.dolcini@...adex.com, jirislaby@...nel.org
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 baijiaju1990@...il.com, sy2239101@...a.edu.cn
Subject: Re: [PATCH] nfc: pn533: Fix null-ptr-deref in pn533_recv_frame()

On 24/04/2024 16:00, Yuxuan-Hu wrote:
> Hi Krzysztof,
> Thanks for your reply.
> 
> On 2024/4/24 13:36, Krzysztof Kozlowski wrote:
>> A bit better solution would be to NULL-ify dev->cmd at the beginning of
>> pn533_send_async_complete(), because that seems logical. The complete
>> callback takes ownership of dev->cmd, so why it performs the assignment
>> at the end?
>>
>> However even above code will keep the race open for short period.
>> Probably some locking would solve it or checking for dev->cmd in few
>> places with barriers.
>>
>> Best regards,
>> Krzysztof
> 
> I think adding a lock seems to be a better solution, however, acquire a 
> lock on each access to dev->cmd does not seem to be an appropriate 
> implementation.
> I wonder whether you think it is appropriate to acquire a lock at the 
> beginning of pn533_recv_frame(), and release it when 
> pn533_wq_cmd_complete() is finished, thus ensuring that another work 
> will not start when one pn533_wq_cmd_complete() work is not yet finished.

I don't know this driver well enough, but it currently does not have any
locking around this so it assumes pn533_recv_frame() cannot start
concurrently, before complete callback finishes.

This could be solved, if it makes sense.

However maybe it would be enough to take:
1. Move contents of dev->cmd to another member in dev (e.g.
dev->cmd_in_progress), first checking if that member is empty (then
maybe wait? or return -EBUSY?)
2. dev->cmd = NULL
3. queue_work (see also explanation of barriers there)
4. The cmd_complete_work workqueue takes ownership of
dev->cmd_in_progress and NULL-ifies it at end of work + barrier.

Best regards,
Krzysztof