| From: Lars Persson <lars.persson@axis.com> |
| Date: Mon, 25 Jun 2018 14:05:25 +0200 |
| Subject: 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> |
| --- |
| fs/cifs/cifsglob.h | 1 + |
| fs/cifs/cifsproto.h | 1 + |
| fs/cifs/connect.c | 8 +++++++- |
| fs/cifs/smb1ops.c | 1 + |
| fs/cifs/smb2ops.c | 1 + |
| fs/cifs/smb2transport.c | 1 + |
| fs/cifs/transport.c | 18 +++++++++++++++++- |
| 7 files changed, 29 insertions(+), 2 deletions(-) |
| |
| --- a/fs/cifs/cifsglob.h |
| +++ b/fs/cifs/cifsglob.h |
| @@ -1232,6 +1232,7 @@ typedef void (mid_callback_t)(struct 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 */ |
| --- a/fs/cifs/cifsproto.h |
| +++ b/fs/cifs/cifsproto.h |
| @@ -74,6 +74,7 @@ extern struct mid_q_entry *AllocMidQEntr |
| 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, |
| --- 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"); |
| --- a/fs/cifs/smb1ops.c |
| +++ b/fs/cifs/smb1ops.c |
| @@ -104,6 +104,7 @@ cifs_find_mid(struct TCP_Server_Info *se |
| 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; |
| } |
| --- a/fs/cifs/smb2ops.c |
| +++ b/fs/cifs/smb2ops.c |
| @@ -138,6 +138,7 @@ smb2_find_mid(struct TCP_Server_Info *se |
| 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; |
| } |
| --- a/fs/cifs/smb2transport.c |
| +++ b/fs/cifs/smb2transport.c |
| @@ -531,6 +531,7 @@ smb2_mid_entry_alloc(const struct smb2_h |
| 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 */ |
| --- a/fs/cifs/transport.c |
| +++ b/fs/cifs/transport.c |
| @@ -58,6 +58,7 @@ AllocMidQEntry(const struct smb_hdr *smb |
| 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 |
| 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 *midE |
| } |
| } |
| #endif |
| - mempool_free(midEntry, cifs_mid_poolp); |
| + cifs_mid_q_entry_release(midEntry); |
| } |
| |
| void |