[3/6] block: Create scsi_sense.h for SCSI and ATAPI

Commit Message

Kees Cook May 23, 2018, 9:17 p.m. UTC

On Wed, May 23, 2018 at 2:06 PM, Martin K. Petersen
<[e-mail]> wrote:
>
> Kees,
>
>> obj-$(CONFIG_SCSI)              += scsi/
>>
>> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
>> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
>> still need to move the code from drivers/scsi/ to block/. Is this
>> okay?
>
> The reason this sucks is that scsi_normalize_sense() is an inherent core
> feature in the SCSI layer. Dealing with sense data for ioctls is just a
> fringe use case.

True, though I'm finding other robustness issues in the CDROM code.
They're probably all insane corner cases, but it seems like it'd be
nice to just fix them:

                if (blk_rq_unmap_user(bio))

This code wasn't checking req->sense_len, for example. It'll just get
stale data at worst case, but it's still ugly, especially since we
have a solution to do it right.

> I don't want to get too hung up on what goes where. But architecturally
> it really irks me to move a core piece of SCSI state machine
> functionality out of the subsystem to accommodate ioctl handling.

It looks like there is more in block/scsi_ioctl.c than just ioctl
handling, which is why I put the function in there originally.
Honestly, it's almost so small I could make it a static inline. :P

> I'm traveling today so I probably won't get a chance to look closely
> until tomorrow morning.

No worries; thanks for looking at it!

-Kees

Comments

Christoph Hellwig May 24, 2018, 7:36 a.m. UTC | #1

On Wed, May 23, 2018 at 02:17:14PM -0700, Kees Cook wrote:
> 
> True, though I'm finding other robustness issues in the CDROM code.
> They're probably all insane corner cases, but it seems like it'd be
> nice to just fix them:
> 
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 3522d2cae1b6..7726c8618c30 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -2222,9 +2223,12 @@ static int cdrom_read_cdda_bpc(struct
> cdrom_device_info *cdi, __u8 __user *ubuf,
> 
>                 blk_execute_rq(q, cdi->disk, rq, 0);
>                 if (scsi_req(rq)->result) {
> -                       struct request_sense *s = req->sense;
> +                       struct scsi_sense_hdr sshdr;
> +
>                         ret = -EIO;
> -                       cdi->last_sense = s->sense_key;
> +                       scsi_normalize_sense(req->sense, req->sense_len,
> +                                            &sshdr);
> +                       cdi->last_sense = sshdr.sense_key;
>                 }
> 
>                 if (blk_rq_unmap_user(bio))

The proper fix here is to rewrite this function to use the the
->generic_packet cdrom_device_ops method.  Is is the only caller
going straight to the scsi passthrough requests, which is a layering
violation.

Patch

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 3522d2cae1b6..7726c8618c30 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2222,9 +2223,12 @@  static int cdrom_read_cdda_bpc(struct
cdrom_device_info *cdi, __u8 __user *ubuf,

                blk_execute_rq(q, cdi->disk, rq, 0);
                if (scsi_req(rq)->result) {
-                       struct request_sense *s = req->sense;
+                       struct scsi_sense_hdr sshdr;
+
                        ret = -EIO;
-                       cdi->last_sense = s->sense_key;
+                       scsi_normalize_sense(req->sense, req->sense_len,
+                                            &sshdr);
+                       cdi->last_sense = sshdr.sense_key;
                }