[Xen-devel] [RFC] libxl: relax readonly check introduced by XSA-142 fix

[Jim Fehlig <jfehlig@xxxxxxxx>]

Hi All,

Apologies for only noticing the fix for XSA-142 as it starting flowing to our
various downstreams. The current fix seems like quite a big hammer IMO. qemu
doesn't support readonly IDE/SATA 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

But 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 skipped.

The attached follow-up loosens the restriction to reject readonly when creating
and emulated IDE disk, but allows it when the backend is known to support 
readonly.

Regards,
Jim

From 3d58f93b58ab544b9ee168b7bbaf59de1c5532ce Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfehlig@xxxxxxxx>
Date: Tue, 10 Nov 2015 11:33:44 -0700
Subject: [PATCH] libxl: relax readonly check introduced by XSA-142 fix

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 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

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 disk, but allows it when the backend is
known to support readonly.

Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
---
 tools/libxl/libxl_dm.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 9c9eaa3..ccfc827 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,34 @@ 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) {
                     flexarray_vappend(dm_args, "-drive",
-                        
GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,cache=writeback",
-                        pdev_path, disk, format),
+                        
GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,readonly=%s,cache=writeback",
+                                  pdev_path, disk, format, disks[i].readwrite 
? "off" : "on"),
                         "-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");
-- 
1.8.0.1

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel