From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f182.google.com ([209.85.128.182]:37082 "EHLO mail-wr0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752174AbdHPDcI (ORCPT ); Tue, 15 Aug 2017 23:32:08 -0400 Received: by mail-wr0-f182.google.com with SMTP id o62so8528415wrc.4 for ; Tue, 15 Aug 2017 20:32:08 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20170816030901.26283-1-toddpoynor@google.com> From: Todd Poynor Date: Tue, 15 Aug 2017 20:32:06 -0700 Message-ID: Subject: Re: [PATCH] scsi: sg: protect accesses to 'reserved' page array To: stable@vger.kernel.org Cc: android-kernel , Hannes Reinecke , Hannes Reinecke , "Martin K . Petersen" , Todd Poynor Content-Type: text/plain; charset="UTF-8" Sender: stable-owner@vger.kernel.org List-ID: Ah, this patch also requires another follow-on fix: > > commit e791ce27c3f6a1d3c746fd6a8f8e36c9540ec6f9 > Author: Hannes Reinecke > Date: Mon Apr 24 10:26:36 2017 +0200 > > scsi: sg: reset 'res_in_use' after unlinking reserved array > > > On Tue, Aug 15, 2017 at 8:09 PM, Todd Poynor wrote: >> >> From: Hannes Reinecke >> >> The 'reserved' page array is used as a short-cut for mapping data, >> saving us to allocate pages per request. However, the 'reserved' array >> is only capable of holding one request, so this patch introduces a mutex >> for protect 'sg_fd' against concurrent accesses. >> >> [toddpoynor@google.com: backport to 3.18-4.9, fixup for bad ioctl >> SG_SET_FORCE_LOW_DMA code removed in later versions and not modified by >> the original patch.] >> >> commit 1bc0eb0446158cc76562176b80623aa119afee5b upstream. >> >> Signed-off-by: Hannes Reinecke >> Reviewed-by: Johannes Thumshirn >> Tested-by: Johannes Thumshirn >> Reviewed-by: Christoph Hellwig >> Signed-off-by: Martin K. Petersen >> Signed-off-by: Todd Poynor >> --- >> Suggested for 4.9, 4.4, and 3.18. This version includes a trivial >> fixup for some bad ioctl code that has been removed. The fix is >> already in 4.12. >> >> Fixes a KASAN use-after-free bug found during multithreaded fuzz testing >> of SCSI char device syscalls, where ioctl SG_SET_RESERVED_SIZE frees a >> pre-allocated "reserve buffer" in a race with an I/O request that uses >> the buffer. >> >> BUG: KASAN: use-after-free in bio_copy_user_iov+xx block/bio.c:1205 >> Call Trace: >> ... >> [] bio_copy_user_iov+0xcdf/0xe50 block/bio.c:1205 >> [] __blk_rq_map_user_iov block/blk-map.c:56 [inline] >> [] blk_rq_map_user_iov+0x22f/0x770 block/blk-map.c:133 >> [] blk_rq_map_user+0x109/0x180 block/blk-map.c:163 >> [] sg_start_req drivers/scsi/sg.c:1766 [inline] >> [] sg_common_write.isra.21+0xc12/0x17a0 >> drivers/scsi/sg.c:775 >> [] sg_write+0x68b/0xb10 drivers/scsi/sg.c:678 >> >> drivers/scsi/sg.c | 47 ++++++++++++++++++++++++++--------------------- >> 1 file changed, 26 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >> index f753df25ba34..cf7e5f096988 100644 >> --- a/drivers/scsi/sg.c >> +++ b/drivers/scsi/sg.c >> @@ -142,6 +142,7 @@ typedef struct sg_fd { /* holds the state >> of a file descriptor */ >> struct sg_device *parentdp; /* owning device */ >> wait_queue_head_t read_wait; /* queue read until command done >> */ >> rwlock_t rq_list_lock; /* protect access to list in req_arr */ >> + struct mutex f_mutex; /* protect against changes in this fd */ >> int timeout; /* defaults to SG_DEFAULT_TIMEOUT */ >> int timeout_user; /* defaults to SG_DEFAULT_TIMEOUT_USER */ >> Sg_scatter_hold reserve; /* buffer held for this file >> descriptor */ >> @@ -155,6 +156,7 @@ typedef struct sg_fd { /* holds the state >> of a file descriptor */ >> unsigned char next_cmd_len; /* 0: automatic, >0: use on next >> write() */ >> char keep_orphan; /* 0 -> drop orphan (def), 1 -> keep for >> read() */ >> char mmap_called; /* 0 -> mmap() never called on this fd */ >> + char res_in_use; /* 1 -> 'reserve' array in use */ >> struct kref f_ref; >> struct execute_work ew; >> } Sg_fd; >> @@ -198,7 +200,6 @@ static void sg_remove_sfp(struct kref *); >> static Sg_request *sg_get_rq_mark(Sg_fd * sfp, int pack_id); >> static Sg_request *sg_add_request(Sg_fd * sfp); >> static int sg_remove_request(Sg_fd * sfp, Sg_request * srp); >> -static int sg_res_in_use(Sg_fd * sfp); >> static Sg_device *sg_get_dev(int dev); >> static void sg_device_destroy(struct kref *kref); >> >> @@ -614,6 +615,7 @@ sg_write(struct file *filp, const char __user *buf, >> size_t count, loff_t * ppos) >> } >> buf += SZ_SG_HEADER; >> __get_user(opcode, buf); >> + mutex_lock(&sfp->f_mutex); >> if (sfp->next_cmd_len > 0) { >> cmd_size = sfp->next_cmd_len; >> sfp->next_cmd_len = 0; /* reset so only this write() >> effected */ >> @@ -622,6 +624,7 @@ sg_write(struct file *filp, const char __user *buf, >> size_t count, loff_t * ppos) >> if ((opcode >= 0xc0) && old_hdr.twelve_byte) >> cmd_size = 12; >> } >> + mutex_unlock(&sfp->f_mutex); >> SCSI_LOG_TIMEOUT(4, sg_printk(KERN_INFO, sdp, >> "sg_write: scsi opcode=0x%02x, cmd_size=%d\n", (int) >> opcode, cmd_size)); >> /* Determine buffer size. */ >> @@ -721,7 +724,7 @@ sg_new_write(Sg_fd *sfp, struct file *file, const char >> __user *buf, >> sg_remove_request(sfp, srp); >> return -EINVAL; /* either MMAP_IO or DIRECT_IO >> (not both) */ >> } >> - if (sg_res_in_use(sfp)) { >> + if (sfp->res_in_use) { >> sg_remove_request(sfp, srp); >> return -EBUSY; /* reserve buffer already being >> used */ >> } >> @@ -892,7 +895,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, >> unsigned long arg) >> return result; >> if (val) { >> sfp->low_dma = 1; >> - if ((0 == sfp->low_dma) && (0 == >> sg_res_in_use(sfp))) { >> + if ((0 == sfp->low_dma) && !sfp->res_in_use) { >> val = (int) sfp->reserve.bufflen; >> sg_remove_scat(sfp, &sfp->reserve); >> sg_build_reserve(sfp, val); >> @@ -967,12 +970,18 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, >> unsigned long arg) >> return -EINVAL; >> val = min_t(int, val, >> >> max_sectors_bytes(sdp->device->request_queue)); >> + mutex_lock(&sfp->f_mutex); >> if (val != sfp->reserve.bufflen) { >> - if (sg_res_in_use(sfp) || sfp->mmap_called) >> + if (sfp->mmap_called || >> + sfp->res_in_use) { >> + mutex_unlock(&sfp->f_mutex); >> return -EBUSY; >> + } >> + >> sg_remove_scat(sfp, &sfp->reserve); >> sg_build_reserve(sfp, val); >> } >> + mutex_unlock(&sfp->f_mutex); >> return 0; >> case SG_GET_RESERVED_SIZE: >> val = min_t(int, sfp->reserve.bufflen, >> @@ -1727,13 +1736,22 @@ sg_start_req(Sg_request *srp, unsigned char *cmd) >> md = &map_data; >> >> if (md) { >> - if (!sg_res_in_use(sfp) && dxfer_len <= rsv_schp->bufflen) >> + mutex_lock(&sfp->f_mutex); >> + if (dxfer_len <= rsv_schp->bufflen && >> + !sfp->res_in_use) { >> + sfp->res_in_use = 1; >> sg_link_reserve(sfp, srp, dxfer_len); >> - else { >> + } else if ((hp->flags & SG_FLAG_MMAP_IO) && >> sfp->res_in_use) { >> + mutex_unlock(&sfp->f_mutex); >> + return -EBUSY; >> + } else { >> res = sg_build_indirect(req_schp, sfp, dxfer_len); >> - if (res) >> + if (res) { >> + mutex_unlock(&sfp->f_mutex); >> return res; >> + } >> } >> + mutex_unlock(&sfp->f_mutex); >> >> md->pages = req_schp->pages; >> md->page_order = req_schp->page_order; >> @@ -2135,6 +2153,7 @@ sg_add_sfp(Sg_device * sdp) >> rwlock_init(&sfp->rq_list_lock); >> >> kref_init(&sfp->f_ref); >> + mutex_init(&sfp->f_mutex); >> sfp->timeout = SG_DEFAULT_TIMEOUT; >> sfp->timeout_user = SG_DEFAULT_TIMEOUT_USER; >> sfp->force_packid = SG_DEF_FORCE_PACK_ID; >> @@ -2210,20 +2229,6 @@ sg_remove_sfp(struct kref *kref) >> schedule_work(&sfp->ew.work); >> } >> >> -static int >> -sg_res_in_use(Sg_fd * sfp) >> -{ >> - const Sg_request *srp; >> - unsigned long iflags; >> - >> - read_lock_irqsave(&sfp->rq_list_lock, iflags); >> - for (srp = sfp->headrp; srp; srp = srp->nextrp) >> - if (srp->res_used) >> - break; >> - read_unlock_irqrestore(&sfp->rq_list_lock, iflags); >> - return srp ? 1 : 0; >> -} >> - >> #ifdef CONFIG_SCSI_PROC_FS >> static int >> sg_idr_max_id(int id, void *p, void *data) >> -- >> 2.14.1.480.gb18f417b89-goog >> >