On Wed, 2015-12-09 at 18:07 +0100, Tomas Henzl wrote: > On 8.12.2015 18:00, James Bottomley wrote: > > Simple enclosure implementations (mostly USB) are allowed to return only > > page 8 to every diagnostic query. That really confuses our > > implementation because we assume the return is the page we asked for and > > end up doing incorrect offsets based on bogus information leading to > > accesses outside of allocated ranges. Fix that by checking the page > > code of the return and giving an error if it isn't the one we asked for. > > This should fix reported bugs with USB storage by simply refusing to > > attach to enclosures that behave like this. It's also good defensive > > practise now that we're starting to see more USB enclosures. > > Ideally this patch also fixes all callers so they evaluate the return value > from ses_recv_diag. That is missed in ses_enclosure_data_process > and ses_get_page2_descriptor.
Well, it wouldn't be a bug fix and it's strictly not necessary. in ses_intf_add() we won't attach if the initial retrieve of page 2 fails. That means we already have an old copy. So if there's a failure in ses_get_page2_descriptor() then we're just working from old data. Essentially there's nothing else we could do (except perhaps log the problem). James > -tms > > > > > Reported-by: Andrea Gelmini <[email protected]> > > Cc: [email protected] > > Signed-off-by: James Bottomley <[email protected]> > > > > --- > > > > diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c > > index dcb0d76..7d9cec5 100644 > > --- a/drivers/scsi/ses.c > > +++ b/drivers/scsi/ses.c > > @@ -84,6 +84,7 @@ static void init_device_slot_control(unsigned char > > *dest_desc, > > static int ses_recv_diag(struct scsi_device *sdev, int page_code, > > void *buf, int bufflen) > > { > > + int ret; > > unsigned char cmd[] = { > > RECEIVE_DIAGNOSTIC, > > 1, /* Set PCV bit */ > > @@ -92,9 +93,26 @@ static int ses_recv_diag(struct scsi_device *sdev, int > > page_code, > > bufflen & 0xff, > > 0 > > }; > > + unsigned char recv_page_code; > > > > - return scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen, > > + ret = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen, > > NULL, SES_TIMEOUT, SES_RETRIES, NULL); > > + if (unlikely(!ret)) > > + return ret; > > + > > + recv_page_code = ((unsigned char *)buf)[0]; > > + > > + if (likely(recv_page_code == page_code)) > > + return ret; > > + > > + /* successful diagnostic but wrong page code. This happens to some > > + * USB devices, just print a message and pretend there was an error */ > > + > > + sdev_printk(KERN_ERR, sdev, > > + "Wrong diagnostic page; asked for %d got %u\n", > > + page_code, recv_page_code); > > + > > + return -EINVAL; > > } > > > > static int ses_send_diag(struct scsi_device *sdev, int page_code, > > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > > the body of a message to [email protected] > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
