Re: [Qemu-devel] [PATCH v4] scsi: esp: check length before dma read

[Paolo Bonzini]

On 17/06/2016 06:19, Amit Shah wrote:
> Hi,
> 
> On (Wed) 15 Jun 2016 [23:06:19], P J P wrote:
>> From: Prasad J Pandit <address@hidden>
>>
>> While doing DMA read into ESP command buffer 's->cmdbuf', it could
>> write past the 's->cmdbuf' area, if it was partially filled;
>> ie. 's->cmdlen' wasn't set at the start of the buffer.
>> Check 'len' to avoid OOB access. Also increase the command buffer
>> size to 32, which is maximum when 's->do_cmd' is set.
>>
>> Reported-by: Li Qiang <address@hidden>
>> Signed-off-by: Prasad J Pandit <address@hidden>
> 
> [...]
> 
>> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
>> index 6c79527..d2c4886 100644
>> --- a/include/hw/scsi/esp.h
>> +++ b/include/hw/scsi/esp.h
>> @@ -14,6 +14,7 @@ void esp_init(hwaddr espaddr, int it_shift,
>>  
>>  #define ESP_REGS 16
>>  #define TI_BUFSZ 16
>> +#define ESP_CMDBUF_SZ 32
>>  
>>  typedef struct ESPState ESPState;
>>  
>> @@ -31,7 +32,7 @@ struct ESPState {
>>      SCSIBus bus;
>>      SCSIDevice *current_dev;
>>      SCSIRequest *current_req;
>> -    uint8_t cmdbuf[TI_BUFSZ];
>> +    uint8_t cmdbuf[ESP_CMDBUF_SZ];
> 
> This was flagged as an incompatibility in the vmstate by a nightly run
> of the vmstate checker:
> 
> Section "esp" Description "esp" Field "cmdbuf" size mismatch: 16 , 32
> Section "dc390" Description "esp" Field "cmdbuf" size mismatch: 16 , 32
> Section "am53c974" Description "esp" Field "cmdbuf" size mismatch: 16 , 32

Oh, good catch.  But from reading the spec the implementation was
completely busted.  Let's just drop handle_satn_stop, cmdbuf, cmdlen and
do_cmd.

Paolo