cifs: Fix use after free of a mid_q_entry

Message ID 20180614083809.3596-1-larper@axis.com
State New
Headers show
Series
  • cifs: Fix use after free of a mid_q_entry
Related show

Commit Message

Lars Persson June 14, 2018, 8:38 a.m.
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>
---
 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(-)

Comments

Steve French June 14, 2018, 8:55 p.m. | #1
Looks plausible - but would like additional feedback if others have opinions


On Thu, Jun 14, 2018 at 3:38 AM, Lars Persson <lars.persson@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@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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars Persson June 19, 2018, 7:24 a.m. | #2
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@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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Shilovsky June 27, 2018, 12:14 p.m. | #3
вт, 19 июн. 2018 г. в 0:25, Lars Persson <lars.persson@axis.com>:
>
> 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@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
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks for catching this!

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

--
Best regards,
Pavel Shilovsky
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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