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