[PATCH] cifs: Fix use after free of a mid_q_entry

Pavel Shilovsky piastryyy at gmail.com
Wed Jun 27 12:14:25 UTC 2018


вт, 19 июн. 2018 г. в 0:25, Lars Persson <lars.persson at 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 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
> >
> --
> 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 for catching this!

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

--
Best regards,
Pavel Shilovsky



More information about the samba-technical mailing list