[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH-for-5.1 v2] hw/ide: Avoid #DIV/0! FPU exception by settin

From: John Snow
Subject: Re: [RFC PATCH-for-5.1 v2] hw/ide: Avoid #DIV/0! FPU exception by setting CD-ROM sector count
Date: Mon, 20 Jul 2020 20:35:31 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

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

   ==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:
  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)) {
+    } 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?)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]