[v2] hw/ide: Avoid #DIV/0! FPU exception by setting CD-ROM sector count

[RFC PATCH-for-5.1 v2] hw/ide: Avoid #DIV/0! FPU exception by setting CD-ROM sector count

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

libFuzzer found an undefined behavior (#DIV/0!) in ide_set_sector()
when using a CD-ROM (reproducer available on the BugLink):

  UndefinedBehaviorSanitizer:DEADLYSIGNAL
  ==12163==ERROR: UndefinedBehaviorSanitizer: FPE on unknown address 0x5616279cffdc (pc 0x5616279cffdc bp 0x7ffcdaabae90 sp 0x7ffcdaabae30 T12163)

Fix by initializing the CD-ROM number of sectors in ide_dev_initfn().

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Fixes: b2df431407 ("ide scsi: Mess with geometry only for hard disk devices")
BugLink: https://bugs.launchpad.net/qemu/+bug/1887309
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Since v1:
- Allow zero-sized drive images (not sure why we need them)
  but display a friendly message that this is unsupported

Unrelated but interesting:
http://www.physics.udel.edu/~watson/scen103/cdsoln.html
---
 hw/ide/qdev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 27ff1f7f66..005d73bdb9 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -201,6 +201,15 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
                               errp)) {
             return;
         }
+    } else {
+        uint64_t nb_sectors;
+
+        blk_get_geometry(dev->conf.blk, &nb_sectors);
+        if (!nb_sectors) {
+            warn_report("Drive image of size zero is unsupported for 'ide-cd', "
+                        "use at your own risk ¯\\_(ツ)_/¯");
+        }
+        dev->conf.secs = nb_sectors;
     }
     if (!blkconf_apply_backend_options(&dev->conf, kind == IDE_CD,
                                        kind != IDE_CD, errp)) {
-- 
2.21.3

Re: [RFC PATCH-for-5.1 v2] hw/ide: Avoid #DIV/0! FPU exception by setting CD-ROM sector count

Posted by Darren Kenny 3 years, 9 months ago

On Friday, 2020-07-17 at 15:38:47 +02, Philippe Mathieu-Daudé wrote:
> libFuzzer found an undefined behavior (#DIV/0!) in ide_set_sector()
> when using a CD-ROM (reproducer available on the BugLink):
>
>   UndefinedBehaviorSanitizer:DEADLYSIGNAL
>   ==12163==ERROR: UndefinedBehaviorSanitizer: FPE on unknown address 0x5616279cffdc (pc 0x5616279cffdc bp 0x7ffcdaabae90 sp 0x7ffcdaabae30 T12163)
>
> Fix by initializing the CD-ROM number of sectors in ide_dev_initfn().
>
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Fixes: b2df431407 ("ide scsi: Mess with geometry only for hard disk devices")
> BugLink: https://bugs.launchpad.net/qemu/+bug/1887309
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

The changes LGTM, presume the 'shrug' is OK to leave in ;)

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

> ---
> Since v1:
> - Allow zero-sized drive images (not sure why we need them)
>   but display a friendly message that this is unsupported
>
> Unrelated but interesting:
> http://www.physics.udel.edu/~watson/scen103/cdsoln.html
> ---
>  hw/ide/qdev.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 27ff1f7f66..005d73bdb9 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -201,6 +201,15 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
>                                errp)) {
>              return;
>          }
> +    } else {
> +        uint64_t nb_sectors;
> +
> +        blk_get_geometry(dev->conf.blk, &nb_sectors);
> +        if (!nb_sectors) {
> +            warn_report("Drive image of size zero is unsupported for 'ide-cd', "
> +                        "use at your own risk ¯\\_(ツ)_/¯");
> +        }
> +        dev->conf.secs = nb_sectors;
>      }
>      if (!blkconf_apply_backend_options(&dev->conf, kind == IDE_CD,
>                                         kind != IDE_CD, errp)) {
> -- 
> 2.21.3

Re: [RFC PATCH-for-5.1 v2] hw/ide: Avoid #DIV/0! FPU exception by setting CD-ROM sector count

Posted by John Snow 3 years, 9 months ago

On 7/17/20 9:38 AM, Philippe Mathieu-Daudé wrote:
> libFuzzer found an undefined behavior (#DIV/0!) in ide_set_sector()
> when using a CD-ROM (reproducer available on the BugLink):
> 
>    UndefinedBehaviorSanitizer:DEADLYSIGNAL
>    ==12163==ERROR: UndefinedBehaviorSanitizer: FPE on unknown address 0x5616279cffdc (pc 0x5616279cffdc bp 0x7ffcdaabae90 sp 0x7ffcdaabae30 T12163)
> 
> Fix by initializing the CD-ROM number of sectors in ide_dev_initfn().
> 
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Fixes: b2df431407 ("ide scsi: Mess with geometry only for hard disk devices")
> BugLink: https://bugs.launchpad.net/qemu/+bug/1887309
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Since v1:
> - Allow zero-sized drive images (not sure why we need them)
>    but display a friendly message that this is unsupported
> 
> Unrelated but interesting:
> http://www.physics.udel.edu/~watson/scen103/cdsoln.html
> ---
>   hw/ide/qdev.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 27ff1f7f66..005d73bdb9 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -201,6 +201,15 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
>                                 errp)) {
>               return;
>           }
> +    } else {
> +        uint64_t nb_sectors;
> +
> +        blk_get_geometry(dev->conf.blk, &nb_sectors);
> +        if (!nb_sectors) {
> +            warn_report("Drive image of size zero is unsupported for 'ide-cd', "
> +                        "use at your own risk ¯\\_(ツ)_/¯");
> +        }
> +        dev->conf.secs = nb_sectors;
>       }
>       if (!blkconf_apply_backend_options(&dev->conf, kind == IDE_CD,
>                                          kind != IDE_CD, errp)) {
> 

I think this patch might be wrong... or at least a misdirection.

ide_set_sector is a helper that takes a logical number and distills it 
back down into the appropriate underlying registers. The case it's 
falling down on is the non-LBA case, using CHS addressing.

Is CHS addressing meaningful for CDROMs? I'm gonna guess no...

I'm looking at the original fuzzer report.
I see two commands coming in, 0x35 and 0xA1.

0x35 is WRITE DMA EXT and is being issued to the second drive, which is 
the HD in this case.

0xA1 is IDENTIFY PACKET DEVICE and goes to the first drive, the CDROM in 
this case. This is usually a fairly straightforward command that makes 
512 bytes of data available via PIO read. (Why is this engaging a DMA 
callback?)

outw 0x176 0x3538
	device/head = 0x38 [0011 1000] <-- Set 2nd drive as active
	command = 0x35     [0011 0101] <-- WRITE DMA EXT
outl 0xcf8 0x80000903
outl 0xcfc 0x184275c
outb 0x376 0x2f
	control = 0x2f [0010 1111] <-- arm a device reset
outb 0x376 0x0
	control = 0x00 [0000 0000] <-- execute a device reset
outw 0x176 0xa1a4
	device/head = 0xa4 [1010 0100]  <-- Set 1st drive as active
	command = 0xa1     [1010 0001]	<-- IDENTIFY PACKET DEVICE
outl 0xcf8 0x80000920
outb 0xcfc 0xff
outb 0xf8 0x9

the device reset here clears the busy flags for *both* drives on the 
controller, but doesn't actually take any care to cancel any outstanding 
requests. This is the main bad thing, as it allows a second request to 
be issued to a different drive while the first request's BH is still out.

When we make a call to the second device, the BH returns but now has the 
wrong context / bus state, and does ... weird, incorrect stuff.

This register is the Device Control register and bit 2, IDE_CMD_RESET, 
is officially the SRST bit (Software Reset).

It's detailed what this bit should do in ATA4 section 9.3 "Software 
Reset." (It seems like a lot, actually?)

--js