SCSI fixes for 4.15-rc3
Subsystems (5)
James Bottomley
maintainerThe most important one is the bfa fix because it's easy to oops the kernel with this driver, a regression in the new timespec conversion in aacraid and a regression in the Fibre Channel ELS handling patch. The other three are a theoretical problem with termination in the vendor/host matching code and a use after free in lpfc.
The patch is available here:
git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-fixes
The short changelog is:
Arnd Bergmann (1):
scsi: aacraid: address UBSAN warning regression
Dan Carpenter (1):
scsi: lpfc: Use after free in lpfc_rq_buf_free()
Johannes Thumshirn (1):
scsi: bfa: fix access to bfad_im_port_s
Martin Wilck (3):
scsi: scsi_devinfo: cleanly zero-pad devinfo strings
scsi: scsi_devinfo: handle non-terminated strings
scsi: libfc: fix ELS request handling
With the diffstat:
drivers/scsi/aacraid/commsup.c | 8 ++++++-- drivers/scsi/bfa/bfad_bsg.c | 6 ++++-- drivers/scsi/libfc/fc_lport.c | 4 ++++ drivers/scsi/lpfc/lpfc_mem.c | 2 +- drivers/scsi/scsi_devinfo.c | 27 ++++++++++-----------------
5 files changed, 25 insertions(+), 22 deletions(-)
And full diff below
James
---
drivers/scsi/aacraid/commsup.c +6 -2
@@ -2482,8 +2482,8 @@ int aac_command_thread(void *data)/* Synchronize our watches */ if (((NSEC_PER_SEC - (NSEC_PER_SEC /HZ )) > now.tv_nsec) && (now.tv_nsec > (NSEC_PER_SEC /HZ ))) - difference = (((NSEC_PER_SEC - now.tv_nsec) *HZ ) - +NSEC_PER_SEC /2 ) /NSEC_PER_SEC ; + difference =HZ +HZ /2 - + now.tv_nsec / (NSEC_PER_SEC /HZ );else {if (now.tv_nsec >NSEC_PER_SEC /2 ) ++now.tv_sec;
@@ -2507,6 +2507,10 @@ int aac_command_thread(void *data)if (kthread_should_stop ())break ; +/* +* we probably want usleep_range() here instead of the +* jiffies computation +*/ schedule_timeout (difference);if (kthread_should_stop ())
drivers/scsi/bfa/bfad_bsg.c +4 -2
@@ -3135,7 +3135,8 @@ bfad_im_bsg_vendor_request(struct bsg_job *job)struct fc_bsg_request *bsg_request = job->request;struct fc_bsg_reply *bsg_reply = job->reply;uint32_t vendor_cmd = bsg_request->rqst_data.h_vendor.vendor_cmd[0 ]; -struct bfad_im_port_s *im_port =shost_priv (fc_bsg_to_shost (job)); +struct Scsi_Host *shost =fc_bsg_to_shost (job); +struct bfad_im_port_s *im_port = shost->hostdata[0 ];struct bfad_s *bfad = im_port->bfad;void *payload_kbuf;int rc = -EINVAL ;
@@ -3350,7 +3351,8 @@ int bfad_im_bsg_els_ct_request(struct bsg_job *job) {struct bfa_bsg_data *bsg_data; -struct bfad_im_port_s *im_port =shost_priv (fc_bsg_to_shost (job)); +struct Scsi_Host *shost =fc_bsg_to_shost (job); +struct bfad_im_port_s *im_port = shost->hostdata[0 ];struct bfad_s *bfad = im_port->bfad;bfa_bsg_fcpt_t *bsg_fcpt;struct bfad_fcxp *drv_fcxp;
drivers/scsi/libfc/fc_lport.c +4 -0
@@ -904,10 +904,14 @@ static void fc_lport_recv_els_req(struct fc_lport *lport,case ELS_FLOGI :if (!lport->point_to_multipoint)fc_lport_recv_flogi_req (lport, fp); +else +fc_rport_recv_req (lport, fp);break ;case ELS_LOGO :if (fc_frame_sid (fp) ==FC_FID_FLOGI )fc_lport_recv_logo_req (lport, fp); +else +fc_rport_recv_req (lport, fp);break ;case ELS_RSCN : lport->tt.disc_recv_req (lport, fp);
drivers/scsi/lpfc/lpfc_mem.c +1 -1
@@ -753,12 +753,12 @@ lpfc_rq_buf_free(struct lpfc_hba *phba, struct lpfc_dmabuf *mp) drqe.address_hi =putPaddrHigh (rqb_entry->dbuf.phys); rc =lpfc_sli4_rq_put (rqb_entry->hrq, rqb_entry->drq, &hrqe, &drqe);if (rc <0 ) { - (rqbp->rqb_free_buffer)(phba, rqb_entry);lpfc_printf_log (phba,KERN_ERR ,LOG_INIT ,"6409 Cannot post to RQ %d: %x %x\n" , rqb_entry->hrq->queue_id, rqb_entry->hrq->host_index, rqb_entry->hrq->hba_index); + (rqbp->rqb_free_buffer)(phba, rqb_entry); }else {list_add_tail (&rqb_entry->hbuf.list, &rqbp->rqb_buffer_list); rqbp->buffer_count++;
drivers/scsi/scsi_devinfo.c +10 -17
@@ -34,7 +34,6 @@ struct scsi_dev_info_list_table { }; -static const char spaces[] =" " ;/* 16 of them */ static blist_flags_t scsi_default_dev_flags;static LIST_HEAD (scsi_dev_info_list);static char scsi_dev_flags[256 ];
@@ -298,20 +297,13 @@ static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,size_t from_length; from_length =strlen (from); -strncpy (to, from,min (to_length, from_length)); -if (from_length < to_length) { -if (compatible) { -/* -* NUL terminate the string if it is short. -*/ - to[from_length] ='\0' ; - }else { -/* -* space pad the string if it is short. -*/ -strncpy (&to[from_length], spaces, - to_length - from_length); - } +/* This zero-pads the destination */ +strncpy (to, from, to_length); +if (from_length < to_length && !compatible) { +/* +* space pad the string if it is short. +*/ +memset (&to[from_length],' ' , to_length - from_length); }if (from_length > to_length) printk(KERN_WARNING "%s: %s string '%s' is too long\n",
@@ -458,7 +450,8 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,/* * vendor strings must be an exact match */ -if (vmax !=strlen (devinfo->vendor) || +if (vmax !=strnlen (devinfo->vendor, +sizeof (devinfo->vendor)) ||memcmp (devinfo->vendor, vskip, vmax))continue ;
@@ -466,7 +459,7 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,* @model specifies the full string, and * must be larger or equal to devinfo->model */ - mlen =strlen (devinfo->model); + mlen =strnlen (devinfo->model,sizeof (devinfo->model));if (mmax < mlen ||memcmp (devinfo->model, mskip, mlen))continue ;return devinfo;
Quoted 6 lines · James Bottomley
The most important one is the bfa fix because it's easy to oops the kernel with this driver, a regression in the new timespec conversion in aacraid and a regression in the Fibre Channel ELS handling patch. The other three are a theoretical problem with termination in the vendor/host matching code and a use after free in lpfc.
No, this is just complete garbage.
Quoted 2 lines
Johannes Thumshirn (1):
scsi: bfa: fix access tobfad_im_port_s
This is utter shite, and doesn't even compile cleanly.
Sure, it's "just" a warning, and the code works. But no, I'm not pulling crap like this. If you save a pointer in an integer "hostdata[0]" field, then you damn well do the proper casts or helper functions or something, you don't just ignore the compiler when it very reasonably warns about it.
What the hell is going on? Nobody compiled this stuff at all? Or nobody cares about new build warnings?
That is not acceptable.
Linus
Linus,
Quoted 10 lines
This is utter shite, and doesn't even compile cleanly.
Sure, it's "just" a warning, and the code works. But no, I'm not pulling crap like this. If you save a pointer in an integer "hostdata[0]" field, then you damn well do the proper casts or helper functions or something, you don't just ignore the compiler when it very reasonably warns about it.
What the hell is going on? Nobody compiled this stuff at all? Or nobody cares about new build warnings?
Arnd and Johannes fixed this up right away:
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
James Bottomley
maintainerQuoted 12 lines · Martin K. Petersen
Linus,
Quoted 10 lines
This is utter shite, and doesn't even compile cleanly.
Sure, it's "just" a warning, and the code works. But no, I'm not pulling crap like this. If you save a pointer in an integer "hostdata[0]" field, then you damn well do the proper casts or helper
functions or something, you don't just ignore the compiler when it very reasonably warns about it.
Quoted 7 lines
Quoted 2 lines
What the hell is going on? Nobody compiled this stuff at all? Or nobody cares about new build warnings?
Arnd and Johannes fixed this up right away:
https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?
h=fixes&id=45349821ab3a8d378b8f37e52c6fe1aa1b870c47
Yes, I have that in the current testing batch, how about I push the combined thing on wednesday.
James
Quoted 2 lines · Martin K. Petersen
Arnd and Johannes fixed this up right away:
The commit you point to _is_ the probnlem. It does:
struct bfad_im_port_s *im_port = shost->hostdata[0];
which is utter bullshit crap.
Notice? It's assigning a pointer (im_port), from an integer value ("hostdata[0]" is "unsigned long").
The code is garbage.
Linus
James Bottomley
maintainerQuoted 8 lines · Linus Torvalds
Quoted 3 lines · Martin K. Petersen
Arnd and Johannes fixed this up right away:
The commit you point to _is_ the probnlem. It does:
Quoted 8 lines
struct
bfad_im_port_s*im_port= shost->hostdata[0];which is utter bullshit crap.
Notice? It's assigning a pointer (
im_port), from an integer value ("hostdata[0]" is "unsigned long").The code is garbage.
he means this one:
Which properly encapsulates the reference in a function which does the correct conversions.
James
Linus,
Quoted 5 lines
The commit you point to _is_ the probnlem. It does:
struct bfad_im_port_s *im_port = shost->hostdata[0];which is utter bullshit crap.
Sorry, wrong commit:
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering