diff mbox

hw/scsi-bus.c: Fix use of uninitialised variable

Message ID CAAu8pHsjVWN7t238CEWkeypbB6GEdmdAw=eWjMr9SqXUyuG=8Q@mail.gmail.com
State New
Headers show

Commit Message

Blue Swirl Aug. 13, 2011, 5:29 p.m. UTC 
On Fri, Aug 12, 2011 at 7:22 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Fri, Aug 12, 2011 at 4:49 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Don't use req before it has been initialised in scsi_req_new().
>> This fixes a compile failure due to gcc complaining about this.
>
> It fixes a crash if the warning is ignored:
> Configuration device id QEMU version 1 machine id 32
>
> Program received signal SIGSEGV, Segmentation fault.
> scsi_req_new (d=0x15e46b0, tag=0x0, lun=0x0, buf=0x7fffffffde41 "\022",
>    hba_private=<value optimized out>) at /src/qemu/hw/scsi-bus.c:375
> 375             if (req->cmd.lba != -1) {
> (gdb) bt
> #0  scsi_req_new (d=0x15e46b0, tag=0x0, lun=0x0, buf=0x7fffffffde41 "\022",
>    hba_private=<value optimized out>) at /src/qemu/hw/scsi-bus.c:375
> #1  0x000000000052c6ef in do_busid_cmd (s=0x15e2790, buf=0x0,
>    busid=<value optimized out>) at /src/qemu/hw/esp.c:247
> #2  0x000000000052cc5d in do_cmd (s=0x15e2790) at /src/qemu/hw/esp.c:270
> #3  handle_satn (s=0x15e2790) at /src/qemu/hw/esp.c:284
> #4  0x000000000052d174 in esp_mem_writeb (opaque=0x15e2790,
>    addr=<value optimized out>, val=0xc2) at /src/qemu/hw/esp.c:640
> #5  0x000000004003d1f5 in ?? ()
> #6  0x0000000001632330 in ?? ()
> #7  0x0000000001632280 in ?? ()
> #8  0x00007fffffffe180 in ?? ()
> #9  0x3d3d87e90d932400 in ?? ()
> #10 0x00007ffff7eefd00 in ?? ()
> #11 0x00000000004dc558 in tb_reset_jump_recursive2 (tb=0xffee100c)
>    at /src/qemu/exec.c:1389
> #12 tb_reset_jump_recursive (tb=0xffee100c) at /src/qemu/exec.c:1395
> #13 0x000000000040bdea in qemu_notify_event () at /src/qemu/cpus.c:616
> #14 <signal handler called>
> #15 0x00000000004de681 in cpu_sparc_exec (env=0x1059600)
>    at /src/qemu/cpu-exec.c:528
> #16 0x000000000040c1fc in tcg_cpu_exec () at /src/qemu/cpus.c:1064
> #17 cpu_exec_all () at /src/qemu/cpus.c:1105
> #18 0x0000000000519497 in main_loop (argc=<value optimized out>,
>    argv=<value optimized out>, envp=<value optimized out>)
>    at /src/qemu/vl.c:1392
> #19 main (argc=<value optimized out>, argv=<value optimized out>,
>    envp=<value optimized out>) at /src/qemu/vl.c:3356
> (gdb) p req
> $1 = <value optimized out>
> (gdb) p req->cmd
> Cannot access memory at address 0x28
> (gdb) p req->cmd.lba
> Cannot access memory at address 0x48
>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/scsi-bus.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
>> index f2af6cd..559d5a4 100644
>> --- a/hw/scsi-bus.c
>> +++ b/hw/scsi-bus.c
>> @@ -372,7 +372,7 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
>>     } else {
>>         trace_scsi_req_parsed(d->id, lun, tag, buf[0],
>>                               cmd.mode, cmd.xfer);
>> -        if (req->cmd.lba != -1) {
>> +        if (cmd.lba != -1) {
>>             trace_scsi_req_parsed_lba(d->id, lun, tag, buf[0],
>>                                       cmd.lba);
>>         }

Something is very wrong with SCSI. OpenBIOS can't boot anymore:
Configuration device id QEMU version 1 machine id 32
CPUs: 1 x FMI,MB86904
UUID: 00000000-0000-0000-0000-000000000000
Welcome to OpenBIOS v1.0 built on Jul 20 2011 21:16
  Type 'help' for detailed information
Trying cdrom:d...
Unhandled Exception 0x0000002a
PC = 0xffd10bdc NPC = 0xffd10be0
Stopping execution

This is due to division by zero in OpenBIOS drivers/esp.c. Bisecting
reveals that this is due to c7b488721d6aafe32994ac63f8d690ae6d4729fa,
SCSI devices now report Unit Attention status after reset. OpenBIOS
does not handle this case and fails (block size is 0).

First OpenBIOS issues Inquiry command, then if a device is present,
Read Capacity. I tried adding Request Sense command after Inquiry, but
then QEMU crashes:

Configuration device id QEMU version 1 machine id 32
Initializing SCSI...dma1: Revision 2
ESP at 0xffdaa230, buffer va 0xffdab000 dva 0xfe000000
done
Initializing SCSI devices...do_command: id 0, cmd[0] 0x80, status 0x80
do_command: id 1, cmd[0] 0x80, status 0x80
do_command: id 2, cmd[0] 0x80, status 0x91
do_command_reply: status 0x93
do_command: id 2, cmd[0] 0x80, status 0x93
request_sense id 2 failed
request_sense id 2 sense key 128
do_command: id 2, cmd[0] 0x80, status 0x91
do_command_reply: status 0x93
read_capacity id 2 bs 2048 sectors 1295228
do_command: id 2, cmd[0] 0x80, status 0x91
*** glibc detected ***
/src/qemu/obj-amd64/sparc-softmmu/qemu-system-sparc: double free or
corruption (out): 0x00000000016407e0 ***
 }

@@ -224,6 +227,26 @@ inquiry(esp_private_t *esp, sd_private_t *sd)
 }


+static unsigned int
+request_sense(esp_private_t *esp, sd_private_t *sd)
+{
+    /* Setup command = Request Sense */
+    memset(esp->buffer, 0, 6);
+    esp->buffer[0] = 0x80;
+    esp->buffer[1] = REQUEST_SENSE;
+
+    esp->buffer[5] = 252;
+
+    if (do_command(esp, sd, 7, 252)) {
+        DPRINTF("request_sense id %d failed\n", sd->id);
+    DPRINTF("request_sense id %d sense key %d\n", sd->id, esp->buffer[0]);
+        return 0;
+    }
+
+    DPRINTF("request_sense id %d sense key %d\n", sd->id, esp->buffer[0]);
+    return 1;
+}
+
 static void
 ob_sd_read_blocks(sd_private_t **sd)
 {
@@ -478,7 +501,9 @@ ob_esp_init(unsigned int slot, uint64_t base,
unsigned long espoffset,
         esp->sd[id].id = id;
         if (!inquiry(esp, &esp->sd[id]))
             continue;
+        request_sense(esp, &esp->sd[id]);
         read_capacity(esp, &esp->sd[id]);
+        request_sense(esp, &esp->sd[id]);

 #ifdef CONFIG_DEBUG_ESP
         dump_drive(&esp->sd[id]);

Comments

Paolo Bonzini Aug. 14, 2011, 5:32 p.m. UTC | #1 
On 08/13/2011 07:29 PM, Blue Swirl wrote:
> On Fri, Aug 12, 2011 at 7:22 PM, Blue Swirl<blauwirbel@gmail.com>  wrote:
>> On Fri, Aug 12, 2011 at 4:49 PM, Peter Maydell<peter.maydell@linaro.org>  wrote:
>>> Don't use req before it has been initialised in scsi_req_new().
>>> This fixes a compile failure due to gcc complaining about this.
>>
>> It fixes a crash if the warning is ignored:
>> Configuration device id QEMU version 1 machine id 32

Please apply it.

> This is due to division by zero in OpenBIOS drivers/esp.c. Bisecting
> reveals that this is due to c7b488721d6aafe32994ac63f8d690ae6d4729fa,
> SCSI devices now report Unit Attention status after reset. OpenBIOS
> does not handle this case and fails (block size is 0).
>
> First OpenBIOS issues Inquiry command, then if a device is present,
> Read Capacity. I tried adding Request Sense command after Inquiry, but
> then QEMU crashes:

Thanks, I'll look into this.  However, not that Inquiry will not report 
unit attention.  The right fix is to send a Test Unit Ready after 
Inquiry and until it passes (and fail after 3/4 tries).

Paolo
diff mbox

Patch

diff --git a/drivers/esp.c b/drivers/esp.c
index 78478f6..2766e87 100644
--- a/drivers/esp.c
+++ b/drivers/esp.c
@@ -28,6 +28,7 @@ 
 
 #define BUFSIZE         4096
 
+#define CONFIG_DEBUG_ESP
 #ifdef CONFIG_DEBUG_ESP
 #define DPRINTF(fmt, args...)                   \
     do { printk(fmt , ##args); } while (0)
@@ -176,12 +177,14 @@  read_capacity(esp_private_t *esp, sd_private_t *sd)
     if (do_command(esp, sd, 11, 8)) {
         sd->sectors = 0;
         sd->bs = 0;
-
+        DPRINTF("read_capacity id %d failed\n", sd->id);
         return 0;
     }
     sd->bs = (esp->buffer[4] << 24) | (esp->buffer[5] << 16) | (esp->buffer[6] << 8) | esp->buffer[7];
     sd->sectors = ((esp->buffer[0] << 24) | (esp->buffer[1] << 16) | (esp->buffer[2] << 8) | esp->buffer[3]) * (sd->bs / 512);
 
+    DPRINTF("read_capacity id %d bs %d sectors %d\n", sd->id, sd->bs,
+            sd->sectors);
     return 1;
 }
 
@@ -224,6 +227,26 @@  inquiry(esp_private_t *esp, sd_private_t *sd)
 }
 
 
+static unsigned int
+request_sense(esp_private_t *esp, sd_private_t *sd)
+{
+    /* Setup command = Request Sense */
+    memset(esp->buffer, 0, 6);
+    esp->buffer[0] = 0x80;
+    esp->buffer[1] = REQUEST_SENSE;
+
+    esp->buffer[5] = 252;
+
+    if (do_command(esp, sd, 7, 252)) {
+        DPRINTF("request_sense id %d failed\n", sd->id);
+    DPRINTF("request_sense id %d sense key %d\n", sd->id, esp->buffer[0]);
+        return 0;
+    }
+
+    DPRINTF("request_sense id %d sense key %d\n", sd->id, esp->buffer[0]);
+    return 1;
+}
+
 static void
 ob_sd_read_blocks(sd_private_t **sd)
 {
@@ -478,7 +501,9 @@  ob_esp_init(unsigned int slot, uint64_t base, unsigned long espoffset,
         esp->sd[id].id = id;
         if (!inquiry(esp, &esp->sd[id]))
             continue;
+        request_sense(esp, &esp->sd[id]);
         read_capacity(esp, &esp->sd[id]);
+        request_sense(esp, &esp->sd[id]);
 
 #ifdef CONFIG_DEBUG_ESP
         dump_drive(&esp->sd[id]);