sanitize sg
Subsystems (2)
v2 cover sanitize sg
Hi all,
the infamous syzkaller incovered some more issues in the sg driver. This patchset fixes those two issues (and adds a fix for yet another potential issue; checking for a NULL dxferp when dxfer_len is not 0). It also removes handling of the SET_FORCE_LOW_DMA ioctl, which never worked since the initial git checkin. And does some code cleanup by removing the private list implementation, using standard lists instead.
As usual, comments and reviews are welcome.
Changes to v1:
- Include reviews from Christoph
- Add patch to close race condition in sg_remove_sfp_usercontext()
- Remove stale variable 'save_scat_len'
Hannes Reinecke (5):
sg: disable SET_FORCE_LOW_DMA
sg: remove 'save_scat_len'
sg: protect accesses to 'reserved' page array
sg: use standard lists for sg_requests
sg: close race condition in sg_remove_sfp_usercontext()
Johannes Thumshirn (1):
sg: check for valid direction before starting the request
drivers/scsi/sg.c | 284 +++++++++++++++++++++++++++--------------------------- include/scsi/sg.h | 1 - 2 files changed, 141 insertions(+), 144 deletions(-)
Signature · 1 line
1.8.5.6
PATCH v2 1/6 sg: disable SET_FORCE_LOW_DMA
The ioctl SET_FORCE_LOW_DMA has never worked since the initial git check-in, and the respective setting is nowadays handled correctly.
So disable it entirely.
--- drivers/scsi/sg.c | 30 +++++++++--------------------- include/scsi/sg.h | 1 - 2 files changed, 9 insertions(+), 22 deletions(-)
drivers/scsi/sg.c +9 -21
@@ -149,7 +149,6 @@Sg_request *headrp;/* head of request slist, NULL->empty */ struct fasync_struct *async_qp;/* used by asynchronous notification */ Sg_request req_arr[SG_MAX_QUEUE ];/* used as singly-linked list */ -char low_dma;/* as in parent but possibly overridden to 1 */ char force_packid;/* 1 -> pack_id input to read(), 0 -> ignored */ char cmd_q;/* 1 -> allow command queuing, 0 -> don't */ unsigned char next_cmd_len;/* 0: automatic, >0: use on next write() */
@@ -887,24 +886,14 @@ static int max_sectors_bytes(struct request_queue *q)/* strange ..., for backward compatibility */ return sfp->timeout_user;case SG_SET_FORCE_LOW_DMA : - result =get_user (val, ip); -if (result) -return result; -if (val) { - sfp->low_dma =1 ; -if ((0 == sfp->low_dma) && (0 ==sg_res_in_use (sfp))) { - val = (int ) sfp->reserve.bufflen; -sg_remove_scat (sfp, &sfp->reserve); -sg_build_reserve (sfp, val); - } - }else { -if (atomic_read (&sdp->detaching)) -return -ENODEV ; - sfp->low_dma = sdp->device->host->unchecked_isa_dma; - } +/* +* N.B. This ioctl never worked properly, but failed to +* return an error value. So returning '0' to keep compability +* with legacy applications. +*/ return 0 ;case SG_GET_LOW_DMA : -return put_user ((int ) sfp->low_dma, ip); +return put_user ((int ) sdp->device->host->unchecked_isa_dma, ip);case SG_GET_SCSI_ID :if (!access_ok (VERIFY_WRITE , p,sizeof (sg_scsi_id_t)))return -EFAULT ;
@@ -1821,6 +1810,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonint sg_tablesize = sfp->parentdp->sg_tablesize;int blk_size = buff_size, order;gfp_t gfp_mask =GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN; +struct sg_device *sdp = sfp->parentdp;if (blk_size <0 )return -EFAULT ;
@@ -1846,7 +1836,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon scatter_elem_sz_prev = num; } -if (sfp->low_dma) +if (sdp->device->host->unchecked_isa_dma) gfp_mask |=GFP_DMA ;if (!capable (CAP_SYS_ADMIN ) || !capable (CAP_SYS_RAWIO ))
@@ -2132,8 +2122,6 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon sfp->timeout =SG_DEFAULT_TIMEOUT ; sfp->timeout_user =SG_DEFAULT_TIMEOUT_USER ; sfp->force_packid =SG_DEF_FORCE_PACK_ID ; - sfp->low_dma = (SG_DEF_FORCE_LOW_DMA ==0 ) ? - sdp->device->host->unchecked_isa_dma :1 ; sfp->cmd_q =SG_DEF_COMMAND_Q ; sfp->keep_orphan =SG_DEF_KEEP_ORPHAN ; sfp->parentdp = sdp;
@@ -2603,7 +2591,7 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)jiffies_to_msecs (fp->timeout), fp->reserve.bufflen, (int ) fp->reserve.k_use_sg, - (int) fp->low_dma); + (int) sdp->device->host->unchecked_isa_dma);seq_printf (s," cmd_q=%d f_packid=%d k_orphan=%d closed=0\n" , (int ) fp->cmd_q, (int ) fp->force_packid, (int ) fp->keep_orphan);
include/scsi/sg.h +0 -1
@@ -197,7 +197,6 @@#define SG_DEFAULT_RETRIES 0/* Defaults, commented if they differ from original sg driver */ -#define SG_DEF_FORCE_LOW_DMA 0/* was 1 -> memory below 16MB on i386 */ #define SG_DEF_FORCE_PACK_ID 0#define SG_DEF_KEEP_ORPHAN 0#define SG_DEF_RESERVED_SIZE SG_SCATTER_SZ/* load time option */
Signature · 1 line
1.8.5.6
Re: [PATCHv2 1/6] sg: disable SET_FORCE_LOW_DMA
Quoted 7 lines · Hannes Reinecke
The ioctl
SET_FORCE_LOW_DMAhas never worked since the initial git check-in, and the respective setting is nowadays handled correctly.
So disable it entirely.Signed-off-by Hannes Reinecke <[email protected]>---
PATCH v2 2/6 sg: remove 'save_scat_len'
In reply to Hannes ReineckeUnused.
--- drivers/scsi/sg.c | 2 -- 1 file changed, 2 deletions(-)
drivers/scsi/sg.c +0 -2
@@ -145,7 +145,6 @@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 */ -unsigned save_scat_len;/* original length of trunc. scat. element */ Sg_request *headrp;/* head of request slist, NULL->empty */ struct fasync_struct *async_qp;/* used by asynchronous notification */ Sg_request req_arr[SG_MAX_QUEUE ];/* used as singly-linked list */
@@ -2006,7 +2005,6 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon req_schp->pages =NULL ; req_schp->page_order =0 ; req_schp->sglist_len =0 ; - sfp->save_scat_len =0 ; srp->res_used =0 ; }
Signature · 1 line
1.8.5.6
Re: [PATCHv2 2/6] sg: remove 'save_scat_len'
Quoted 5 lines · Hannes Reinecke
Unused.
Signed-off-by Hannes Reinecke <[email protected]>---
PATCH v2 3/6 sg: protect accesses to 'reserved' page array
In reply to Hannes ReineckeThe '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.
--- drivers/scsi/sg.c | 47 +++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-)
drivers/scsi/sg.c +27 -20
@@ -142,6 +142,7 @@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 */
@@ -153,6 +154,7 @@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;
@@ -196,7 +198,6 @@ static int sg_common_write(Sg_fd * sfp, Sg_request * srp,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);
@@ -612,6 +613,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd) } 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 */
@@ -620,6 +622,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)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. */
@@ -719,10 +722,13 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)sg_remove_request (sfp, srp);return -EINVAL ;/* either MMAP_IO or DIRECT_IO (not both) */ } -if (sg_res_in_use (sfp)) { +mutex_lock (&sfp->f_mutex); +if (sfp->res_in_use) { +mutex_unlock (&sfp->f_mutex);sg_remove_request (sfp, srp);return -EBUSY ;/* reserve buffer already being used */ } +mutex_unlock (&sfp->f_mutex); } ul_timeout =msecs_to_jiffies (srp->header.timeout); timeout = (ul_timeout <INT_MAX ) ? ul_timeout :INT_MAX ;
@@ -955,12 +961,18 @@ static int max_sectors_bytes(struct request_queue *q)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,
@@ -986,7 +998,9 @@ static int max_sectors_bytes(struct request_queue *q) result =get_user (val, ip);if (result)return result; +mutex_lock (&sfp->f_mutex); sfp->next_cmd_len = (val >0 ) ? val :0 ; +mutex_unlock (&sfp->f_mutex);return 0 ;case SG_GET_VERSION_NUM :return put_user (sg_version_num, ip);
@@ -1713,13 +1727,19 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon 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 { 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;
@@ -2117,6 +2137,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonrwlock_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 ;
@@ -2190,20 +2211,6 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonschedule_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)
Signature · 1 line
1.8.5.6
Re: [PATCHv2 3/6] sg: protect accesses to 'reserved' page array
Quoted 9 lines · 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.Signed-off-by Hannes Reinecke <[email protected]>---
PATCH v2 4/6 sg: check for valid direction before starting the request
In reply to Hannes ReineckeFrom: Johannes Thumshirn <[email protected]>
Check for a valid direction before starting the request, otherwise we risk running into an assertion in the scsi midlayer checking for vaild requests.
--- drivers/scsi/sg.c | 46 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 12 deletions(-)
drivers/scsi/sg.c +34 -12
@@ -663,18 +663,14 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)* is a non-zero input_size, so emit a warning. */ if (hp->dxfer_direction ==SG_DXFER_TO_FROM_DEV ) { -static char cmd[TASK_COMM_LEN ]; -if (strcmp (current->comm, cmd)) { -printk_ratelimited (KERN_WARNING -"sg_write: data in/out %d/%d bytes " -"for SCSI command 0x%x-- guessing " -"data in;\n program %s not setting " -"count and/or reply_len properly\n" , - old_hdr.reply_len - (int )SZ_SG_HEADER , - input_size, (unsigned int ) cmnd[0 ], - current->comm); -strcpy (cmd, current->comm); - } +printk_ratelimited (KERN_WARNING +"sg_write: data in/out %d/%d bytes " +"for SCSI command 0x%x-- guessing " +"data in;\n program %s not setting " +"count and/or reply_len properly\n" , + old_hdr.reply_len - (int )SZ_SG_HEADER , + input_size, (unsigned int ) cmnd[0 ], + current->comm); } k =sg_common_write (sfp, srp, cmnd, sfp->timeout, blocking);return (k <0 ) ? k : count;
@@ -756,6 +752,29 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)return count; } +static bool sg_is_valid_dxfer (sg_io_hdr_t *hp) +{ +switch (dxfer_direction) { +case SG_DXFER_NONE : +if (hp->dxferp || hp->dxfer_len >0 ) +return false; +return true; +case SG_DXFER_TO_DEV : +case SG_DXFER_FROM_DEV : +case SG_DXFER_TO_FROM_DEV : +if (!hp->dxferp || hp->dxfer_len ==0 ) +return false; +return true; +case SG_DXFER_UNKNOWN : +if ((!hp->dxferp && hp->dxfer_len) || + (hp->dxferp && hp->dxfer_len ==0 )) +return false; +return true; +default : +return false; + } +} +static int sg_common_write (Sg_fd * sfp,Sg_request * srp,unsigned char *cmnd,int timeout,int blocking)
@@ -776,6 +795,9 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)"sg_common_write: scsi opcode=0x%02x, cmd_size=%d\n" , (int) cmnd[0], (int) hp->cmd_len)); +if (!sg_is_valid_dxfer (hp)) +return -EINVAL ; + k =sg_start_req (srp, cmnd);if (k) { SCSI_LOG_TIMEOUT(1, sg_printk(KERN_INFO, sfp->parentdp,
Signature · 1 line
1.8.5.6
Re: [PATCHv2 4/6] sg: check for valid direction before starting the request
Quoted 10 lines · Hannes Reinecke
From: Johannes Thumshirn <[email protected]>
Check for a valid direction before starting the request, otherwise we risk running into an assertion in the scsi midlayer checking for vaild requests.
Signed-off-by Johannes Thumshirn <[email protected]>Reported-by Dmitry Vyukov <[email protected]>Signed-off-by Hannes Reinecke <[email protected]>---
[...]
drivers/scsi/sg.c static int sg_allow_access(struct file *filp, unsigned char *cmd) · line 757
Quoted 4 lines
drivers/scsi/sg.c +3
-0+static bool sg_is_valid_dxfer (sg_io_hdr_t *hp) +{ +switch (dxfer_direction) {
This wont build now... needs a +hp
kbuild test robot
build botRe: [PATCHv2 4/6] sg: check for valid direction before starting the request
In reply to Hannes Reineckereproduce:
Hi Johannes,
[auto build test ERROR on scsi/for-next]
[also build test ERROR on v4.10-rc6 next-20170203]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Hannes-Reinecke/sanitize-sg/20170203-202314 base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next config: x86_64-acpi-redef (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64
Note: the linux-review/Hannes-Reinecke/sanitize-sg/20170203-202314 HEAD c442f1293c347b17429edecbc5d29d4eb39152b0 builds fine.
It only hurts bisectibility.
All error/warnings (new ones prefixed by >>):
drivers/scsi/sg.c: In function 'sg_is_valid_dxfer':
>> drivers/scsi/sg.c:757:10: error: 'dxfer_direction' undeclared (first use in this function)
switch (dxfer_direction) {
^~~~~~~~~~~~~~~
drivers/scsi/sg.c:757:10: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/scsi/sg.c:776:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^vim +/dxfer_direction +757 drivers/scsi/sg.c
751 *o_srp = srp;
752 return count;
753 }
754
755 static bool sg_is_valid_dxfer(sg_io_hdr_t *hp)
756 {
> 757 switch (dxfer_direction) {
758 case SG_DXFER_NONE:
759 if (hp->dxferp || hp->dxfer_len > 0)
760 return false;
761 return true;
762 case SG_DXFER_TO_DEV:
763 case SG_DXFER_FROM_DEV:
764 case SG_DXFER_TO_FROM_DEV:
765 if (!hp->dxferp || hp->dxfer_len == 0)
766 return false;
767 return true;
768 case SG_DXFER_UNKNOWN:
769 if ((!hp->dxferp && hp->dxfer_len) ||
770 (hp->dxferp && hp->dxfer_len == 0))
771 return false;
772 return true;
773 default:
774 return false;
775 }
> 776 } 777
778 static int
779 sg_common_write(Sg_fd * sfp, Sg_request * srp,
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
PATCH v2 5/6 sg: use standard lists for sg_requests
In reply to Hannes Reinecke'Sg_request' is using a private list implementation; convert it to standard lists.
--- drivers/scsi/sg.c | 149 +++++++++++++++++++++++------------------------------- 1 file changed, 62 insertions(+), 87 deletions(-)
drivers/scsi/sg.c +62 -87
@@ -122,7 +122,7 @@struct sg_fd ; typedef struct sg_request { /* SG_MAX_QUEUE requests outstanding per file */ - struct sg_request *nextrp; /* NULL -> tail request (slist) */ + struct list_head entry; /* list entry */ struct sg_fd *parentfp; /* NULL -> not in use */ Sg_scatter_hold data; /* hold buffer, perhaps scatter list */ sg_io_hdr_t header; /* scsi command+info, see <scsi/sg.h> */
@@ -146,7 +146,7 @@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 */ -Sg_request *headrp;/* head of request slist, NULL->empty */ +struct list_head rq_list;/* head of request list */ struct fasync_struct *async_qp;/* used by asynchronous notification */ Sg_request req_arr[SG_MAX_QUEUE ];/* used as singly-linked list */ char force_packid;/* 1 -> pack_id input to read(), 0 -> ignored */
@@ -754,7 +754,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)static bool sg_is_valid_dxfer (sg_io_hdr_t *hp) { -switch (dxfer_direction) { +switch (hp->dxfer_direction) {case SG_DXFER_NONE :if (hp->dxferp || hp->dxfer_len >0 )return false;
@@ -954,7 +954,7 @@ static int max_sectors_bytes(struct request_queue *q)if (!access_ok (VERIFY_WRITE , ip,sizeof (int )))return -EFAULT ;read_lock_irqsave (&sfp->rq_list_lock, iflags); -for (srp = sfp->headrp; srp; srp = srp->nextrp) { +list_for_each_entry (srp, &sfp->rq_list, entry) {if ((1 == srp->done) && (!srp->sg_io_owned)) {read_unlock_irqrestore (&sfp->rq_list_lock, iflags);
@@ -967,7 +967,8 @@ static int max_sectors_bytes(struct request_queue *q)return 0 ;case SG_GET_NUM_WAITING :read_lock_irqsave (&sfp->rq_list_lock, iflags); -for (val =0 , srp = sfp->headrp; srp; srp = srp->nextrp) { + val =0 ; +list_for_each_entry (srp, &sfp->rq_list, entry) {if ((1 == srp->done) && (!srp->sg_io_owned)) ++val; }
@@ -1042,35 +1043,33 @@ static int max_sectors_bytes(struct request_queue *q)if (!rinfo)return -ENOMEM ;read_lock_irqsave (&sfp->rq_list_lock, iflags); -for (srp = sfp->headrp, val =0 ; val <SG_MAX_QUEUE ; - ++val, srp = srp ? srp->nextrp : srp) { + val =0 ; +list_for_each_entry (srp, &sfp->rq_list, entry) { +if (val >SG_MAX_QUEUE ) +break ;memset (&rinfo[val],0 ,SZ_SG_REQ_INFO ); -if (srp) { - rinfo[val].req_state = srp->done +1 ; - rinfo[val].problem = - srp->header.masked_status & - srp->header.host_status & - srp->header.driver_status; -if (srp->done) - rinfo[val].duration = - srp->header.duration; -else { - ms =jiffies_to_msecs (jiffies); - rinfo[val].duration = - (ms > srp->header.duration) ? - (ms - srp->header.duration) :0 ; - } - rinfo[val].orphan = srp->orphan; - rinfo[val].sg_io_owned = - srp->sg_io_owned; - rinfo[val].pack_id = - srp->header.pack_id; - rinfo[val].usr_ptr = - srp->header.usr_ptr; + rinfo[val].req_state = srp->done +1 ; + rinfo[val].problem = + srp->header.masked_status & + srp->header.host_status & + srp->header.driver_status; +if (srp->done) + rinfo[val].duration = + srp->header.duration; +else { + ms =jiffies_to_msecs (jiffies); + rinfo[val].duration = + (ms > srp->header.duration) ? + (ms - srp->header.duration) :0 ; } + rinfo[val].orphan = srp->orphan; + rinfo[val].sg_io_owned = srp->sg_io_owned; + rinfo[val].pack_id = srp->header.pack_id; + rinfo[val].usr_ptr = srp->header.usr_ptr; + val++; }read_unlock_irqrestore (&sfp->rq_list_lock, iflags); - result =__copy_to_user (p, rinfo, + result =__copy_to_user (p, rinfo,SZ_SG_REQ_INFO *SG_MAX_QUEUE ); result = result ? -EFAULT :0 ;kfree (rinfo);
@@ -1176,7 +1175,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonreturn POLLERR ;poll_wait (filp, &sfp->read_wait, wait);read_lock_irqsave (&sfp->rq_list_lock, iflags); -for (srp = sfp->headrp; srp; srp = srp->nextrp) { +list_for_each_entry (srp, &sfp->rq_list, entry) {/* if any read waiting, flag it */ if ((0 == res) && (1 == srp->done) && (!srp->sg_io_owned)) res =POLLIN |POLLRDNORM ;
@@ -2057,7 +2056,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonunsigned long iflags;write_lock_irqsave (&sfp->rq_list_lock, iflags); -for (resp = sfp->headrp; resp; resp = resp->nextrp) { +list_for_each_entry (resp, &sfp->rq_list, entry) {/* look for requests that are ready + not SG_IO owned */ if ((1 == resp->done) && (!resp->sg_io_owned) && ((-1 == pack_id) || (resp->header.pack_id == pack_id))) {
@@ -2075,70 +2074,45 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon {int k;unsigned long iflags; -Sg_request *resp;Sg_request *rp = sfp->req_arr;write_lock_irqsave (&sfp->rq_list_lock, iflags); - resp = sfp->headrp; -if (!resp) { -memset (rp,0 ,sizeof (Sg_request)); - rp->parentfp = sfp; - resp = rp; - sfp->headrp = resp; - }else { -if (0 == sfp->cmd_q) - resp =NULL ;/* command queuing disallowed */ -else { -for (k =0 ; k <SG_MAX_QUEUE ; ++k, ++rp) { -if (!rp->parentfp) -break ; - } -if (k <SG_MAX_QUEUE ) { -memset (rp,0 ,sizeof (Sg_request)); - rp->parentfp = sfp; -while (resp->nextrp) - resp = resp->nextrp; - resp->nextrp = rp; - resp = rp; - }else - resp =NULL ; +if (!list_empty (&sfp->rq_list)) { +if (!sfp->cmd_q) + goto out_unlock; + +for (k =0 ; k <SG_MAX_QUEUE ; ++k, ++rp) { +if (!rp->parentfp) +break ; } +if (k >=SG_MAX_QUEUE ) + goto out_unlock; } -if (resp) { - resp->nextrp =NULL ; - resp->header.duration =jiffies_to_msecs (jiffies); - } +memset (rp,0 ,sizeof (Sg_request)); + rp->parentfp = sfp; + rp->header.duration =jiffies_to_msecs (jiffies); +list_add_tail (&rp->entry, &sfp->rq_list);write_unlock_irqrestore (&sfp->rq_list_lock, iflags); -return resp; +return rp; +out_unlock: +write_unlock_irqrestore (&sfp->rq_list_lock, iflags); +return NULL ; }/* Return of 1 for found; 0 for not found */ static int sg_remove_request (Sg_fd * sfp,Sg_request * srp) { -Sg_request *prev_rp; -Sg_request *rp;unsigned long iflags;int res =0 ; -if ((!sfp) || (!srp) || (!sfp->headrp)) +if (!sfp || !srp ||list_empty (&sfp->rq_list))return res;write_lock_irqsave (&sfp->rq_list_lock, iflags); - prev_rp = sfp->headrp; -if (srp == prev_rp) { - sfp->headrp = prev_rp->nextrp; - prev_rp->parentfp =NULL ; +if (!list_empty (&srp->entry)) { +list_del (&srp->entry); + srp->parentfp =NULL ; res =1 ; - }else { -while ((rp = prev_rp->nextrp)) { -if (srp == rp) { - prev_rp->nextrp = rp->nextrp; - rp->parentfp =NULL ; - res =1 ; -break ; - } - prev_rp = rp; - } }write_unlock_irqrestore (&sfp->rq_list_lock, iflags);return res;
@@ -2157,7 +2131,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned loninit_waitqueue_head (&sfp->read_wait);rwlock_init (&sfp->rq_list_lock); - +INIT_LIST_HEAD (&sfp->rq_list);kref_init (&sfp->f_ref);mutex_init (&sfp->f_mutex); sfp->timeout =SG_DEFAULT_TIMEOUT ;
@@ -2196,10 +2170,13 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon { struct sg_fd *sfp = container_of(work, struct sg_fd, ew.work);struct sg_device *sdp = sfp->parentdp; +Sg_request *srp;/* Cleanup any responses which were never read(). */ -while (sfp->headrp) -sg_finish_rem_req (sfp->headrp); +while (!list_empty (&sfp->rq_list)) { + srp =list_first_entry (&sfp->rq_list, Sg_request, entry); +sg_finish_rem_req (srp); + }if (sfp->reserve.bufflen >0 ) { SCSI_LOG_TIMEOUT(6, sg_printk(KERN_INFO, sdp,
@@ -2602,7 +2579,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)/* must be called while holding sg_index_lock */ static void sg_proc_debug_helper (struct seq_file *s,Sg_device * sdp) { -int k, m, new_interface, blen, usg; +int k, new_interface, blen, usg;Sg_request *srp;Sg_fd *fp;const sg_io_hdr_t *hp;
@@ -2622,13 +2599,11 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)seq_printf (s," cmd_q=%d f_packid=%d k_orphan=%d closed=0\n" , (int ) fp->cmd_q, (int ) fp->force_packid, (int ) fp->keep_orphan); -for (m =0 , srp = fp->headrp; - srp !=NULL ; - ++m, srp = srp->nextrp) { +list_for_each_entry (srp, &fp->rq_list, entry) { hp = &srp->header; new_interface = (hp->interface_id =='\0' ) ?0 :1 ;if (srp->res_used) { -if (new_interface && +if (new_interface && (SG_FLAG_MMAP_IO & hp->flags)) cp =" mmap>> " ;else
@@ -2659,7 +2634,7 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)seq_printf (s,"ms sgat=%d op=0x%02x\n" , usg, (int ) srp->data.cmd_opcode); } -if (0 == m) +if (list_empty (&fp->rq_list))seq_puts (s," No requests active\n" );read_unlock (&fp->rq_list_lock); }
Signature · 1 line
1.8.5.6
Re: [PATCHv2 5/6] sg: use standard lists for sg_requests
Quoted 5 lines · Hannes Reinecke
'
Sg_request' is using a private list implementation; convert it to standard lists.Signed-off-by Hannes Reinecke <[email protected]>---
[..]
drivers/scsi/sg.c static int sg_allow_access(struct file *filp, unsigned char *cmd) · line 757
Quoted 5 lines
static bool sg_is_valid_dxfer(sg_io_hdr_t *hp) {drivers/scsi/sg.c +1
-1-switch (dxfer_direction) { +switch (hp->dxfer_direction) {
And here it is
PATCH v2 6/6 sg: close race condition in sg_remove_sfp_usercontext()
In reply to Hannes Reineckesg_remove_sfp_usercontext() is clearing any sg requests, but needs to take 'rq_list_lock' when modifying the list.
--- drivers/scsi/sg.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/scsi/sg.c +10 -2
@@ -524,6 +524,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd) }else count = (old_hdr->result ==0 ) ?0 : -EIO ;sg_finish_rem_req (srp); +sg_remove_request (sfp, srp); retval = count; free_old_hdr:kfree (old_hdr);
@@ -564,6 +565,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd) } err_out: err2 =sg_finish_rem_req (srp); +sg_remove_request (sfp, srp);return err ? : err2 ? : count; }
@@ -803,6 +805,7 @@ static bool sg_is_valid_dxfer(sg_io_hdr_t *hp)SCSI_LOG_TIMEOUT (1 ,sg_printk (KERN_INFO , sfp->parentdp,"sg_common_write: start_req err=%d\n" , k));sg_finish_rem_req (srp); +sg_remove_request (sfp, srp);return k;/* probably out of space --> ENOMEM */ }if (atomic_read (&sdp->detaching)) {
@@ -815,6 +818,7 @@ static bool sg_is_valid_dxfer(sg_io_hdr_t *hp) }sg_finish_rem_req (srp); +sg_remove_request (sfp, srp);return -ENODEV ; }
@@ -1291,6 +1295,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonstruct sg_fd *sfp = srp->parentfp;sg_finish_rem_req (srp); +sg_remove_request (sfp, srp);kref_put (&sfp->f_ref, sg_remove_sfp); }
@@ -1825,8 +1830,6 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonelse sg_remove_scat (sfp, req_schp); -sg_remove_request (sfp, srp); -return ret; }
@@ -2171,12 +2174,17 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon struct sg_fd *sfp = container_of(work, struct sg_fd, ew.work);struct sg_device *sdp = sfp->parentdp;Sg_request *srp; +unsigned long iflags;/* Cleanup any responses which were never read(). */ +write_lock_irqsave (&sfp->rq_list_lock, iflags);while (!list_empty (&sfp->rq_list)) { srp =list_first_entry (&sfp->rq_list, Sg_request, entry);sg_finish_rem_req (srp); +list_del (&srp->entry); + srp->parentfp =NULL ; } +write_unlock_irqrestore (&sfp->rq_list_lock, iflags);if (sfp->reserve.bufflen >0 ) { SCSI_LOG_TIMEOUT(6, sg_printk(KERN_INFO, sdp,
Signature · 1 line
1.8.5.6
Quoted 23 lines · Hannes Reinecke
Hi all,
the infamous syzkaller incovered some more issues in the sg driver. This patchset fixes those two issues (and adds a fix for yet another potential issue; checking for a NULL dxferp when
dxfer_lenis not 0). It also removes handling of theSET_FORCE_LOW_DMAioctl, which never worked since the initial git checkin. And does some code cleanup by removing the private list implementation, using standard lists instead.As usual, comments and reviews are welcome.
Changes to v1:
- Include reviews from Christoph
- Add patch to close race condition insg_remove_sfp_usercontext()
- Remove stale variable 'save_scat_len'Hannes Reinecke (5):
sg: disableSET_FORCE_LOW_DMA
sg: remove 'save_scat_len'
sg: protect accesses to 'reserved' page array
sg: use standard lists forsg_requests
sg: close race condition insg_remove_sfp_usercontext()
Quoted 7 lines
Johannes Thumshirn (1):
sg: check for valid direction before starting the requestdrivers/scsi/sg.c | 284 +++++++++++++++++++++++++++--------------------------- include/scsi/sg.h | 1 - 2 files changed, 141 insertions(+), 144 deletions(-)
For the whole series
(sg_inq not broken, sg_turs not broken, syzcaller bug on fixed and syzcaller use-after-free fixed, no additional messages in dmesg with a KASAN and LOCKDEP enabled kernel)
Signature · 6 lines
Johannes Thumshirn Storage [email protected] +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850