Re: [Qemu-devel] [PATCH for-4.1] scsi-cd: Fix inserting read-only media in empty drive

[Max Reitz]

On 30.07.19 10:29, Kevin Wolf wrote:
> Am 30.07.2019 um 08:31 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>>
>>> scsi-disks decides whether it has a read-only device by looking at
>>> whether the BlockBackend specified as drive=... is read-only. In the
>>> case of an anonymous BlockBackend (with a node name specified in
>>> drive=...), this is the read-only flag of the attached node. In the case
>>> of an empty anonymous BlockBackend, it's always read-write because
>>> nothing prevented it from being read-write.
>>>
>>> This is a problem because scsi-cd would take write permissions on the
>>> anonymous BlockBackend of an empty drive created without a drive=...
>>> option. Using blockdev-insert-medium with a read-only node fails then
>>> with the error message "Block node is read-only".
>>>
>>> Fix scsi_realize() so that scsi-cd devices always take read-only
>>> permissions on their BlockBackend instead.
>>>
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1733920
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>> ---
>>>  hw/scsi/scsi-disk.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>>> index 8e95e3e38d..af3e622dc5 100644
>>> --- a/hw/scsi/scsi-disk.c
>>> +++ b/hw/scsi/scsi-disk.c
>>> @@ -2318,6 +2318,7 @@ static void 
>>> scsi_disk_unit_attention_reported(SCSIDevice *dev)
>>>  static void scsi_realize(SCSIDevice *dev, Error **errp)
>>>  {
>>>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
>>> +    bool read_only;
>>>  
>>>      if (!s->qdev.conf.blk) {
>>>          error_setg(errp, "drive property not set");
>>> @@ -2351,8 +2352,13 @@ static void scsi_realize(SCSIDevice *dev, Error 
>>> **errp)
>>>              return;
>>>          }
>>>      }
>>> -    if (!blkconf_apply_backend_options(&dev->conf,
>>> -                                       blk_is_read_only(s->qdev.conf.blk),
>>> +
>>> +    read_only = blk_is_read_only(s->qdev.conf.blk);
>>> +    if (dev->type == TYPE_ROM) {
>>> +        read_only = true;
>>> +    }
>>> +
>>> +    if (!blkconf_apply_backend_options(&dev->conf, read_only,
>>>                                         dev->type == TYPE_DISK, errp)) {
>>>          return;
>>>      }
>>
>> For what it's worth, we have code similar to the one after this patch in
>>
>> * ide_dev_initfn()
>>
>> * xen_block_realize()  (I guess)
>>
>> We have code similar to the one before this patch in
>>
>> * floppy_drive_realize()
>>
>>   I figure we avoid the problem by recomputing read-only on media
>>   change, in fd_change_cb().  Funny: looks like a medium's
>>   read-only-ness lingers after unload until the next medium is loaded.
> 
> We may try to, but it looks something is broken for floppies.
> 
> The bug only came to my attention yesterday, so I haven't got a full
> test case yet, but the half that I already have fails for floppy. I'll
> look into this, but it was more important to me to get at least the
> scsi-cd fix into 4.1.
> 
>> * nvme_realize()
>>
>> * virtio_blk_device_realize()
>>
>> * scsi_generic_realize()
>>
>> * usb_msd_storage_realize()
>>
>> Are these all okay?  Should they work more like floppy?  If not, what
>> makes floppy special?
> 
> Most of them aren't relevant in this context because this is a problem
> with removable media, and most devices don't support that. So as far as
> I know all we need to check is floppy, ATAPI and SCSI CD-ROM.
> 
> Floppy is special because it's the only read-write device that supports
> removable media (and you can insert a read-only floppy after ejecting a
> read-write one or vice versa). CDs can be simpler because they are
> always read-only.

There are also SD cards.

(The SD code just rejects read-only BBs, and it takes PERM_WRITE on it.
 So I suppose it’s good because this way you simply can never insert
read-only nodes.)

Max

signature.asc
Description: OpenPGP digital signature