* v5.1-rc1 cifs bug: underflow; use-after-free.
@ 2019-03-19 11:51 Dominik Brodowski
2019-03-19 15:26 ` Aurélien Aptel
0 siblings, 1 reply; 12+ messages in thread
From: Dominik Brodowski @ 2019-03-19 11:51 UTC (permalink / raw)
To: sfrench; +Cc: linux-cifs
Steve,
when mounting a cifs (vers=2.0, unfortunately...) volume on v5.1-rc1, I get
the following warning (slightly edited to avoid information leaks):
[ 118.326535] CIFS: Attempting to mount //some/what
[ 118.667347] ------------[ cut here ]------------
[ 118.667367] refcount_t: underflow; use-after-free.
[ 118.667384] WARNING: CPU: 1 PID: 1966 at lib/refcount.c:190 refcount_sub_and_test_checked+0x5c/0x70
[ 118.667387] Modules linked in:
[ 118.667392] CPU: 1 PID: 1966 Comm: mount.cifs Tainted: G T 5.1.0-rc1 #1
[ 118.667395] Hardware name: Dell Inc. XPS 13 9343/0TM99H, BIOS A11 12/08/2016
[ 118.667400] RIP: 0010:refcount_sub_and_test_checked+0x5c/0x70
[ 118.667432] Call Trace:
[ 118.667439] close_shroot+0x21/0xa0
[ 118.667444] smb2_query_path_info+0x16b/0x1f0
[ 118.667454] cifs_get_inode_info+0x2b3/0x860
[ 118.667467] cifs_root_iget+0x12c/0x670
[ 118.667473] cifs_smb3_do_mount+0x4f7/0x680
[ 118.667479] ? rcu_read_lock_sched_held+0x74/0x80
[ 118.667483] ? kfree+0x248/0x290
[ 118.667490] legacy_get_tree+0x24/0x40
[ 118.667494] vfs_get_tree+0x3d/0x110
[ 118.667500] do_mount+0x30a/0xef0
[ 118.667504] ? rcu_read_lock_sched_held+0x74/0x80
[ 118.667512] ksys_mount+0xbd/0xe0
[ 118.667517] __x64_sys_mount+0x22/0x30
[ 118.667522] do_syscall_64+0x50/0x160
[ 118.667527] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 118.667531] RIP: 0033:0x754fd3d1d68e
Thanks,
Dominik
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v5.1-rc1 cifs bug: underflow; use-after-free.
2019-03-19 11:51 v5.1-rc1 cifs bug: underflow; use-after-free Dominik Brodowski
@ 2019-03-19 15:26 ` Aurélien Aptel
2019-03-19 15:47 ` Aurélien Aptel
2019-03-19 16:26 ` Dominik Brodowski
0 siblings, 2 replies; 12+ messages in thread
From: Aurélien Aptel @ 2019-03-19 15:26 UTC (permalink / raw)
To: Dominik Brodowski, sfrench; +Cc: linux-cifs
Hi,
Dominik Brodowski <linux@dominikbrodowski.net> writes:
> when mounting a cifs (vers=2.0, unfortunately...) volume on v5.1-rc1, I get
> the following warning (slightly edited to avoid information leaks):
The cached root can be closed 2 ways:
- from the cifs_get_inode_info()
- from a lease break while it is open
So here's my theory:
in the mount task:
=> mount()
...
=> cifs_get_inode_info()
=> open_shroot()
(at this point root has open handle with lease)
in the receive loop task:
<==== LEASE BREAK arrives (root modified from another smb
client)
queues & call cached root lease break callback
smb2_cached_lease_break()
=> close_shroot()
refcount reaches 0, we release the cached fid
back in the mount task:
=> we are done with the handle time to call
=> close_shroot()
refcount already 0, releasing again
----
Now, since the release function doesn't actually frees the cached_fid
struct but closes the handle sets an invalid flag instead I think this
message can be ignored, because the release function checks for the flag
anyway. i.e. second time we call smb2_close_cached_fid, it is a no-op.
See:
static void
smb2_close_cached_fid(struct kref *ref)
{
struct cached_fid *cfid = container_of(ref, struct cached_fid,
refcount);
if (cfid->is_valid) {
cifs_dbg(FYI, "clear cached root file handle\n");
SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
cfid->fid->volatile_fid);
cfid->is_valid = false;
cfid->file_all_info_is_valid = false;
}
}
void close_shroot(struct cached_fid *cfid)
{
mutex_lock(&cfid->fid_mutex);
kref_put(&cfid->refcount, smb2_close_cached_fid);
mutex_unlock(&cfid->fid_mutex);
}
If you enable verbose debugging [1], if my theory is correct you should
see a lease break messsage followed by "clear cached root file handle"
message before the warning.
Since we take a mutex before and after the kref, it kind of defeats the
purpose of the atomic kref i.e. we could use a regular integer as
refcount and simply do this:
void close_shroot(struct cached_fid *cfid)
{
mutex_lock(&cfid->fid_mutex);
if (cfid->refcount-- && cfid->is_valid) {
cifs_dbg(FYI, "clear cached root file handle\n");
SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
cfid->fid->volatile_fid);
cfid->is_valid = false;
cfid->file_all_info_is_valid = false;
}
mutex_unlock(&cfid->fid_mutex);
}
(we need to replace other usage of the kref and check they are all
protected by taking the mutex as well)
1: https://wiki.samba.org/index.php/Bug_Reporting#cifs.ko
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v5.1-rc1 cifs bug: underflow; use-after-free.
2019-03-19 15:26 ` Aurélien Aptel
@ 2019-03-19 15:47 ` Aurélien Aptel
2019-03-19 16:26 ` Dominik Brodowski
1 sibling, 0 replies; 12+ messages in thread
From: Aurélien Aptel @ 2019-03-19 15:47 UTC (permalink / raw)
To: Dominik Brodowski, sfrench; +Cc: linux-cifs
Aurélien Aptel <aaptel@suse.com> writes:
> if (cfid->refcount-- && cfid->is_valid) {
Actually, let's not decrement in the condition :)
void close_shroot(struct cached_fid *cfid)
{
mutex_lock(&cfid->fid_mutex);
if (cfid->refcount > 0 && cfid->is_valid) {
cifs_dbg(FYI, "clear cached root file handle\n");
SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
cfid->fid->volatile_fid);
cfid->is_valid = false;
cfid->file_all_info_is_valid = false;
cfid->refcount--;
}
mutex_unlock(&cfid->fid_mutex);
}
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v5.1-rc1 cifs bug: underflow; use-after-free.
2019-03-19 15:26 ` Aurélien Aptel
2019-03-19 15:47 ` Aurélien Aptel
@ 2019-03-19 16:26 ` Dominik Brodowski
2019-03-20 11:12 ` Aurélien Aptel
1 sibling, 1 reply; 12+ messages in thread
From: Dominik Brodowski @ 2019-03-19 16:26 UTC (permalink / raw)
To: Aurélien Aptel; +Cc: sfrench, linux-cifs
Hi Aurélien,
Thanks for taking a look at this issue. Fortunately, it is easily
reproducable (at least for me).
> If you enable verbose debugging [1], if my theory is correct you should
> see a lease break messsage followed by "clear cached root file handle"
> message before the warning.
Hm, no.
...
[ 2466.101770] fs/cifs/connect.c: Socket created
[ 2466.101813] fs/cifs/connect.c: sndbuf 16384 rcvbuf 131072 rcvtimeo 0x1b58
[ 2466.158066] fs/cifs/connect.c: Demultiplex PID: 3380
[ 2466.158074] fs/cifs/connect.c: CIFS VFS: in cifs_get_smb_ses as Xid: 1 with uid: 0
[ 2466.158302] fs/cifs/connect.c: Existing smb sess not found
[ 2466.158582] fs/cifs/smb2pdu.c: Negotiate protocol
[ 2466.159125] fs/cifs/transport.c: Sending smb: smb_len=106
[ 2466.196439] fs/cifs/connect.c: RFC1002 header 0xaa
[ 2466.196513] fs/cifs/smb2misc.c: SMB2 data length 42 offset 128
[ 2466.196565] fs/cifs/smb2misc.c: SMB2 len 170
[ 2466.196723] fs/cifs/transport.c: cifs_sync_mid_result: cmd=0 mid=0 state=4
[ 2466.196781] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release
[ 2466.196838] fs/cifs/smb2pdu.c: mode 0x1
[ 2466.196882] fs/cifs/smb2pdu.c: negotiated smb2.0 dialect
[ 2466.196982] fs/cifs/asn1.c: OID len = 10 oid = 0x1 0x3 0x6 0x1
[ 2466.197038] fs/cifs/connect.c: Security Mode: 0x1 Capabilities: 0x300001 TimeAdjust: 0
[ 2466.197083] fs/cifs/smb2pdu.c: Session Setup
[ 2466.197132] fs/cifs/smb2pdu.c: sess setup type 4
[ 2466.197185] fs/cifs/transport.c: Sending smb: smb_len=124
[ 2466.243262] fs/cifs/connect.c: RFC1002 header 0xdc
[ 2466.243298] fs/cifs/smb2misc.c: SMB2 data length 148 offset 72
[ 2466.243305] fs/cifs/smb2misc.c: SMB2 len 220
[ 2466.243376] fs/cifs/transport.c: cifs_sync_mid_result: cmd=1 mid=1 state=4
[ 2466.243532] fs/cifs/smb2maperror.c: Mapping SMB2 status code 0xc0000016 to POSIX err -5
[ 2466.243542] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release
[ 2466.243625] fs/cifs/smb2pdu.c: rawntlmssp session setup challenge phase
[ 2466.260371] fs/cifs/transport.c: Sending smb: smb_len=310
[ 2466.786417] fs/cifs/connect.c: RFC1002 header 0x48
[ 2466.786460] fs/cifs/smb2misc.c: SMB2 data length 0 offset 72
[ 2466.786469] fs/cifs/smb2misc.c: SMB2 len 73
[ 2466.786694] fs/cifs/smb2misc.c: Calculated size 73 length 72 mismatch mid 2
[ 2466.786810] fs/cifs/transport.c: cifs_sync_mid_result: cmd=1 mid=2 state=4
[ 2466.786828] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release
[ 2466.787077] fs/cifs/smb2pdu.c: SMB2/3 session established successfully
[ 2466.787229] fs/cifs/connect.c: CIFS VFS: leaving cifs_get_smb_ses (xid = 1) rc = 0
[ 2466.787373] fs/cifs/connect.c: CIFS VFS: in cifs_setup_ipc as Xid: 2 with uid: 0
[ 2466.787487] fs/cifs/smb2pdu.c: TCON
[ 2466.787675] fs/cifs/transport.c: Sending smb: smb_len=152
[ 2466.846776] fs/cifs/connect.c: RFC1002 header 0x50
[ 2466.846823] fs/cifs/smb2misc.c: SMB2 len 80
[ 2466.847300] fs/cifs/smb2ops.c: add 33 credits total=65
[ 2466.847382] fs/cifs/transport.c: cifs_sync_mid_result: cmd=3 mid=3 state=4
[ 2466.847408] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release
[ 2466.847527] fs/cifs/smb2pdu.c: connection to pipe share
[ 2466.847626] fs/cifs/connect.c: CIFS VFS: leaving cifs_setup_ipc (xid = 2) rc = 0
[ 2466.847716] fs/cifs/connect.c: IPC tcon rc = 0 ipc tid = 58268
[ 2466.847833] fs/cifs/connect.c: CIFS VFS: in cifs_get_tcon as Xid: 3 with uid: 0
[ 2466.847843] fs/cifs/smb2pdu.c: TCON
[ 2466.848031] fs/cifs/transport.c: Sending smb: smb_len=158
[ 2466.943307] fs/cifs/connect.c: RFC1002 header 0x50
[ 2466.943355] fs/cifs/smb2misc.c: SMB2 len 80
[ 2466.943373] fs/cifs/smb2ops.c: add 33 credits total=97
[ 2466.943467] fs/cifs/transport.c: cifs_sync_mid_result: cmd=3 mid=4 state=4
[ 2466.943488] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release
[ 2466.943666] fs/cifs/smb2pdu.c: connection to disk share
[ 2466.943766] fs/cifs/connect.c: CIFS VFS: leaving cifs_get_tcon (xid = 3) rc = 0
[ 2466.943854] fs/cifs/connect.c: Tcon rc = 0
[ 2466.944054] fs/cifs/smb2pdu.c: create/open
[ 2466.944185] fs/cifs/transport.c: Sending smb: smb_len=132
[ 2466.993187] fs/cifs/connect.c: RFC1002 header 0x98
[ 2466.993254] fs/cifs/smb2misc.c: SMB2 data length 0 offset 0
[ 2466.993270] fs/cifs/smb2misc.c: SMB2 len 153
[ 2466.993286] fs/cifs/smb2misc.c: Calculated size 153 length 152 mismatch mid 5
[ 2466.993307] fs/cifs/smb2ops.c: add 10 credits total=106
[ 2466.993414] fs/cifs/transport.c: cifs_sync_mid_result: cmd=5 mid=5 state=4
[ 2466.993442] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release
[ 2466.993738] fs/cifs/smb2pdu.c: Query FSInfo level 5
[ 2466.993870] fs/cifs/transport.c: Sending smb: smb_len=109
[ 2467.039768] fs/cifs/connect.c: RFC1002 header 0x5c
[ 2467.039822] fs/cifs/smb2misc.c: SMB2 data length 20 offset 72
[ 2467.039834] fs/cifs/smb2misc.c: SMB2 len 92
[ 2467.039851] fs/cifs/smb2ops.c: add 10 credits total=115
[ 2467.040006] fs/cifs/transport.c: cifs_sync_mid_result: cmd=16 mid=6 state=4
[ 2467.040021] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release
[ 2467.040039] fs/cifs/smb2pdu.c: Query FSInfo level 4
[ 2467.040105] fs/cifs/transport.c: Sending smb: smb_len=109
[ 2467.093835] fs/cifs/connect.c: RFC1002 header 0x50
[ 2467.093895] fs/cifs/smb2misc.c: SMB2 data length 8 offset 72
[ 2467.093909] fs/cifs/smb2misc.c: SMB2 len 80
[ 2467.094007] fs/cifs/smb2ops.c: add 10 credits total=124
[ 2467.094084] fs/cifs/transport.c: cifs_sync_mid_result: cmd=16 mid=7 state=4
[ 2467.094105] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release
[ 2467.094269] fs/cifs/smb2pdu.c: Close
[ 2467.094342] fs/cifs/transport.c: Sending smb: smb_len=92
[ 2467.136116] fs/cifs/connect.c: RFC1002 header 0x7c
[ 2467.136152] fs/cifs/smb2misc.c: SMB2 len 124
[ 2467.136162] fs/cifs/smb2ops.c: add 10 credits total=133
[ 2467.136219] fs/cifs/transport.c: cifs_sync_mid_result: cmd=6 mid=8 state=4
[ 2467.136227] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release
[ 2467.136345] fs/cifs/connect.c: is_path_remote: full_path:
[ 2467.136367] fs/cifs/smb2pdu.c: create/open
[ 2467.136408] fs/cifs/transport.c: Sending smb: smb_len=132
[ 2467.176286] fs/cifs/connect.c: RFC1002 header 0x98
[ 2467.176314] fs/cifs/smb2misc.c: SMB2 data length 0 offset 0
[ 2467.176320] fs/cifs/smb2misc.c: SMB2 len 153
[ 2467.176327] fs/cifs/smb2misc.c: Calculated size 153 length 152 mismatch mid 9
[ 2467.176339] fs/cifs/smb2ops.c: add 10 credits total=142
[ 2467.176393] fs/cifs/transport.c: cifs_sync_mid_result: cmd=5 mid=9 state=4
[ 2467.176402] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release
[ 2467.176417] fs/cifs/smb2pdu.c: Close
[ 2467.212780] fs/cifs/smb2ops.c: add 10 credits total=151
[ 2467.212845] fs/cifs/smb2pdu.c: create/open
[ 2467.256263] fs/cifs/smb2misc.c: SMB2 data length 0 offset 0
[ 2467.256274] fs/cifs/smb2misc.c: Calculated size 153 length 152 mismatch mid 11
[ 2467.256285] fs/cifs/smb2ops.c: add 10 credits total=160
[ 2467.256359] fs/cifs/smb2pdu.c: Close
[ 2467.289638] fs/cifs/smb2ops.c: add 10 credits total=169
[ 2467.289873] fs/cifs/connect.c: CIFS VFS: leaving cifs_mount (xid = 0) rc = 0
[ 2467.294012] fs/cifs/inode.c: CIFS VFS: in cifs_root_iget as Xid: 4 with uid: 0
[ 2467.294118] fs/cifs/inode.c: Getting info on
[ 2467.339730] fs/cifs/smb2misc.c: SMB2 data length 0 offset 0
[ 2467.339741] fs/cifs/smb2misc.c: Calculated size 153 length 152 mismatch mid 13
[ 2467.339774] fs/cifs/smb2misc.c: SMB2 data length 102 offset 72
[ 2467.340050] fs/cifs/smb2pdu.c: Query Info
[ 2467.376660] ------------[ cut here ]------------
[ 2467.376697] refcount_t: underflow; use-after-free.
... and then the call trace I already sent.
Thanks,
Dominik
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v5.1-rc1 cifs bug: underflow; use-after-free.
2019-03-19 16:26 ` Dominik Brodowski
@ 2019-03-20 11:12 ` Aurélien Aptel
2019-03-26 7:18 ` Dominik Brodowski
0 siblings, 1 reply; 12+ messages in thread
From: Aurélien Aptel @ 2019-03-20 11:12 UTC (permalink / raw)
To: Dominik Brodowski; +Cc: sfrench, linux-cifs
Dominik Brodowski <linux@dominikbrodowski.net> writes:
> Thanks for taking a look at this issue. Fortunately, it is easily
> reproducable (at least for me).
Which server are you doing this against? I couldn't reproduce against
Windows Server 2016.
>> If you enable verbose debugging [1], if my theory is correct you should
>> see a lease break messsage followed by "clear cached root file handle"
>> message before the warning.
>
> Hm, no.
Ok well I'm not sure what is happening then. But the final points still
stand:
- since we don't free anything in the release function, there is no
use-after-free.
- the access to the kref is already protected by crfid.fid_mutex so we
could replace it with a regular int and avoid the warning generated by
kref_put() that you see.
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v5.1-rc1 cifs bug: underflow; use-after-free.
2019-03-20 11:12 ` Aurélien Aptel
@ 2019-03-26 7:18 ` Dominik Brodowski
2019-03-26 12:39 ` [PATCH v1] CIFS: prevent refcount underflow Aurelien Aptel
0 siblings, 1 reply; 12+ messages in thread
From: Dominik Brodowski @ 2019-03-26 7:18 UTC (permalink / raw)
To: Aurélien Aptel; +Cc: sfrench, linux-cifs
On Wed, Mar 20, 2019 at 12:12:21PM +0100, Aurélien Aptel wrote:
> Dominik Brodowski <linux@dominikbrodowski.net> writes:
> > Thanks for taking a look at this issue. Fortunately, it is easily
> > reproducable (at least for me).
>
> Which server are you doing this against? I couldn't reproduce against
> Windows Server 2016.
Had to find that out first, as I'm merely a user here: It's OES 2015 with
Samba version 3.6.3.
> >> If you enable verbose debugging [1], if my theory is correct you should
> >> see a lease break messsage followed by "clear cached root file handle"
> >> message before the warning.
> >
> > Hm, no.
>
> Ok well I'm not sure what is happening then. But the final points still
> stand:
>
> - since we don't free anything in the release function, there is no
> use-after-free.
> - the access to the kref is already protected by crfid.fid_mutex so we
> could replace it with a regular int and avoid the warning generated by
> kref_put() that you see.
If you have a patch ready, I can easily test that.
Thanks,
Dominik
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1] CIFS: prevent refcount underflow
2019-03-26 7:18 ` Dominik Brodowski
@ 2019-03-26 12:39 ` Aurelien Aptel
2019-03-26 15:46 ` Dominik Brodowski
2019-03-27 22:36 ` Pavel Shilovsky
0 siblings, 2 replies; 12+ messages in thread
From: Aurelien Aptel @ 2019-03-26 12:39 UTC (permalink / raw)
To: linux-cifs; +Cc: linux, Aurelien Aptel
Replace kref_t by a simple refcount. We do not care about the
atomicity of the operation as long as the mutex is held.
This fixes a false-positive memory leak warning from kref_put() where
we close the cached fid twice and make the kref underflow.
Link: https://lore.kernel.org/linux-cifs/20190319115151.GA2092@light.dominikbrodowski.net/
Signed-off-by: Aurelien Aptel <aaptel@suse.com>
---
fs/cifs/cifsglob.h | 2 +-
fs/cifs/smb2ops.c | 34 ++++++++++++++--------------------
2 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 38feae812b47..256cd48fb4c7 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -972,7 +972,7 @@ struct cached_fid {
bool is_valid:1; /* Do we have a useable root fid */
bool file_all_info_is_valid:1;
- struct kref refcount;
+ refcount_t refcount;
struct cifs_fid *fid;
struct mutex fid_mutex;
struct cifs_tcon *tcon;
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 1022a3771e14..062c081a298c 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -608,25 +608,21 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
return rc;
}
-static void
-smb2_close_cached_fid(struct kref *ref)
-{
- struct cached_fid *cfid = container_of(ref, struct cached_fid,
- refcount);
-
- if (cfid->is_valid) {
- cifs_dbg(FYI, "clear cached root file handle\n");
- SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
- cfid->fid->volatile_fid);
- cfid->is_valid = false;
- cfid->file_all_info_is_valid = false;
- }
-}
-
void close_shroot(struct cached_fid *cfid)
{
+ unsigned int n;
mutex_lock(&cfid->fid_mutex);
- kref_put(&cfid->refcount, smb2_close_cached_fid);
+ n = refcount_read(&cfid->refcount);
+ if (n > 0) {
+ refcount_dec(&cfid->refcount);
+ if (n == 1 && cfid->is_valid) {
+ cifs_dbg(FYI, "clear cached root file handle\n");
+ SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
+ cfid->fid->volatile_fid);
+ cfid->is_valid = false;
+ cfid->file_all_info_is_valid = false;
+ }
+ }
mutex_unlock(&cfid->fid_mutex);
}
@@ -662,7 +658,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
if (tcon->crfid.is_valid) {
cifs_dbg(FYI, "found a cached root file handle\n");
memcpy(pfid, tcon->crfid.fid, sizeof(struct cifs_fid));
- kref_get(&tcon->crfid.refcount);
+ refcount_inc(&tcon->crfid.refcount);
mutex_unlock(&tcon->crfid.fid_mutex);
return 0;
}
@@ -728,9 +724,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
tcon->crfid.tcon = tcon;
tcon->crfid.is_valid = true;
- kref_init(&tcon->crfid.refcount);
- kref_get(&tcon->crfid.refcount);
-
+ refcount_set(&tcon->crfid.refcount, 1);
qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1] CIFS: prevent refcount underflow
2019-03-26 12:39 ` [PATCH v1] CIFS: prevent refcount underflow Aurelien Aptel
@ 2019-03-26 15:46 ` Dominik Brodowski
2019-03-26 16:53 ` Aurélien Aptel
2019-03-27 22:36 ` Pavel Shilovsky
1 sibling, 1 reply; 12+ messages in thread
From: Dominik Brodowski @ 2019-03-26 15:46 UTC (permalink / raw)
To: Aurelien Aptel; +Cc: linux-cifs
On Tue, Mar 26, 2019 at 01:39:21PM +0100, Aurelien Aptel wrote:
> Replace kref_t by a simple refcount. We do not care about the
> atomicity of the operation as long as the mutex is held.
>
> This fixes a false-positive memory leak warning from kref_put() where
> we close the cached fid twice and make the kref underflow.
>
> Link: https://lore.kernel.org/linux-cifs/20190319115151.GA2092@light.dominikbrodowski.net/
>
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
Reported-and-tested-by: Dominik Brodowski <linux@dominikbrodowski.net>
Thanks,
Dominik
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1] CIFS: prevent refcount underflow
2019-03-26 15:46 ` Dominik Brodowski
@ 2019-03-26 16:53 ` Aurélien Aptel
2019-03-26 16:53 ` Dominik Brodowski
0 siblings, 1 reply; 12+ messages in thread
From: Aurélien Aptel @ 2019-03-26 16:53 UTC (permalink / raw)
To: Dominik Brodowski; +Cc: linux-cifs
Dominik Brodowski <linux@dominikbrodowski.net> writes:
> Reported-and-tested-by: Dominik Brodowski <linux@dominikbrodowski.net>
Great, thanks Dominik.
I chose to keep a refcount_t instead of a simple int because it still checks
for overflows.
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1] CIFS: prevent refcount underflow
2019-03-26 16:53 ` Aurélien Aptel
@ 2019-03-26 16:53 ` Dominik Brodowski
0 siblings, 0 replies; 12+ messages in thread
From: Dominik Brodowski @ 2019-03-26 16:53 UTC (permalink / raw)
To: Aurélien Aptel; +Cc: linux-cifs
On Tue, Mar 26, 2019 at 05:53:15PM +0100, Aurélien Aptel wrote:
>
> Dominik Brodowski <linux@dominikbrodowski.net> writes:
> > Reported-and-tested-by: Dominik Brodowski <linux@dominikbrodowski.net>
>
> Great, thanks Dominik.
>
> I chose to keep a refcount_t instead of a simple int because it still checks
> for overflows.
Yes, that sounds to be the better option.
Thanks,
Dominik
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1] CIFS: prevent refcount underflow
2019-03-26 12:39 ` [PATCH v1] CIFS: prevent refcount underflow Aurelien Aptel
2019-03-26 15:46 ` Dominik Brodowski
@ 2019-03-27 22:36 ` Pavel Shilovsky
2019-03-27 23:44 ` ronnie sahlberg
1 sibling, 1 reply; 12+ messages in thread
From: Pavel Shilovsky @ 2019-03-27 22:36 UTC (permalink / raw)
To: Aurelien Aptel, Steve French, Ronnie Sahlberg; +Cc: linux-cifs, linux
вт, 26 мар. 2019 г. в 05:39, Aurelien Aptel <aaptel@suse.com>:
>
> Replace kref_t by a simple refcount. We do not care about the
> atomicity of the operation as long as the mutex is held.
>
> This fixes a false-positive memory leak warning from kref_put() where
> we close the cached fid twice and make the kref underflow.
>
> Link: https://lore.kernel.org/linux-cifs/20190319115151.GA2092@light.dominikbrodowski.net/
>
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> ---
> fs/cifs/cifsglob.h | 2 +-
> fs/cifs/smb2ops.c | 34 ++++++++++++++--------------------
> 2 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 38feae812b47..256cd48fb4c7 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -972,7 +972,7 @@ struct cached_fid {
> bool is_valid:1; /* Do we have a useable root fid */
> bool file_all_info_is_valid:1;
>
> - struct kref refcount;
> + refcount_t refcount;
> struct cifs_fid *fid;
> struct mutex fid_mutex;
> struct cifs_tcon *tcon;
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 1022a3771e14..062c081a298c 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -608,25 +608,21 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
> return rc;
> }
>
> -static void
> -smb2_close_cached_fid(struct kref *ref)
> -{
> - struct cached_fid *cfid = container_of(ref, struct cached_fid,
> - refcount);
> -
> - if (cfid->is_valid) {
> - cifs_dbg(FYI, "clear cached root file handle\n");
> - SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> - cfid->fid->volatile_fid);
> - cfid->is_valid = false;
> - cfid->file_all_info_is_valid = false;
> - }
> -}
> -
> void close_shroot(struct cached_fid *cfid)
> {
> + unsigned int n;
> mutex_lock(&cfid->fid_mutex);
> - kref_put(&cfid->refcount, smb2_close_cached_fid);
> + n = refcount_read(&cfid->refcount);
> + if (n > 0) {
> + refcount_dec(&cfid->refcount);
> + if (n == 1 && cfid->is_valid) {
> + cifs_dbg(FYI, "clear cached root file handle\n");
> + SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> + cfid->fid->volatile_fid);
> + cfid->is_valid = false;
> + cfid->file_all_info_is_valid = false;
> + }
> + }
> mutex_unlock(&cfid->fid_mutex);
> }
>
> @@ -662,7 +658,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> if (tcon->crfid.is_valid) {
> cifs_dbg(FYI, "found a cached root file handle\n");
> memcpy(pfid, tcon->crfid.fid, sizeof(struct cifs_fid));
> - kref_get(&tcon->crfid.refcount);
> + refcount_inc(&tcon->crfid.refcount);
> mutex_unlock(&tcon->crfid.fid_mutex);
> return 0;
> }
> @@ -728,9 +724,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> tcon->crfid.tcon = tcon;
> tcon->crfid.is_valid = true;
> - kref_init(&tcon->crfid.refcount);
> - kref_get(&tcon->crfid.refcount);
> -
> + refcount_set(&tcon->crfid.refcount, 1);
>
> qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
> --
> 2.16.4
>
I looked at the usage of cached root handle: we call open_shroot in
two places (https://github.com/smfrench/smb3-kernel/search?q=open_shroot&unscoped_q=open_shroot):
1) smb3_qfs_tcon
2) smb2_query_path_info
In both places we call close_shroot() after we use the handle. Another
extra reference (that keeps the file handle opened) is being acquired
by open_shroot itself:
kref_init(&tcon->crfid.refcount); <--- initialize to 1
kref_get(&tcon->crfid.refcount); <--- bump to 2
So, once we stopped using the handle, there should be one reference
left. This reference is being put by the lease break handling code
when such a lease break comes. But here is the problem:
--------------------------------open_shroot()--------------------------------
rc = compound_send_recv(xid, ses, flags, 2, rqst,
resp_buftype, rsp_iov);
if (rc)
goto oshr_exit;
o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
oparms.fid->persistent_fid = o_rsp->PersistentFileId;
oparms.fid->volatile_fid = o_rsp->VolatileFileId;
#ifdef CONFIG_CIFS_DEBUG2
oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
#endif /* CIFS_DEBUG2 */
if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
oplock = smb2_parse_lease_state(server, o_rsp,
&oparms.fid->epoch,
oparms.fid->lease_key);
else
goto oshr_exit;
^^^
------------------------------------------------------------------------------------
if we the server doesn't respond with a lease, we return with rc=0
immediately without marking such a handle as cached and without
holding any reference to the handle (even the one that were
requested). Then we call close_shroot() trying to put non-existing
reference -- BUG that was reported.
The proper fix would be to continue initializing the cached root
handle but skip getting the extra reference (see kref_get call
explained above) if the server didn't give a lease. It doesn't seem
that any other changes are needed here.
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1] CIFS: prevent refcount underflow
2019-03-27 22:36 ` Pavel Shilovsky
@ 2019-03-27 23:44 ` ronnie sahlberg
0 siblings, 0 replies; 12+ messages in thread
From: ronnie sahlberg @ 2019-03-27 23:44 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: Aurelien Aptel, Steve French, Ronnie Sahlberg, linux-cifs, linux
On Thu, Mar 28, 2019 at 8:36 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> вт, 26 мар. 2019 г. в 05:39, Aurelien Aptel <aaptel@suse.com>:
> >
> > Replace kref_t by a simple refcount. We do not care about the
> > atomicity of the operation as long as the mutex is held.
> >
> > This fixes a false-positive memory leak warning from kref_put() where
> > we close the cached fid twice and make the kref underflow.
> >
> > Link: https://lore.kernel.org/linux-cifs/20190319115151.GA2092@light.dominikbrodowski.net/
> >
> > Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> > ---
> > fs/cifs/cifsglob.h | 2 +-
> > fs/cifs/smb2ops.c | 34 ++++++++++++++--------------------
> > 2 files changed, 15 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 38feae812b47..256cd48fb4c7 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -972,7 +972,7 @@ struct cached_fid {
> > bool is_valid:1; /* Do we have a useable root fid */
> > bool file_all_info_is_valid:1;
> >
> > - struct kref refcount;
> > + refcount_t refcount;
> > struct cifs_fid *fid;
> > struct mutex fid_mutex;
> > struct cifs_tcon *tcon;
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 1022a3771e14..062c081a298c 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -608,25 +608,21 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
> > return rc;
> > }
> >
> > -static void
> > -smb2_close_cached_fid(struct kref *ref)
> > -{
> > - struct cached_fid *cfid = container_of(ref, struct cached_fid,
> > - refcount);
> > -
> > - if (cfid->is_valid) {
> > - cifs_dbg(FYI, "clear cached root file handle\n");
> > - SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> > - cfid->fid->volatile_fid);
> > - cfid->is_valid = false;
> > - cfid->file_all_info_is_valid = false;
> > - }
> > -}
> > -
> > void close_shroot(struct cached_fid *cfid)
> > {
> > + unsigned int n;
> > mutex_lock(&cfid->fid_mutex);
> > - kref_put(&cfid->refcount, smb2_close_cached_fid);
> > + n = refcount_read(&cfid->refcount);
> > + if (n > 0) {
> > + refcount_dec(&cfid->refcount);
> > + if (n == 1 && cfid->is_valid) {
> > + cifs_dbg(FYI, "clear cached root file handle\n");
> > + SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> > + cfid->fid->volatile_fid);
> > + cfid->is_valid = false;
> > + cfid->file_all_info_is_valid = false;
> > + }
> > + }
> > mutex_unlock(&cfid->fid_mutex);
> > }
> >
> > @@ -662,7 +658,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> > if (tcon->crfid.is_valid) {
> > cifs_dbg(FYI, "found a cached root file handle\n");
> > memcpy(pfid, tcon->crfid.fid, sizeof(struct cifs_fid));
> > - kref_get(&tcon->crfid.refcount);
> > + refcount_inc(&tcon->crfid.refcount);
> > mutex_unlock(&tcon->crfid.fid_mutex);
> > return 0;
> > }
> > @@ -728,9 +724,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> > memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> > tcon->crfid.tcon = tcon;
> > tcon->crfid.is_valid = true;
> > - kref_init(&tcon->crfid.refcount);
> > - kref_get(&tcon->crfid.refcount);
> > -
> > + refcount_set(&tcon->crfid.refcount, 1);
> >
> > qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> > if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
> > --
> > 2.16.4
> >
>
> I looked at the usage of cached root handle: we call open_shroot in
> two places (https://github.com/smfrench/smb3-kernel/search?q=open_shroot&unscoped_q=open_shroot):
>
> 1) smb3_qfs_tcon
> 2) smb2_query_path_info
>
> In both places we call close_shroot() after we use the handle. Another
> extra reference (that keeps the file handle opened) is being acquired
> by open_shroot itself:
>
> kref_init(&tcon->crfid.refcount); <--- initialize to 1
> kref_get(&tcon->crfid.refcount); <--- bump to 2
>
> So, once we stopped using the handle, there should be one reference
> left. This reference is being put by the lease break handling code
> when such a lease break comes. But here is the problem:
>
> --------------------------------open_shroot()--------------------------------
> rc = compound_send_recv(xid, ses, flags, 2, rqst,
> resp_buftype, rsp_iov);
> if (rc)
> goto oshr_exit;
>
> o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
> oparms.fid->persistent_fid = o_rsp->PersistentFileId;
> oparms.fid->volatile_fid = o_rsp->VolatileFileId;
> #ifdef CONFIG_CIFS_DEBUG2
> oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
> #endif /* CIFS_DEBUG2 */
>
> if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> oplock = smb2_parse_lease_state(server, o_rsp,
> &oparms.fid->epoch,
>
> oparms.fid->lease_key);
> else
> goto oshr_exit;
> ^^^
> ------------------------------------------------------------------------------------
>
> if we the server doesn't respond with a lease, we return with rc=0
> immediately without marking such a handle as cached and without
> holding any reference to the handle (even the one that were
> requested). Then we call close_shroot() trying to put non-existing
> reference -- BUG that was reported.
>
> The proper fix would be to continue initializing the cached root
> handle but skip getting the extra reference (see kref_get call
> explained above) if the server didn't give a lease. It doesn't seem
> that any other changes are needed here.
Good analysis. I think you are right.
I would actually also move this initialization so it happens in
oshr_exit: and make it conditional to rc==0.
I will send a patch for this.
Thanks.
>
> --
> Best regards,
> Pavel Shilovsky
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-03-27 23:44 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-19 11:51 v5.1-rc1 cifs bug: underflow; use-after-free Dominik Brodowski
2019-03-19 15:26 ` Aurélien Aptel
2019-03-19 15:47 ` Aurélien Aptel
2019-03-19 16:26 ` Dominik Brodowski
2019-03-20 11:12 ` Aurélien Aptel
2019-03-26 7:18 ` Dominik Brodowski
2019-03-26 12:39 ` [PATCH v1] CIFS: prevent refcount underflow Aurelien Aptel
2019-03-26 15:46 ` Dominik Brodowski
2019-03-26 16:53 ` Aurélien Aptel
2019-03-26 16:53 ` Dominik Brodowski
2019-03-27 22:36 ` Pavel Shilovsky
2019-03-27 23:44 ` ronnie sahlberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.