вт, 19 июн. 2018 г. в 0:25, Lars Persson <lars.persson@xxxxxxxx>:
>
> 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@xxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for catching this!
Reviewed-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx>
--
Best regards,
Pavel Shilovsky
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
