cifs: Fix use after free of a mid_q_entry

commit 696e420bb2a6624478105651d5368d45b502b324 upstream.

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@axis.com>
Acked-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
[bwh: Backported to 3.16: Drop redundant assignment to mid_entry]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index dfdb22f6b41911a1b4c4f84eef8e9adadbad439f..0a69907b5f53c5937c0fa8c47e453fa778de4566 100644 (file)
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1232,6 +1232,7 @@ typedef void (mid_callback_t)(struct mid_q_entry *mid);
 /* 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 6e2d4581ceb2c39a9b4faca7acc9cd68e4e751ae..b8d81a86404c5e03c569b819db1f2de7e1df59b8 100644 (file)
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -74,6 +74,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_call_async(struct TCP_Server_Info *server,
                        struct smb_rqst *rqst,

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 9ba78c4ca138e5dd0f81afb355310e0664f9fa22..2b4ffbd37007fbea7362d19c21799d59a2463eda 100644 (file)
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -903,8 +903,11 @@ cifs_demultiplex_thread(void *p)
                else
                        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;
@@ -920,6 +923,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 6c776f53118436fb061dbe7250a7f06e5873f137..67f7073d2a522a89391a560e0ccf011c6f5c8cac 100644 (file)
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -104,6 +104,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 c43a0fd068bf2c79fa504c2f9535e6579766be27..cf6feecf8309c725748bc14fae6e82c09458e2cc 100644 (file)
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -138,6 +138,7 @@ smb2_find_mid(struct TCP_Server_Info *server, char *buf)
                if ((mid->mid == hdr->MessageId) &&
                    (mid->mid_state == MID_REQUEST_SUBMITTED) &&
                    (mid->command == hdr->Command)) {
+                       kref_get(&mid->refcount);
                        spin_unlock(&GlobalMid_Lock);
                        return mid;
                }

diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
index 9e8a1c97dcd032f8467b31452bccc057c7d65781..3fb9be656ce6c2f5d06219e6e9aa5ddd72d656b1 100644 (file)
--- a/fs/cifs/smb2transport.c
+++ b/fs/cifs/smb2transport.c
@@ -531,6 +531,7 @@ smb2_mid_entry_alloc(const struct smb2_hdr *smb_buffer,
                return temp;
        else {
                memset(temp, 0, sizeof(struct mid_q_entry));
+               kref_init(&temp->refcount);
                temp->mid = smb_buffer->MessageId;      /* always LE */
                temp->pid = current->pid;
                temp->command = smb_buffer->Command;    /* Always LE */

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index ecaf72bd60daa8efae017deda288a61b27e294f6..970bac263a5d5fa7be2d1780be81a910987366e6 100644 (file)
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -58,6 +58,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
                return temp;
        else {
                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);
@@ -80,6 +81,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)
 {
@@ -108,7 +124,7 @@ DeleteMidQEntry(struct mid_q_entry *midEntry)
                }
        }
 #endif
-       mempool_free(midEntry, cifs_mid_poolp);
+       cifs_mid_q_entry_release(midEntry);
 }
 
 void