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

[Kevin Wolf]

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.

Kevin