Re: [PATCH V2] libxl: relax readonly check introduced by XSA-142 fix

[Jim Fehlig <jfehlig@suse.com>]

On 11/12/2015 07:40 PM, Jim Fehlig wrote:
> The fix for XSA-142 is quite a big hammer, rejecting readonly
> disk configuration even when the requested backend is known to
> support readonly. While it is true that qemu doesn't support
> readonly for emulated IDE or AHCI disks
>
> $ /usr/lib/xen/bin/qemu-system-i386 \
>  -drive file=/tmp/disk.raw,if=ide,media=disk,format=raw,readonly=on
> qemu-system-i386: Can't use a read-only drive
>
> $ /usr/lib/xen/bin/qemu-system-i386 -device ahci,id=ahci0 \
>  -drive file=/tmp/disk.raw,if=none,id=ahcidisk-0,format=raw,readonly=on \
>  -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0
> qemu-system-i386: -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0:
> Can't use a read-only drive
>
> It does support readonly SCSI disks
>
> $ /usr/lib/xen/bin/qemu-system-i386 \
>  -drive file=/tmp/disk.raw,if=scsi,media=disk,format=raw,readonly=on
> [ok]
>
> Inside a guest using such a disk, the SCSI kernel driver sees write
> protect on
>
> [   7.339232] sd 2:0:1:0: [sdb] Write Protect is on
>
> Also, PV drivers support readonly, but the patch rejects such
> configuration even when PV drivers (vdev=xvd*) have been explicitly
> specified and creation of an emulated twin is skiped.
>
> This follow-up patch loosens the restriction to reject readonly when
> creating and emulated IDE or AHCI disk, but allows it when the backend

s/and/an/. I can fix this typo if a V3 is needed.

Regards,
Jim

> is known to support readonly.
>
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>
> V2: Along with IDE+readonly, blacklist AHCI+readonly since it is not
>     supported by qemu either.
>
>  tools/libxl/libxl_dm.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 9c9eaa3..cb6deec 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1152,12 +1152,6 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>                          (gc, "file=%s,if=ide,index=%d,readonly=%s,media=cdrom,format=%s,cache=writeback,id=ide-%i",
>                           disks[i].pdev_path, disk, disks[i].readwrite ? "off" : "on", format, dev_number);
>              } else {
> -                if (!disks[i].readwrite) {
> -                    LOG(ERROR,
> -                        "qemu-xen doesn't support read-only disk drivers");
> -                    return ERROR_INVAL;
> -                }
> -
>                  if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
>                      LOG(WARN, "cannot support"" empty disk format for %s",
>                          disks[i].vdev);
> @@ -1185,29 +1179,38 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>                   * For other disks we translate devices 0..3 into
>                   * hd[a-d] and ignore the rest.
>                   */
> -                if (strncmp(disks[i].vdev, "sd", 2) == 0)
> +                if (strncmp(disks[i].vdev, "sd", 2) == 0) {
>                      drive = libxl__sprintf
> -                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
> -                         pdev_path, disk, format);
> -                else if (strncmp(disks[i].vdev, "xvd", 3) == 0)
> +                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,readonly=%s,cache=writeback",
> +                         pdev_path, disk, format, disks[i].readwrite ? "off" : "on");
> +                } else if (strncmp(disks[i].vdev, "xvd", 3) == 0) {
>                      /*
>                       * Do not add any emulated disk when PV disk are
>                       * explicitly asked for.
>                       */
>                      continue;
> -                else if (disk < 6 && b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI) {
> +                } else if (disk < 6 && b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI) {
> +                    if (!disks[i].readwrite) {
> +                        LOG(ERROR, "qemu-xen doesn't support read-only AHCI disk drivers");
> +                        return ERROR_INVAL;
> +                    }
>                      flexarray_vappend(dm_args, "-drive",
>                          GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,cache=writeback",
>                          pdev_path, disk, format),
>                          "-device", GCSPRINTF("ide-hd,bus=ahci0.%d,unit=0,drive=ahcidisk-%d",
>                          disk, disk), NULL);
>                      continue;
> -                } else if (disk < 4)
> +                } else if (disk < 4) {
> +                    if (!disks[i].readwrite) {
> +                        LOG(ERROR, "qemu-xen doesn't support read-only IDE disk drivers");
> +                        return ERROR_INVAL;
> +                    }
>                      drive = libxl__sprintf
>                          (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
>                           pdev_path, disk, format);
> -                else
> +                } else {
>                      continue; /* Do not emulate this disk */
> +                }
>              }
>  
>              flexarray_append(dm_args, "-drive");