lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jLzNNejH1MEH3ct0hSyOtG1zk8=gE4Qpd2yDvQTSahL+w@mail.gmail.com>
Date:   Wed, 23 May 2018 14:17:14 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Martin K. Petersen" <martin.petersen@...cle.com>
Cc:     Jens Axboe <axboe@...nel.dk>,
        Christoph Hellwig <hch@...radead.org>,
        James Bottomley <James.Bottomley@...senpartnership.com>,
        Tejun Heo <tj@...nel.org>, Borislav Petkov <bp@...en8.de>,
        "David S. Miller" <davem@...emloft.net>,
        "Manoj N. Kumar" <manoj@...ux.vnet.ibm.com>,
        "Matthew R. Ochs" <mrochs@...ux.vnet.ibm.com>,
        Uma Krishnan <ukrishn@...ux.vnet.ibm.com>,
        linux-block <linux-block@...r.kernel.org>,
        linux-ide@...r.kernel.org, linux-scsi@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

On Wed, May 23, 2018 at 2:06 PM, Martin K. Petersen
<martin.petersen@...cle.com> 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:

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))

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

-- 
Kees Cook
Pixel Security

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ