| 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
| ||
|
Message-ID: <7210239c-e15b-4c35-b234-c3bc939d4ab6@huaweicloud.com>
Date: Wed, 6 Aug 2025 00:54:42 +0800
From: Wang Zhaolong <wangzhaolong@...weicloud.com>
To: Enzo Matsumiya <ematsumiya@...e.de>
Cc: sfrench@...ba.org, pshilov@...rosoft.com, linux-cifs@...r.kernel.org,
samba-technical@...ts.samba.org, linux-kernel@...r.kernel.org,
chengzhihao1@...wei.com, yi.zhang@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
>
> Do you have a reproducer for this? mids are allocated from kmem cache,
> and a leak should certainly be visible (WARN on rmmod), even without any
> debugging facilities enabled.
Thanks for your reply and review!
Yes, I have the reproducer. I have listed it in the commit message.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=220404
>
> However, I do know that the following problem is quite common in cifs:
>
> thread 0 | thread 1
> ----------------|----------------
> | lock
> | check data
> | data is valid
> | unlock
> lock |
> invalidate data | lock (spins)
> unlock | ...
> | // assumes data still valid
> | use invalid data (not really freed)
> | unlock
>
> You see that no matter how many locks you add to protect data, there's
> still a chance of having this "race condition" feeling.
>
> So, personally, I'm skeptical about having yet another spinlock with
> questionable or no effect at all.
>
> But again, if I can reproduce this bug myself, it'll be much easier to
> analyse effectiveness/review your patches.
>
> Apart from that, cleanup patches always get my +1 :)
>
The data is interrelated, and updates to the data are protected by per-mid locks
to avoid `race condition`.
For example, in the following scenario, the update and check of
mid->mid_state are protected by the lock spin_lock(&midQ[i]->mid_lock),
which can prevent leakage issues.
```
User foreground process cifsd
cifs_readdir
open_cached_dir
cifs_send_recv
compound_send_recv
smb2_setup_request
smb2_mid_entry_alloc
smb2_get_mid_entry
smb2_mid_entry_alloc
mempool_alloc // alloc mid
kref_init(&temp->refcount); // refcount = 1
mid[0]->callback = cifs_compound_callback;
mid[1]->callback = cifs_compound_last_callback;
smb_send_rqst
rc = wait_for_response
wait_event_state TASK_KILLABLE
cifs_demultiplex_thread
allocate_buffers
server->bigbuf = cifs_buf_get()
standard_receive3
->find_mid()
smb2_find_mid
__smb2_find_mid
kref_get(&mid->refcount) // +1
cifs_handle_standard
handle_mid
/* bigbuf will also leak */
mid->resp_buf = server->bigbuf
server->bigbuf = NULL;
dequeue_mid
/* in for loop */
/* Signal interrupts wait: rc = -ERESTARTSYS */
spin_lock(&midQ[i]->mid_lock);
mids[0]->callback
cifs_compound_callback
/* The change comes too late */
mid->mid_state = MID_RESPONSE_READY
spin_lock(&midQ[i]->mid_lock);
spin_lock(&midQ[i]->mid_lock);
/* if (... || midQ[i]->mid_state == MID_RESPONSE_RECEIVED) Not satisfied *?
spin_unlock(&midQ[i]->mid_lock);
release_mid // -1
release_mid // -1
```
Or, in the case where the callback is replaced
User foreground process cifsd
cifs_readdir
open_cached_dir
cifs_send_recv
compound_send_recv
smb2_setup_request
smb2_mid_entry_alloc
smb2_get_mid_entry
smb2_mid_entry_alloc
mempool_alloc // alloc mid
kref_init(&temp->refcount); // refcount = 1
mid[0]->callback = cifs_compound_callback;
mid[1]->callback = cifs_compound_last_callback;
smb_send_rqst
rc = wait_for_response
wait_event_state TASK_KILLABLE
cifs_demultiplex_thread
allocate_buffers
server->bigbuf = cifs_buf_get()
standard_receive3
->find_mid()
smb2_find_mid
__smb2_find_mid
kref_get(&mid->refcount) // +1
cifs_handle_standard
handle_mid
/* bigbuf will also leak */
mid->resp_buf = server->bigbuf
server->bigbuf = NULL;
dequeue_mid
/* in for loop */
/* Signal interrupts wait: rc = -ERESTARTSYS */
spin_lock(&midQ[i]->mid_lock);
/* if (... || midQ[i]->mid_state == MID_RESPONSE_RECEIVED) Satisfied *?
midQ[0]->callback = cifs_cancelled_callback;
cancelled_mid[i] = true;
spin_unlock(&midQ[i]->mid_lock);
spin_lock(&midQ[i]->mid_lock);
mids[0]->callback
cifs_cancelled_callback
cifs_compound_callback
/* The change comes too late */
mid->mid_state = MID_RESPONSE_READY
release_mid // -1
spin_lock(&midQ[i]->mid_lock);
release_mid // -1
I know this approach is quite ugly, but it is a relatively reliable
method that preserves the historical bugfix logic.
Relevant patches:
d527f51331ca ("cifs: Fix UAF in cifs_demultiplex_thread()")
ee258d79159a ("CIFS: Move credit processing to mid callbacks for SMB3")
8544f4aa9dd1 ("CIFS: Fix credit computation for compounded requests")
d7d7a66aacd6 ("cifs: avoid use of global locks for high contention data")
Thanks again for your reply. I’m looking forward to any further
suggestions you may have to improve the patch.
Best regards,
Wang Zhaolong
Powered by blists - more mailing lists