| Message ID | 1516638634.2545.0.camel@wdc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 2018-01-22 11:30 AM, Bart Van Assche wrote: > On Mon, 2018-01-22 at 12:06 +0100, Dmitry Vyukov wrote: >> general protection fault: 0000 [#1] SMP KASAN > > How about the untested patch below? > > Thanks, > > Bart. > > > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c > index cd9b6ebd7257..04a644b39d79 100644 > --- a/drivers/scsi/sg.c > +++ b/drivers/scsi/sg.c > @@ -627,6 +627,10 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos) > mutex_unlock(&sfp->f_mutex); > SCSI_LOG_TIMEOUT(4, sg_printk(KERN_INFO, sdp, > "sg_write: scsi opcode=0x%02x, cmd_size=%d\n", (int) opcode, cmd_size)); > + if (cmd_size > sizeof(cmnd)) { > + sg_remove_request(sfp, srp); > + return -EFAULT; > + } > /* Determine buffer size. */ > input_size = count - cmd_size; > mxsize = max(input_size, old_hdr.reply_len); > Using 'scsi_logging_level -s -T 5' on the sg driver and running the test program provided, the cmd_size is 9, just like the ioctl() in his program set it to. The sizeof(cmnd) is 252. So I don't know what caused the GPF but it wasn't cmd_size being out of bounds. As for the above patch, did you notice this check in that function: if ((!hp->cmdp) || (hp->cmd_len < 6) || (hp->cmd_len > sizeof (cmnd))) { sg_remove_request(sfp, srp); return -EMSGSIZE; } As far as I remember, Dmitry has not indicated in multiple reports over several years what /dev/sg0 is. Perhaps it misbehaves when it gets a SCSI command in the T10 range (i.e. not vendor specific) with a 9 byte cdb length. As far as I'm aware T10 (and the Ansi committee before it) have never defined a cdb with an odd length. For those that are not aware, the sg driver is a relatively thin shim over the block layer, the SCSI mid-level, and a low-level driver which may have another kernel driver stack underneath it (e.g. UAS (USB attached SCSI)). The previous report from syzkaller on the sg driver ("scsi: memory leak in sg_start_req") has resulted in one accepted patch on the block layer with probably more to come in the same area. Testing the patch Dmitry gave (with some added error checks which reported no problems) with the scsi_debug driver supplying /dev/sg0 I have not seen any problems running that test program. Again there might be a very slow memory leak, but if there is I don't believe it is in the sg driver. While it's not invalid from a testing perspective, throwing total nonsense at a pass-through mechanism, including absurd SCSI commands at best will test error paths, but only at a very shallow level. Setting up almost valid pass-through scenarios will test error paths at a deeper level. Then there are lots of valid pass-through scenarios that would be expected not to fail. Doug Gilbert
On Mon, Jan 22, 2018 at 7:57 PM, Douglas Gilbert <dgilbert@interlog.com> wrote: > On 2018-01-22 11:30 AM, Bart Van Assche wrote: >> >> On Mon, 2018-01-22 at 12:06 +0100, Dmitry Vyukov wrote: >>> >>> general protection fault: 0000 [#1] SMP KASAN >> >> >> How about the untested patch below? >> >> Thanks, >> >> Bart. >> >> >> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >> index cd9b6ebd7257..04a644b39d79 100644 >> --- a/drivers/scsi/sg.c >> +++ b/drivers/scsi/sg.c >> @@ -627,6 +627,10 @@ sg_write(struct file *filp, const char __user *buf, >> size_t count, loff_t * ppos) >> mutex_unlock(&sfp->f_mutex); >> SCSI_LOG_TIMEOUT(4, sg_printk(KERN_INFO, sdp, >> "sg_write: scsi opcode=0x%02x, cmd_size=%d\n", (int) >> opcode, cmd_size)); >> + if (cmd_size > sizeof(cmnd)) { >> + sg_remove_request(sfp, srp); >> + return -EFAULT; >> + } >> /* Determine buffer size. */ >> input_size = count - cmd_size; >> mxsize = max(input_size, old_hdr.reply_len); >> > > Using 'scsi_logging_level -s -T 5' on the sg driver and running the test > program provided, the cmd_size is 9, just like the ioctl() in his program > set it to. The sizeof(cmnd) is 252. So I don't know what caused the > GPF but it wasn't cmd_size being out of bounds. > > As for the above patch, did you notice this check in that function: > > if ((!hp->cmdp) || (hp->cmd_len < 6) || (hp->cmd_len > sizeof > (cmnd))) { > sg_remove_request(sfp, srp); > return -EMSGSIZE; > } > > As far as I remember, Dmitry has not indicated in multiple reports > over several years what /dev/sg0 is. That's because I know nothing about sg. If you give a command to run, I will provide it's output. > Perhaps it misbehaves when it > gets a SCSI command in the T10 range (i.e. not vendor specific) with > a 9 byte cdb length. As far as I'm aware T10 (and the Ansi committee > before it) have never defined a cdb with an odd length. > > For those that are not aware, the sg driver is a relatively thin > shim over the block layer, the SCSI mid-level, and a low-level > driver which may have another kernel driver stack underneath it > (e.g. UAS (USB attached SCSI)). The previous report from syzkaller > on the sg driver ("scsi: memory leak in sg_start_req") has resulted > in one accepted patch on the block layer with probably more to > come in the same area. > > Testing the patch Dmitry gave (with some added error checks which > reported no problems) with the scsi_debug driver supplying /dev/sg0 > I have not seen any problems running that test program. Again > there might be a very slow memory leak, but if there is I don't > believe it is in the sg driver. Did you run it in a loop? First runs pass just fine for me too. > While it's not invalid from a testing perspective, throwing total > nonsense at a pass-through mechanism, including absurd SCSI commands > at best will test error paths, but only at a very shallow level. > Setting up almost valid pass-through scenarios will test error > paths at a deeper level. Then there are lots of valid pass-through > scenarios that would be expected not to fail. Agree. syzkaller can test very elaborate scenarios, but it needs help for this (telling what are these "almost valid" inputs). Kernel has hundreds of APIs, some of them are quite elaborate and require expertise to use (and undocumented), we can't describe all of them. Frequently after adding a proper description, syzkaller finds a dozen or two of bugs in the subsystem.
On Mon, 2018-01-22 at 20:06 +0100, Dmitry Vyukov wrote: > On Mon, Jan 22, 2018 at 7:57 PM, Douglas Gilbert <dgilbert@interlog.com> wrote: > > As far as I remember, Dmitry has not indicated in multiple reports > > over several years what /dev/sg0 is. > > That's because I know nothing about sg. If you give a command to run, > I will provide it's output. /dev/sg0 refers to a SCSI device. We would like to know the name of the SCSI LLD driver. Would it be possible to provide the output of the following commands: readlink /sys/class/scsi_generic/sg0 cat /sys/class/scsi_generic/sg0/device/vendor Thanks, Bart.
Uh, I've answered this a week ago, but did not notice that Doug dropped everybody from CC. Reporting to all. On Mon, Jan 22, 2018 at 8:16 PM, Douglas Gilbert <dgilbert@interlog.com> wrote: > On 2018-01-22 02:06 PM, Dmitry Vyukov wrote: >> >> On Mon, Jan 22, 2018 at 7:57 PM, Douglas Gilbert <dgilbert@interlog.com> > Please show me the output of 'lsscsi -g' on your test machine. > /dev/sg0 is often associated with /dev/sda which is often a SATA > SSD (or a virtualized one) that holds the root file system. > With the sg pass-through driver it is relatively easy to write > random (user provided data) over the root file system which will > almost certainly "root" the system. This is pretty standard qemu vm started with: qemu-system-x86_64 -hda wheezy.img -net user,host=10.0.2.10 -net nic -nographic -kernel arch/x86/boot/bzImage -append "console=ttyS0 root=/dev/sda earlyprintk=serial " -m 2G -smp 4 # lsscsi -g [0:0:0:0] disk ATA QEMU HARDDISK 0 /dev/sda /dev/sg0 [1:0:0:0] cd/dvd QEMU QEMU DVD-ROM 2.0. /dev/sr0 /dev/sg1 # readlink /sys/class/scsi_generic/sg0 ../../devices/pci0000:00/0000:00:01.1/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0 # cat /sys/class/scsi_generic/sg0/device/vendor ATA >>> Perhaps it misbehaves when it >>> gets a SCSI command in the T10 range (i.e. not vendor specific) with >>> a 9 byte cdb length. As far as I'm aware T10 (and the Ansi committee >>> before it) have never defined a cdb with an odd length. >>> >>> For those that are not aware, the sg driver is a relatively thin >>> shim over the block layer, the SCSI mid-level, and a low-level >>> driver which may have another kernel driver stack underneath it >>> (e.g. UAS (USB attached SCSI)). The previous report from syzkaller >>> on the sg driver ("scsi: memory leak in sg_start_req") has resulted >>> in one accepted patch on the block layer with probably more to >>> come in the same area. >>> >>> Testing the patch Dmitry gave (with some added error checks which >>> reported no problems) with the scsi_debug driver supplying /dev/sg0 >>> I have not seen any problems running that test program. Again >>> there might be a very slow memory leak, but if there is I don't >>> believe it is in the sg driver. >> >> >> Did you run it in a loop? First runs pass just fine for me too. > > > Is thirty minutes long enough ?? Yes, it certainly should be enough. Here is what I see: # while ./a.out; do echo RUN; done RUN RUN RUN RUN RUN RUN RUN [ 371.977266] ================================================================== [ 371.980158] BUG: KASAN: double-free or invalid-free in __put_task_struct+0x1e7/0x5c0 .... Here is full execution trace of the write call if that will be of any help: https://gist.githubusercontent.com/dvyukov/14ae64c3e753dedf9ab2608676ecf0b9/raw/9803d52bb1e317a9228e362236d042aaf0fa9d69/gistfile1.txt This is on upstream commit 0d665e7b109d512b7cae3ccef6e8654714887844. Also attaching my config just in case.
On 2018-01-30 07:22 AM, Dmitry Vyukov wrote: > Uh, I've answered this a week ago, but did not notice that Doug > dropped everybody from CC. Reporting to all. > > On Mon, Jan 22, 2018 at 8:16 PM, Douglas Gilbert <dgilbert@interlog.com> wrote: >> On 2018-01-22 02:06 PM, Dmitry Vyukov wrote: >>> >>> On Mon, Jan 22, 2018 at 7:57 PM, Douglas Gilbert <dgilbert@interlog.com> >> Please show me the output of 'lsscsi -g' on your test machine. >> /dev/sg0 is often associated with /dev/sda which is often a SATA >> SSD (or a virtualized one) that holds the root file system. >> With the sg pass-through driver it is relatively easy to write >> random (user provided data) over the root file system which will >> almost certainly "root" the system. > > > This is pretty standard qemu vm started with: > > qemu-system-x86_64 -hda wheezy.img -net user,host=10.0.2.10 -net nic > -nographic -kernel arch/x86/boot/bzImage -append "console=ttyS0 > root=/dev/sda earlyprintk=serial " -m 2G -smp 4 > > # lsscsi -g > [0:0:0:0] disk ATA QEMU HARDDISK 0 /dev/sda /dev/sg0 With lk 4.15.0-rc9 I can run your test program (with some additions, see attachment) for 30 minutes against a scsi_debug simulated disk. You can easily replicate this test just run 'modprobe scsi_debug' and a third line should appear in your lsscsi output. The new device will most likely be /dev/sg2 . With lk 4.15.0 (release) running against a SAS SSD (SEAGATE ST200FM0073), the test has been running 20 minutes and counting without problems. That is using a LSI HBA with the mpt3sas driver. > [1:0:0:0] cd/dvd QEMU QEMU DVD-ROM 2.0. /dev/sr0 /dev/sg1 > > # readlink /sys/class/scsi_generic/sg0 > ../../devices/pci0000:00/0000:00:01.1/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0 > > # cat /sys/class/scsi_generic/sg0/device/vendor > ATA ^^^^^ That subsystem is the culprit IMO, most likely libata. Until you can show this test failing on something other than an ATA disk, then I will treat this issue as closed. Doug Gilbert >>>> Perhaps it misbehaves when it >>>> gets a SCSI command in the T10 range (i.e. not vendor specific) with >>>> a 9 byte cdb length. As far as I'm aware T10 (and the Ansi committee >>>> before it) have never defined a cdb with an odd length. >>>> >>>> For those that are not aware, the sg driver is a relatively thin >>>> shim over the block layer, the SCSI mid-level, and a low-level >>>> driver which may have another kernel driver stack underneath it >>>> (e.g. UAS (USB attached SCSI)). The previous report from syzkaller >>>> on the sg driver ("scsi: memory leak in sg_start_req") has resulted >>>> in one accepted patch on the block layer with probably more to >>>> come in the same area. >>>> >>>> Testing the patch Dmitry gave (with some added error checks which >>>> reported no problems) with the scsi_debug driver supplying /dev/sg0 >>>> I have not seen any problems running that test program. Again >>>> there might be a very slow memory leak, but if there is I don't >>>> believe it is in the sg driver. >>> >>> >>> Did you run it in a loop? First runs pass just fine for me too. >> >> >> Is thirty minutes long enough ?? > > > Yes, it certainly should be enough. Here is what I see: > > > # while ./a.out; do echo RUN; done > RUN > RUN > RUN > RUN > RUN > RUN > RUN > [ 371.977266] ================================================================== > [ 371.980158] BUG: KASAN: double-free or invalid-free in > __put_task_struct+0x1e7/0x5c0 > .... > > > Here is full execution trace of the write call if that will be of any help: > https://gist.githubusercontent.com/dvyukov/14ae64c3e753dedf9ab2608676ecf0b9/raw/9803d52bb1e317a9228e362236d042aaf0fa9d69/gistfile1.txt > > This is on upstream commit 0d665e7b109d512b7cae3ccef6e8654714887844. > Also attaching my config just in case. >
On Thu, Feb 1, 2018 at 7:03 AM, Douglas Gilbert <dgilbert@interlog.com> wrote: > On 2018-01-30 07:22 AM, Dmitry Vyukov wrote: >> >> Uh, I've answered this a week ago, but did not notice that Doug >> dropped everybody from CC. Reporting to all. >> >> On Mon, Jan 22, 2018 at 8:16 PM, Douglas Gilbert <dgilbert@interlog.com> >> wrote: >>> >>> On 2018-01-22 02:06 PM, Dmitry Vyukov wrote: >>>> >>>> >>>> On Mon, Jan 22, 2018 at 7:57 PM, Douglas Gilbert <dgilbert@interlog.com> >>> >>> Please show me the output of 'lsscsi -g' on your test machine. >>> /dev/sg0 is often associated with /dev/sda which is often a SATA >>> SSD (or a virtualized one) that holds the root file system. >>> With the sg pass-through driver it is relatively easy to write >>> random (user provided data) over the root file system which will >>> almost certainly "root" the system. >> >> >> >> This is pretty standard qemu vm started with: >> >> qemu-system-x86_64 -hda wheezy.img -net user,host=10.0.2.10 -net nic >> -nographic -kernel arch/x86/boot/bzImage -append "console=ttyS0 >> root=/dev/sda earlyprintk=serial " -m 2G -smp 4 >> >> # lsscsi -g >> [0:0:0:0] disk ATA QEMU HARDDISK 0 /dev/sda /dev/sg0 > > > With lk 4.15.0-rc9 I can run your test program (with some additions, see > attachment) for 30 minutes against a scsi_debug simulated disk. You can > easily replicate this test just run 'modprobe scsi_debug' and a third > line should appear in your lsscsi output. The new device will most likely > be /dev/sg2 . > > With lk 4.15.0 (release) running against a SAS SSD (SEAGATE ST200FM0073), > the test has been running 20 minutes and counting without problems. That > is using a LSI HBA with the mpt3sas driver. > >> [1:0:0:0] cd/dvd QEMU QEMU DVD-ROM 2.0. /dev/sr0 /dev/sg1 >> >> # readlink /sys/class/scsi_generic/sg0 >> >> ../../devices/pci0000:00/0000:00:01.1/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0 >> >> # cat /sys/class/scsi_generic/sg0/device/vendor >> ATA > > > ^^^^^ > That subsystem is the culprit IMO, most likely libata. > > Until you can show this test failing on something other than an > ATA disk, then I will treat this issue as closed. Hi Doug, Why is bug in ATA not a bug? Is it long unused by everybody? I've got it by running qemu with default flags...
On Thu, 2018-02-01 at 08:04 +0100, Dmitry Vyukov wrote: > On Thu, Feb 1, 2018 at 7:03 AM, Douglas Gilbert <dgilbert@interlog.com> wrote: > > On 2018-01-30 07:22 AM, Dmitry Vyukov wrote: [...] > > > [1:0:0:0] cd/dvd QEMU QEMU DVD-ROM 2.0. /dev/sr0 /dev/sg1 > > > > > > # readlink /sys/class/scsi_generic/sg0 > > > > > > ../../devices/pci0000:00/0000:00:01.1/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0 > > > > > > # cat /sys/class/scsi_generic/sg0/device/vendor > > > ATA > > > > > > ^^^^^ > > That subsystem is the culprit IMO, most likely libata. > > > > Until you can show this test failing on something other than an > > ATA disk, then I will treat this issue as closed. > > Hi Doug, > > Why is bug in ATA not a bug? Is it long unused by everybody? I've got > it by running qemu with default flags... If the bug is in libata then it's not on Doug to fix it since he's only maintaining sg. Ben.
On Thu, Feb 1, 2018 at 5:17 PM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > On Thu, 2018-02-01 at 08:04 +0100, Dmitry Vyukov wrote: >> On Thu, Feb 1, 2018 at 7:03 AM, Douglas Gilbert <dgilbert@interlog.com> wrote: >> > On 2018-01-30 07:22 AM, Dmitry Vyukov wrote: > [...] >> > > [1:0:0:0] cd/dvd QEMU QEMU DVD-ROM 2.0. /dev/sr0 /dev/sg1 >> > > >> > > # readlink /sys/class/scsi_generic/sg0 >> > > >> > > ../../devices/pci0000:00/0000:00:01.1/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0 >> > > >> > > # cat /sys/class/scsi_generic/sg0/device/vendor >> > > ATA >> > >> > >> > ^^^^^ >> > That subsystem is the culprit IMO, most likely libata. >> > >> > Until you can show this test failing on something other than an >> > ATA disk, then I will treat this issue as closed. >> >> Hi Doug, >> >> Why is bug in ATA not a bug? Is it long unused by everybody? I've got >> it by running qemu with default flags... > > If the bug is in libata then it's not on Doug to fix it since he's only > maintaining sg. Then I think we need to CC ata maintainers rather than treat it as closed. +Tejun, linux-ide@, you can see full thread here: https://groups.google.com/forum/#!topic/syzkaller/9RNr9Gu0MyY
On Thu, Feb 01, 2018 at 05:21:12PM +0100, 'Dmitry Vyukov' via syzkaller wrote: > On Thu, Feb 1, 2018 at 5:17 PM, Ben Hutchings > <ben.hutchings@codethink.co.uk> wrote: > > On Thu, 2018-02-01 at 08:04 +0100, Dmitry Vyukov wrote: > >> On Thu, Feb 1, 2018 at 7:03 AM, Douglas Gilbert <dgilbert@interlog.com> wrote: > >> > On 2018-01-30 07:22 AM, Dmitry Vyukov wrote: > > [...] > >> > > [1:0:0:0] cd/dvd QEMU QEMU DVD-ROM 2.0. /dev/sr0 /dev/sg1 > >> > > > >> > > # readlink /sys/class/scsi_generic/sg0 > >> > > > >> > > ../../devices/pci0000:00/0000:00:01.1/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0 > >> > > > >> > > # cat /sys/class/scsi_generic/sg0/device/vendor > >> > > ATA > >> > > >> > > >> > ^^^^^ > >> > That subsystem is the culprit IMO, most likely libata. > >> > > >> > Until you can show this test failing on something other than an > >> > ATA disk, then I will treat this issue as closed. > >> > >> Hi Doug, > >> > >> Why is bug in ATA not a bug? Is it long unused by everybody? I've got > >> it by running qemu with default flags... > > > > If the bug is in libata then it's not on Doug to fix it since he's only > > maintaining sg. > > > Then I think we need to CC ata maintainers rather than treat it as closed. > +Tejun, linux-ide@, you can see full thread here: > https://groups.google.com/forum/#!topic/syzkaller/9RNr9Gu0MyY > To get memory corruption it's actually sufficient just to submit "1-byte" reads; there's no need for the SG_NEXT_CMD_LEN ioctl or anything: #include <fcntl.h> #include <unistd.h> int main() { int fd = open("/dev/sg0", O_RDWR); char buf[43] = { [36] = 0x08 /* READ_6 */ }; for (;;) write(fd, buf, sizeof(buf)); } (where /dev/sg0 is the default QEMU disk type, "82371SB PIIX3 IDE") The SCSI command descriptor block is the 6 bytes at indices 36-41, so index 42 is the only data byte. Also this is a different bug from the crash in ata_bmdma_fill_sg() which is fixed by "libata: fix length validation of ATAPI-relayed SCSI commands". I'm guessing the driver is DMA'ing to somewhere it shouldn't be... Eric
On Sun, Feb 4, 2018 at 10:07 AM, Eric Biggers <ebiggers3@gmail.com> wrote: > On Thu, Feb 01, 2018 at 05:21:12PM +0100, 'Dmitry Vyukov' via syzkaller wrote: >> On Thu, Feb 1, 2018 at 5:17 PM, Ben Hutchings >> <ben.hutchings@codethink.co.uk> wrote: >> > On Thu, 2018-02-01 at 08:04 +0100, Dmitry Vyukov wrote: >> >> On Thu, Feb 1, 2018 at 7:03 AM, Douglas Gilbert <dgilbert@interlog.com> wrote: >> >> > On 2018-01-30 07:22 AM, Dmitry Vyukov wrote: >> > [...] >> >> > > [1:0:0:0] cd/dvd QEMU QEMU DVD-ROM 2.0. /dev/sr0 /dev/sg1 >> >> > > >> >> > > # readlink /sys/class/scsi_generic/sg0 >> >> > > >> >> > > ../../devices/pci0000:00/0000:00:01.1/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0 >> >> > > >> >> > > # cat /sys/class/scsi_generic/sg0/device/vendor >> >> > > ATA >> >> > >> >> > >> >> > ^^^^^ >> >> > That subsystem is the culprit IMO, most likely libata. >> >> > >> >> > Until you can show this test failing on something other than an >> >> > ATA disk, then I will treat this issue as closed. >> >> >> >> Hi Doug, >> >> >> >> Why is bug in ATA not a bug? Is it long unused by everybody? I've got >> >> it by running qemu with default flags... >> > >> > If the bug is in libata then it's not on Doug to fix it since he's only >> > maintaining sg. >> >> >> Then I think we need to CC ata maintainers rather than treat it as closed. >> +Tejun, linux-ide@, you can see full thread here: >> https://groups.google.com/forum/#!topic/syzkaller/9RNr9Gu0MyY >> > > To get memory corruption it's actually sufficient just to submit "1-byte" reads; > there's no need for the SG_NEXT_CMD_LEN ioctl or anything: > > #include <fcntl.h> > #include <unistd.h> > > int main() > { > int fd = open("/dev/sg0", O_RDWR); > char buf[43] = { [36] = 0x08 /* READ_6 */ }; > > for (;;) > write(fd, buf, sizeof(buf)); > } > > (where /dev/sg0 is the default QEMU disk type, "82371SB PIIX3 IDE") > > The SCSI command descriptor block is the 6 bytes at indices 36-41, so index 42 > is the only data byte. > > Also this is a different bug from the crash in ata_bmdma_fill_sg() which is > fixed by "libata: fix length validation of ATAPI-relayed SCSI commands". > > I'm guessing the driver is DMA'ing to somewhere it shouldn't be... It would be good to add KASAN checks to the DMA code that issues transfers. This is another case where a silent memory corruption causes dozens of assorted crashes all over the kernel. If we add checks, KASAN would pinpoint the exact stack that issues the bad command. This may be the simplest way to debug this bug as well. I've filed https://bugzilla.kernel.org/show_bug.cgi?id=198661 for this.
On Sun, Feb 04, 2018 at 12:10:58PM +0100, Dmitry Vyukov wrote: > > > > To get memory corruption it's actually sufficient just to submit "1-byte" reads; > > there's no need for the SG_NEXT_CMD_LEN ioctl or anything: > > > > #include <fcntl.h> > > #include <unistd.h> > > > > int main() > > { > > int fd = open("/dev/sg0", O_RDWR); > > char buf[43] = { [36] = 0x08 /* READ_6 */ }; > > > > for (;;) > > write(fd, buf, sizeof(buf)); > > } > > > > (where /dev/sg0 is the default QEMU disk type, "82371SB PIIX3 IDE") > > > > The SCSI command descriptor block is the 6 bytes at indices 36-41, so index 42 > > is the only data byte. > > > > Also this is a different bug from the crash in ata_bmdma_fill_sg() which is > > fixed by "libata: fix length validation of ATAPI-relayed SCSI commands". > > > > I'm guessing the driver is DMA'ing to somewhere it shouldn't be... > > It would be good to add KASAN checks to the DMA code that issues > transfers. This is another case where a silent memory corruption > causes dozens of assorted crashes all over the kernel. If we add > checks, KASAN would pinpoint the exact stack that issues the bad > command. This may be the simplest way to debug this bug as well. I've > filed https://bugzilla.kernel.org/show_bug.cgi?id=198661 for this. It seems the problem is related to the fact that in the PRD (Physical Region Descriptor) list for the DMA transfer for "BMDMA" ATA disks, the disk (emulated in QEMU here: https://github.com/qemu/qemu/blob/master/hw/ide/pci.c#L89) ignores the low bit in the lengths, causing a length of 1 (byte) to be interpreted as 0. But, at the same time there is also a special case where a length of 0 is interpreted as 65536 bytes. So the disk will DMA up to 65536 bytes into a 1-byte buffer, causing massive memory corruption. I'm not sure what the best fix is, but probably it needs to be required that the lengths in the sglist have the alignment needed for the disk. KASAN would not have helped here unfortunately. Even if there were KASAN checks when mapping the sglist for DMA or when filling in the PRD list, the kernel would not have known that the disk would actually interpret "1" as "65536". - Eric
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c index cd9b6ebd7257..04a644b39d79 100644 --- a/drivers/scsi/sg.c +++ b/drivers/scsi/sg.c @@ -627,6 +627,10 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos) mutex_unlock(&sfp->f_mutex); SCSI_LOG_TIMEOUT(4, sg_printk(KERN_INFO, sdp, "sg_write: scsi opcode=0x%02x, cmd_size=%d\n", (int) opcode, cmd_size)); + if (cmd_size > sizeof(cmnd)) { + sg_remove_request(sfp, srp); + return -EFAULT; + } /* Determine buffer size. */ input_size = count - cmd_size; mxsize = max(input_size, old_hdr.reply_len);
On Mon, 2018-01-22 at 12:06 +0100, Dmitry Vyukov wrote: > general protection fault: 0000 [#1] SMP KASAN How about the untested patch below? Thanks, Bart.