v5.1-rc1 cifs bug: underflow; use-after-free.

v5.1-rc1 cifs bug: underflow; use-after-free.

[Dominik Brodowski]

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

Re: v5.1-rc1 cifs bug: underflow; use-after-free.

[Aurélien Aptel]

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)

Re: v5.1-rc1 cifs bug: underflow; use-after-free.

[Aurélien Aptel]

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)

Re: v5.1-rc1 cifs bug: underflow; use-after-free.

[Dominik Brodowski]

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

Re: v5.1-rc1 cifs bug: underflow; use-after-free.

[Aurélien Aptel]

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)

Re: v5.1-rc1 cifs bug: underflow; use-after-free.

[Dominik Brodowski]

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

[PATCH v1] CIFS: prevent refcount underflow

[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

Re: [PATCH v1] CIFS: prevent refcount underflow

[Dominik Brodowski]

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

Re: [PATCH v1] CIFS: prevent refcount underflow

[Aurélien Aptel]

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)

Re: [PATCH v1] CIFS: prevent refcount underflow

[Dominik Brodowski]

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

Re: [PATCH v1] CIFS: prevent refcount underflow

[Pavel Shilovsky]

вт, 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

Re: [PATCH v1] CIFS: prevent refcount underflow

[ronnie sahlberg]

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