Re: [Qemu-devel] [PATCH 2/2] ide: fix crash in IDE cdrom read

[Denis V. Lunev]

On 11/28/2017 07:56 PM, Kevin Wolf wrote:
> Am 28.11.2017 um 13:10 hat Denis V. Lunev geschrieben:
>> There is the following crash reported from the field in QEMU 2.9:
>>     bdrv_inc_in_flight (address@hidden)
>>     blk_aio_prwv
>>     blk_aio_preadv
>>     ide_buffered_readv
>>     cd_read_sector
>>     ide_data_readw
>>     portio_read
>>     memory_region_read_accessor
>>     access_with_adjusted_size
>>     memory_region_dispatch_read1
>>     memory_region_dispatch_read
>>     address_space_read_continue
>>     address_space_read_full
>>     address_space_read
>>     address_space_rw
>>     kvm_handle_io
>>     kvm_cpu_exec
>>     qemu_kvm_cpu_thread_fn
>>     start_thread
>>     clone
>> Indeed, the CDROM device without media has blk->bs == NULL. We should
>> check that the media is really available for the device like has been done
>> in SCSI code.
>>
>> May be the patch adds a bit more check than necessary, but this is not be
>> the problem. We should always stay on the safe side.
>>
>> Signed-off-by: Denis V. Lunev <address@hidden>
>> CC: John Snow <address@hidden>
>> CC: Kevin Wolf <address@hidden>
>> CC: Stefan Hajnoczi <address@hidden>
>> ---
>>  hw/ide/atapi.c | 32 ++++++++++++++++++++++++++++----
>>  hw/ide/core.c  |  4 ++--
>>  2 files changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index c0509c8bf5..fa50c0ccf6 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -119,6 +119,11 @@ cd_read_sector_sync(IDEState *s)
>>  
>>      trace_cd_read_sector_sync(s->lba);
>>  
>> +    if (!blk_is_available(s->blk)) {
>> +        ret = -ENOMEDIUM;
>> +        goto fail;
>> +    }
>> +
>>      switch (s->cd_sector_size) {
>>      case 2048:
>>          ret = blk_pread(s->blk, (int64_t)s->lba << ATAPI_SECTOR_BITS,
>> @@ -132,8 +137,8 @@ cd_read_sector_sync(IDEState *s)
>>          }
>>          break;
>>      default:
>> -        block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_READ);
>> -        return -EIO;
>> +        ret = -EIO;
>> +        goto fail;
>>      }
>>  
>>      if (ret < 0) {
>> @@ -145,6 +150,10 @@ cd_read_sector_sync(IDEState *s)
>>      }
>>  
>>      return ret;
>> +
>> +fail:
>> +    block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_READ);
>> +    return ret;
>>  }
> I'm not sure if we can do anything about blk_aio_* in the short time
> that we have until 2.11, so maybe we need to fix some callers like IDE
> (we won't catch all of them anyway), but at least the synchronous one
> should be easily handled in blk_prwv() by returning -ENOMEDIUM before we
> access blk_bs(blk).
>
> AIO is trickier because we need to schedule a BH, and blk_drain() can't
> execute that BH yet unless we increase bs->in_flight - which we
> obviously can't do for a NULL bs->in_flight. The proper solution
> involves a separate blk->in_flight counter for the BB and a blk_drain()
> implementation that considers it.
>
> Kevin
I have double checked that SCSI code is correct.
AHCI comes through IDE thus with this patch applied
we will be safe at emulation layer.

Den