Hi
For your reference, this is a KASAN log for one of the use-after-free possibilies.
==================================================================
BUG: KASAN: use-after-free in cifs_demultiplex_thread+0xc77/0x1200
Write of size 4 at addr ffff880032a29958 by task cifsd/1199
CPU: 0 PID: 1199 Comm: cifsd Tainted: G C 4.17.0+ #4
Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
Call Trace:
dump_stack+0x7b/0xb5
print_address_description+0x6f/0x280
kasan_report+0x25b/0x380
? cifs_demultiplex_thread+0xc77/0x1200
__asan_store4+0x7b/0x80
cifs_demultiplex_thread+0xc77/0x1200
? cifs_handle_standard+0x280/0x280
? compat_start_thread+0x60/0x60
? kasan_check_write+0x14/0x20
? finish_task_switch+0xf6/0x3b0
? __schedule+0x502/0xd80
? __sched_text_start+0x8/0x8
? __kthread_parkme+0xcb/0x100
? kthread_blkcg+0x70/0x70
kthread+0x180/0x1d0
? cifs_handle_standard+0x280/0x280
? kthread_create_worker_on_cpu+0xc0/0xc0
ret_from_fork+0x35/0x40
Allocated by task 2688:
save_stack+0x46/0xd0
kasan_kmalloc+0xad/0xe0
kasan_slab_alloc+0x11/0x20
kmem_cache_alloc+0xe8/0x200
mempool_alloc_slab+0x15/0x20
mempool_alloc+0x111/0x280
smb2_mid_entry_alloc+0x3e/0x180
smb2_setup_request+0x14f/0x2b0
cifs_send_recv+0x191/0x800
smb2_send_recv+0x1b3/0x300
SMB2_open+0x8ca/0x13a0
smb2_open_op_close+0x122/0x300
smb2_query_path_info+0x85/0x110
cifs_get_inode_info+0x911/0xf00
cifs_lookup+0x434/0x670
cifs_atomic_open+0xe2/0x640
path_openat+0x1484/0x1de0
do_filp_open+0x126/0x1c0
do_sys_open+0x17d/0x2b0
__x64_sys_openat+0x59/0x70
do_syscall_64+0x78/0x170
entry_SYSCALL_64_after_hwframe+0x44/0xa9
Freed by task 2688:
save_stack+0x46/0xd0
__kasan_slab_free+0x129/0x190
kasan_slab_free+0xe/0x10
kmem_cache_free+0x7c/0x210
mempool_free_slab+0x17/0x20
mempool_free+0x65/0x170
DeleteMidQEntry+0x71/0x90
cifs_delete_mid+0x7f/0x90
cifs_send_recv+0x47f/0x800
smb2_send_recv+0x1b3/0x300
SMB2_open+0x8ca/0x13a0
smb2_open_op_close+0x122/0x300
smb2_query_path_info+0x85/0x110
cifs_get_inode_info+0x911/0xf00
cifs_lookup+0x434/0x670
cifs_atomic_open+0xe2/0x640
path_openat+0x1484/0x1de0
do_filp_open+0x126/0x1c0
do_sys_open+0x17d/0x2b0
__x64_sys_openat+0x59/0x70
do_syscall_64+0x78/0x170
entry_SYSCALL_64_after_hwframe+0x44/0xa9
The buggy address belongs to the object at ffff880032a29900
which belongs to the cache cifs_mpx_ids of size 104
The buggy address is located 88 bytes inside of
104-byte region [ffff880032a29900, ffff880032a29968)
The buggy address belongs to the page:
page:ffffea0000ca8a40 count:1 mapcount:0 mapping:0000000000000000 index:0x0
flags: 0xfffffc0000100(slab)
raw: 000fffffc0000100 0000000000000000 0000000000000000 0000000180150015
raw: ffffea000059a600 0000000a0000000a ffff880011eb1340 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff880032a29800: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
ffff880032a29880: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc
>ffff880032a29900: fb fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc
^
ffff880032a29980: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
ffff880032a29a00: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc
==================================================================
On 06/14/2018 10:38 AM, Lars Persson wrote:
With protocol version 2.0 mounts we have seen crashes with corrupt mid
entries. Either the server->pending_mid_q list becomes corrupt with a
cyclic reference in one element or a mid object fetched by the
demultiplexer thread becomes overwritten during use.
Code review identified a race between the demultiplexer thread and the
request issuing thread. The demultiplexer thread seems to be written
with the assumption that it is the sole user of the mid object until
it calls the mid callback which either wakes the issuer task or
deletes the mid.
This assumption is not true because the issuer task can be woken up
earlier by a signal. If the demultiplexer thread has proceeded as far
as setting the mid_state to MID_RESPONSE_RECEIVED then the issuer
thread will happily end up calling cifs_delete_mid while the
demultiplexer thread still is using the mid object.
Inserting a delay in the cifs demultiplexer thread widens the race
window and makes reproduction of the race very easy:
if (server->large_buf)
buf = server->bigbuf;
+ usleep_range(500, 4000);
server->lstrp = jiffies;
To resolve this I think the proper solution involves putting a
reference count on the mid object. This patch makes sure that the
demultiplexer thread holds a reference until it has finished
processing the transaction.
Signed-off-by: Lars Persson <larper@xxxxxxxx>
---
fs/cifs/cifsglob.h | 1 +
fs/cifs/cifsproto.h | 1 +
fs/cifs/connect.c | 7 ++++++-
fs/cifs/smb1ops.c | 1 +
fs/cifs/smb2ops.c | 1 +
fs/cifs/smb2transport.c | 1 +
fs/cifs/transport.c | 18 +++++++++++++++++-
7 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index cb950a5fa078..c7ee09d9a236 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1362,6 +1362,7 @@ typedef int (mid_handle_t)(struct TCP_Server_Info *server,
/* one of these for every pending CIFS request to the server */
struct mid_q_entry {
struct list_head qhead; /* mids waiting on reply from this server */
+ struct kref refcount;
struct TCP_Server_Info *server; /* server corresponding to this mid */
__u64 mid; /* multiplex id */
__u32 pid; /* process id */
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 365a414a75e9..c4e5c69810f9 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -76,6 +76,7 @@ extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
struct TCP_Server_Info *server);
extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
extern void cifs_delete_mid(struct mid_q_entry *mid);
+extern void cifs_mid_q_entry_release(struct mid_q_entry *midEntry);
extern void cifs_wake_up_task(struct mid_q_entry *mid);
extern int cifs_handle_standard(struct TCP_Server_Info *server,
struct mid_q_entry *mid);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 7a10a5d0731f..90cedf6b3228 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -920,8 +920,11 @@ cifs_demultiplex_thread(void *p)
length = mid_entry->receive(server, mid_entry);
}
- if (length < 0)
+ if (length < 0) {
+ if (mid_entry)
+ cifs_mid_q_entry_release(mid_entry);
continue;
+ }
if (server->large_buf)
buf = server->bigbuf;
@@ -938,6 +941,8 @@ cifs_demultiplex_thread(void *p)
if (!mid_entry->multiRsp || mid_entry->multiEnd)
mid_entry->callback(mid_entry);
+
+ cifs_mid_q_entry_release(mid_entry);
} else if (server->ops->is_oplock_break &&
server->ops->is_oplock_break(buf, server)) {
cifs_dbg(FYI, "Received oplock break\n");
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index aff8ce8ba34d..646dcd149de1 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -107,6 +107,7 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer)
if (compare_mid(mid->mid, buf) &&
mid->mid_state == MID_REQUEST_SUBMITTED &&
le16_to_cpu(mid->command) == buf->Command) {
+ kref_get(&mid->refcount);
spin_unlock(&GlobalMid_Lock);
return mid;
}
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 9c6d95ffca97..ba0bc31786d1 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -203,6 +203,7 @@ smb2_find_mid(struct TCP_Server_Info *server, char *buf)
if ((mid->mid == wire_mid) &&
(mid->mid_state == MID_REQUEST_SUBMITTED) &&
(mid->command == shdr->Command)) {
+ kref_get(&mid->refcount);
spin_unlock(&GlobalMid_Lock);
return mid;
}
diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
index 8806f3f76c1d..97f24d82ae6b 100644
--- a/fs/cifs/smb2transport.c
+++ b/fs/cifs/smb2transport.c
@@ -548,6 +548,7 @@ smb2_mid_entry_alloc(const struct smb2_sync_hdr *shdr,
temp = mempool_alloc(cifs_mid_poolp, GFP_NOFS);
memset(temp, 0, sizeof(struct mid_q_entry));
+ kref_init(&temp->refcount);
temp->mid = le64_to_cpu(shdr->MessageId);
temp->pid = current->pid;
temp->command = shdr->Command; /* Always LE */
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 927226a2122f..60faf2fcec7f 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -61,6 +61,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
temp = mempool_alloc(cifs_mid_poolp, GFP_NOFS);
memset(temp, 0, sizeof(struct mid_q_entry));
+ kref_init(&temp->refcount);
temp->mid = get_mid(smb_buffer);
temp->pid = current->pid;
temp->command = cpu_to_le16(smb_buffer->Command);
@@ -82,6 +83,21 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
return temp;
}
+static void _cifs_mid_q_entry_release(struct kref *refcount)
+{
+ struct mid_q_entry *mid = container_of(refcount, struct mid_q_entry,
+ refcount);
+
+ mempool_free(mid, cifs_mid_poolp);
+}
+
+void cifs_mid_q_entry_release(struct mid_q_entry *midEntry)
+{
+ spin_lock(&GlobalMid_Lock);
+ kref_put(&midEntry->refcount, _cifs_mid_q_entry_release);
+ spin_unlock(&GlobalMid_Lock);
+}
+
void
DeleteMidQEntry(struct mid_q_entry *midEntry)
{
@@ -110,7 +126,7 @@ DeleteMidQEntry(struct mid_q_entry *midEntry)
}
}
#endif
- mempool_free(midEntry, cifs_mid_poolp);
+ cifs_mid_q_entry_release(midEntry);
}
void
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
