On 9.12.2015 19:15, James Bottomley wrote: > 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).
You are right it is very unlikely that we read page2 with success in ses_intf_add and later page 8 is read instead in ses_get_page2_descriptor and rewrites the page 2. That means that we no longer have the old copy. Anyway its almost impossible and your patch per se is correct. Reviewed-by: Tomas Henzl <[email protected]> Tomas > > 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 -- 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
