libxl: relax readonly check introduced by XSA-142 fix

From 3d58f93b58ab544b9ee168b7bbaf59de1c5532ce Mon Sep 17 00:00:00 2001

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

tools/libxl/libxl_dm.c +4 -10

index 9c9eaa3..ccfc827 100644
@@ -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.
                      */

tools/libxl/libxl_dm.c +2 -1

                     continue;
-                } else if (disk < 4)
+                } else if (disk < 4) {
+                    if (!disks[i].readwrite) {

tools/libxl/libxl_dm.c +0 -0

             }
 
             flexarray_append(dm_args, "-drive");

Signature · 1 line

1.8.0.1

Quoted 24 lines · Jim Fehlig

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.

Quoted 5 lines

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

Hi Jim,

thanks for the patch, I think is good. Just one question below:

Quoted 39 lines

tools/libxl/libxl_dm.c +5 -12

index 9c9eaa3..ccfc827 100644
@@ -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",

tools/libxl/libxl_dm.c static int libxl__build_device_model_args_new(libxl__gc *gc, · line 1197

Quoted 5 lines

tools/libxl/libxl_dm.c +2 -1

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

The commit message doesn't say anything about AHCI. Are AHCI disks actually emulated correctly by QEMU with readonly=on?

Quoted 6 lines

tools/libxl/libxl_dm.c +2 -1

                     continue;
-                } else if (disk < 4)
+                } else if (disk < 4) {
+                    if (!disks[i].readwrite) {

tools/libxl/libxl_dm.c static int libxl__build_device_model_args_new(libxl__gc *gc, · line 1209

Quoted 10 lines

tools/libxl/libxl_dm.c +5 -1

+                        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 */
+                }

I don't think that libxl CODING_STYLE requires brackets for one statement blocks. But I won't enforce it.

Quoted 8 lines

tools/libxl/libxl_dm.c +0 -0

             }
 
             flexarray_append(dm_args, "-drive");

Signature · 1 line

1.8.0.1

Quoted 2 lines · Stefano Stabellini

I don't think that libxl CODING_STYLE requires brackets for one statement blocks. But I won't enforce it.

I can remove the unneeded brackets if desired. That habit comes from working on projects where all if/else blocks are required to have brackets when any one of the blocks requires brackets.

Quoted 2 lines

The commit message doesn't say anything about AHCI. Are AHCI disks actually emulated correctly by QEMU with readonly=on?

I just double checked, and good thing since AHCI + readonly is another rejected
combination

/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

So IDE/SATA/AHCI are all incompatible with readonly=on. I'll fix this and ammend
the commit message in V2.

Quoted 14 lines · Jim Fehlig

Quoted 3 lines

The commit message doesn't say anything about AHCI. Are AHCI disks actually emulated correctly by QEMU with readonly=on?

I just double checked, and good thing since AHCI + readonly is another rejected
combination

/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

Quoted 3 lines

So IDE/SATA/AHCI are all incompatible with readonly=on. I'll fix this and ammend
the commit message in V2.

Just to clarify when you say "rejected" and "incompatible" do you mean that qemu will fail to start if you try, or that it will ignore you and give a writeable disk?

If, as I think, it will fail, why don't we just always ask and rely on qemu to reject, instead of trying to whitelist the ones we know work in the libxl code?

Quoted 22 lines · Ian Campbell

Quoted 20 lines · Jim Fehlig

Quoted 2 lines

The commit message doesn't say anything about AHCI. Are AHCI disks actually emulated correctly by QEMU with readonly=on?

I just double checked, and good thing since AHCI + readonly is another
rejected
combination

/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

So IDE/SATA/AHCI are all incompatible with readonly=on. I'll fix this and
ammend
the commit message in V2.

Quoted 5 lines

Just to clarify when you say "rejected" and "incompatible" do you mean that
qemu will fail to start if you try, or that it will ignore you and give a
writeable disk?

qemu will fail to start.

Quoted 4 lines

If, as I think, it will fail, why don't we just always ask and rely on qemu
to reject, instead of trying to whitelist the ones we know work in the libxl code?

That would be possible, but makes it more difficult to track down why the domain failed to start.[...]

libxl: error: libxl_create.c:1340:domcreate_devmodel_started: device model did not start: -6