[PATCH] cifs: Fix use after free of a mid_q_entry
Steve French
smfrench at gmail.com
Thu Jun 14 20:55:19 UTC 2018
Looks plausible - but would like additional feedback if others have opinions
On Thu, Jun 14, 2018 at 3:38 AM, Lars Persson <lars.persson at axis.com> 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 at axis.com>
> ---
> 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
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Steve
More information about the samba-technical
mailing list