[PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Stefano Stabellini]

The current libxl code doesn't deal with read-only drives at all.

Upstream QEMU and qemu-xen only support read-only cdrom drives: make
sure to specify "readonly=on" for cdrom drives and return error in case
the user requested a non-cdrom read-only drive.

This is XSA-142, discovered by Lin Liu
(https://bugzilla.redhat.com/show_bug.cgi?id=1257893).

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
XSA-142 is still unissued at this time
---
 tools/libxl/libxl_dm.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 02c0162..468ff9c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1110,13 +1110,18 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             if (disks[i].is_cdrom) {
                 if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY)
                     drive = libxl__sprintf
-                        (gc, "if=ide,index=%d,media=cdrom,cache=writeback,id=ide-%i",
-                         disk, dev_number);
+                        (gc, "if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
+                         disk, disks[i].readwrite ? "off" : "on", dev_number);
                 else
                     drive = libxl__sprintf
-                        (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s,cache=writeback,id=ide-%i",
-                         disks[i].pdev_path, disk, format, dev_number);
+                        (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) {
+                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "qemu-xen doesn't support read-only disk drivers");
+                    return ERROR_INVAL;
+                }
+
                 if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
                     LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "cannot support"
                                " empty disk format for %s", disks[i].vdev);
-- 
1.7.10.4

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Ian Jackson]

Stefano Stabellini writes ("[PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen"):
> The current libxl code doesn't deal with read-only drives at all.
> 
> Upstream QEMU and qemu-xen only support read-only cdrom drives: make
> sure to specify "readonly=on" for cdrom drives and return error in case
> the user requested a non-cdrom read-only drive.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I have determined by experiment that cdroms are always marked
readonly.  My qemu-dm opened a test iso, which I had expressed to be
passed as a cdrom, readonly.

So maybe the guest would be able to get qemu to write to it, but this
would just result in errors (or maybe a crash somewhere in qemu).  I
was not able to easily persuade Linux to write to the cdrom device.

> -                        (gc, "if=ide,index=%d,media=cdrom,cache=writeback,id=ide-%i",
> -                         disk, dev_number);
> +                        (gc, "if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
> +                         disk, disks[i].readwrite ? "off" : "on", dev_number);

I don't know what the effect of this would be on qemu iff
readwrite==0.  I think it might be ineffective.  But that is certainly
not a security problem.

I have just sent a draft of the advisory.

Ian.

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[M A Young]

On Tue, 15 Sep 2015, Stefano Stabellini wrote:

>              } else {
> +                if (!disks[i].readwrite) {
> +                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "qemu-xen doesn't support read-only disk drivers");
> +                    return ERROR_INVAL;
> +                }
> +

Is ERROR_INVAL the right error to return? I get

libxl_dm.c: In function 'libxl__build_device_model_args_new':
libxl_dm.c:807:28: error: return makes pointer from integer without a cast 
[-Werror=int-conversion]
                     return ERROR_INVAL;
                            ^
cc1: all warnings being treated as errors

when I try to build xen with the proposed patch.  NULL is returned when 
there is a problem in other places in this function.

	Michael Young

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Ian Jackson]

M A Young writes ("Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen"):
> On Tue, 15 Sep 2015, Stefano Stabellini wrote:
> Is ERROR_INVAL the right error to return? I get
> 
> libxl_dm.c: In function 'libxl__build_device_model_args_new':
> libxl_dm.c:807:28: error: return makes pointer from integer without a cast 
> [-Werror=int-conversion]
>                      return ERROR_INVAL;
>                             ^
> cc1: all warnings being treated as errors

Yikes.

> when I try to build xen with the proposed patch.  NULL is returned when 
> there is a problem in other places in this function.

Clearly not.

Stefano, do you want to respin ?

Ian.

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Wei Liu]

On Tue, Sep 15, 2015 at 06:53:53PM +0100, M A Young wrote:
> On Tue, 15 Sep 2015, Stefano Stabellini wrote:
> 
> >              } else {
> > +                if (!disks[i].readwrite) {
> > +                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "qemu-xen doesn't support read-only disk drivers");
> > +                    return ERROR_INVAL;
> > +                }
> > +
> 
> Is ERROR_INVAL the right error to return? I get
> 
> libxl_dm.c: In function 'libxl__build_device_model_args_new':
> libxl_dm.c:807:28: error: return makes pointer from integer without a cast 
> [-Werror=int-conversion]
>                      return ERROR_INVAL;
>                             ^
> cc1: all warnings being treated as errors
> 
> when I try to build xen with the proposed patch.  NULL is returned when 
> there is a problem in other places in this function.
> 

I don't follow.

The function in question is libxl__build_device_model_args_new, which
returns an integer. And there are other places in that function that
return ERROR_*.

Do I miss anything?

Wei.

> 	Michael Young

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[M A Young]

On Wed, 16 Sep 2015, Wei Liu wrote:

> On Tue, Sep 15, 2015 at 06:53:53PM +0100, M A Young wrote:
> > On Tue, 15 Sep 2015, Stefano Stabellini wrote:
> > 
> > >              } else {
> > > +                if (!disks[i].readwrite) {
> > > +                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "qemu-xen doesn't support read-only disk drivers");
> > > +                    return ERROR_INVAL;
> > > +                }
> > > +
> > 
> > Is ERROR_INVAL the right error to return? I get
> > 
> > libxl_dm.c: In function 'libxl__build_device_model_args_new':
> > libxl_dm.c:807:28: error: return makes pointer from integer without a cast 
> > [-Werror=int-conversion]
> >                      return ERROR_INVAL;
> >                             ^
> > cc1: all warnings being treated as errors
> > 
> > when I try to build xen with the proposed patch.  NULL is returned when 
> > there is a problem in other places in this function.
> > 
> 
> I don't follow.
> 
> The function in question is libxl__build_device_model_args_new, which
> returns an integer. And there are other places in that function that
> return ERROR_*.
> 
> Do I miss anything?
> 
> Wei.

Hang on, I think it is a case of 4.6 != 4.5 (which is what I was building 
on). So we probably just need a fixed version for earlier versions of xen.

	Michael Young

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Ian Campbell]

On Wed, 2015-09-16 at 14:54 +0100, Ian Jackson wrote:
> M A Young writes ("Re: [PATCH v2 for-4.6] libxl: handle read-only drives
> with qemu-xen"):
> > On Tue, 15 Sep 2015, Stefano Stabellini wrote:
> > Is ERROR_INVAL the right error to return? I get
> > 
> > libxl_dm.c: In function 'libxl__build_device_model_args_new':
> > libxl_dm.c:807:28: error: return makes pointer from integer without a
> > cast 
> > [-Werror=int-conversion]
> >                      return ERROR_INVAL;
> >                             ^
> > cc1: all warnings being treated as errors
> 
> Yikes.
> 
> > when I try to build xen with the proposed patch.  NULL is returned when
> > there is a problem in other places in this function.
> 
> Clearly not.
> 
> Stefano, do you want to respin ?

>From the other subthread it seems this is down to:

    commit de214e9f93de57fb5239e958372f314d29d3f7a9
    Author: Olaf Hering <    olaf@aepfle.de    >
    Date:   Mon Apr 20 13:40:31 2015 +0000

        libxl: pass environment to device model
        
        Prepare device-model setup functions to pass also environment variables
        to the spawned process. This is required for upcoming changes which will
        set DISPLAY and XAUTHORITY for SDL.
        
        Signed-off-by: Olaf Hering <    olaf@aepfle.de    >
        Cc: Ian Jackson <    ian.jackson@eu.citrix.com    >
        Cc: Stefano Stabellini <    stefano.stabellini@eu.citrix.com    >
        Cc: Ian Campbell <    ian.campbell@citrix.com    >
        Cc: Wei Liu <    wei.liu2@citrix.com    >
        Acked-by: Ian Campbell <    ian.campbell@citrix.com    >

which is 4.6 but not in 4.5 (where Michael was applying). So I think this
is just an issue for backport (should return NULL instead) and not for
applying now.

So shall we go ahead with this for 4.6 or is there more
testing/discussion/whatever needed?

Ian.

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[M A Young]

On Thu, 17 Sep 2015, Ian Campbell wrote:

> On Wed, 2015-09-16 at 14:54 +0100, Ian Jackson wrote:
> > M A Young writes ("Re: [PATCH v2 for-4.6] libxl: handle read-only drives
> > with qemu-xen"):
> > > On Tue, 15 Sep 2015, Stefano Stabellini wrote:
> > > Is ERROR_INVAL the right error to return? I get
> > > 
> > > libxl_dm.c: In function 'libxl__build_device_model_args_new':
> > > libxl_dm.c:807:28: error: return makes pointer from integer without a
> > > cast 
> > > [-Werror=int-conversion]
> > >                      return ERROR_INVAL;
> > >                             ^
> > > cc1: all warnings being treated as errors
> > 
> > Yikes.
> > 
> > > when I try to build xen with the proposed patch.  NULL is returned when
> > > there is a problem in other places in this function.
> > 
> > Clearly not.
> > 
> > Stefano, do you want to respin ?
> 
> From the other subthread it seems this is down to:
> 
>     commit de214e9f93de57fb5239e958372f314d29d3f7a9
>     Author: Olaf Hering <    olaf@aepfle.de    >
>     Date:   Mon Apr 20 13:40:31 2015 +0000
> 
>         libxl: pass environment to device model
>         
>         Prepare device-model setup functions to pass also environment variables
>         to the spawned process. This is required for upcoming changes which will
>         set DISPLAY and XAUTHORITY for SDL.
>         
>         Signed-off-by: Olaf Hering <    olaf@aepfle.de    >
>         Cc: Ian Jackson <    ian.jackson@eu.citrix.com    >
>         Cc: Stefano Stabellini <    stefano.stabellini@eu.citrix.com    >
>         Cc: Ian Campbell <    ian.campbell@citrix.com    >
>         Cc: Wei Liu <    wei.liu2@citrix.com    >
>         Acked-by: Ian Campbell <    ian.campbell@citrix.com    >
> 
> which is 4.6 but not in 4.5 (where Michael was applying). So I think this
> is just an issue for backport (should return NULL instead) and not for
> applying now.
> 
> So shall we go ahead with this for 4.6 or is there more
> testing/discussion/whatever needed?

There is an update in the bugzilla report (testing 4.5 with NULL returned)
at https://bugzilla.redhat.com/show_bug.cgi?id=1257893#c9
querying whether xvd? read-only drives should work. I think the answer is 
still no, but I should at least mention the question here.

	Michael Young

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Wei Liu]

On Thu, Sep 17, 2015 at 12:13:20PM +0100, Ian Campbell wrote:
> On Wed, 2015-09-16 at 14:54 +0100, Ian Jackson wrote:
> > M A Young writes ("Re: [PATCH v2 for-4.6] libxl: handle read-only drives
> > with qemu-xen"):
> > > On Tue, 15 Sep 2015, Stefano Stabellini wrote:
> > > Is ERROR_INVAL the right error to return? I get
> > > 
> > > libxl_dm.c: In function 'libxl__build_device_model_args_new':
> > > libxl_dm.c:807:28: error: return makes pointer from integer without a
> > > cast 
> > > [-Werror=int-conversion]
> > >                      return ERROR_INVAL;
> > >                             ^
> > > cc1: all warnings being treated as errors
> > 
> > Yikes.
> > 
> > > when I try to build xen with the proposed patch.  NULL is returned when
> > > there is a problem in other places in this function.
> > 
> > Clearly not.
> > 
> > Stefano, do you want to respin ?
> 
> >From the other subthread it seems this is down to:
> 
>     commit de214e9f93de57fb5239e958372f314d29d3f7a9
>     Author: Olaf Hering <    olaf@aepfle.de    >
>     Date:   Mon Apr 20 13:40:31 2015 +0000
> 
>         libxl: pass environment to device model
>         
>         Prepare device-model setup functions to pass also environment variables
>         to the spawned process. This is required for upcoming changes which will
>         set DISPLAY and XAUTHORITY for SDL.
>         
>         Signed-off-by: Olaf Hering <    olaf@aepfle.de    >
>         Cc: Ian Jackson <    ian.jackson@eu.citrix.com    >
>         Cc: Stefano Stabellini <    stefano.stabellini@eu.citrix.com    >
>         Cc: Ian Campbell <    ian.campbell@citrix.com    >
>         Cc: Wei Liu <    wei.liu2@citrix.com    >
>         Acked-by: Ian Campbell <    ian.campbell@citrix.com    >
> 
> which is 4.6 but not in 4.5 (where Michael was applying). So I think this
> is just an issue for backport (should return NULL instead) and not for
> applying now.
> 
> So shall we go ahead with this for 4.6 or is there more
> testing/discussion/whatever needed?
> 

Yes, of course.

Wei.

> Ian.

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Ian Campbell]

On Thu, 2015-09-17 at 12:35 +0100, Wei Liu wrote:
> On Thu, Sep 17, 2015 at 12:13:20PM +0100, Ian Campbell wrote:
> > On Wed, 2015-09-16 at 14:54 +0100, Ian Jackson wrote:
> > > M A Young writes ("Re: [PATCH v2 for-4.6] libxl: handle read-only
> > > drives
> > > with qemu-xen"):
> > > > On Tue, 15 Sep 2015, Stefano Stabellini wrote:
> > > > Is ERROR_INVAL the right error to return? I get
> > > > 
> > > > libxl_dm.c: In function 'libxl__build_device_model_args_new':
> > > > libxl_dm.c:807:28: error: return makes pointer from integer without
> > > > a
> > > > cast 
> > > > [-Werror=int-conversion]
> > > >                      return ERROR_INVAL;
> > > >                             ^
> > > > cc1: all warnings being treated as errors
> > > 
> > > Yikes.
> > > 
> > > > when I try to build xen with the proposed patch.  NULL is returned
> > > > when
> > > > there is a problem in other places in this function.
> > > 
> > > Clearly not.
> > > 
> > > Stefano, do you want to respin ?
> > 
> > > From the other subthread it seems this is down to:
> > 
> >     commit de214e9f93de57fb5239e958372f314d29d3f7a9
> >     Author: Olaf Hering <    olaf@aepfle.de    >
> >     Date:   Mon Apr 20 13:40:31 2015 +0000
> > 
> >         libxl: pass environment to device model
> >         
> >         Prepare device-model setup functions to pass also environment
> > variables
> >         to the spawned process. This is required for upcoming changes
> > which will
> >         set DISPLAY and XAUTHORITY for SDL.
> >         
> >         Signed-off-by: Olaf Hering <    olaf@aepfle.de    >
> >         Cc: Ian Jackson <    ian.jackson@eu.citrix.com    >
> >         Cc: Stefano Stabellini <    stefano.stabellini@eu.citrix.com   
> >  >
> >         Cc: Ian Campbell <    ian.campbell@citrix.com    >
> >         Cc: Wei Liu <    wei.liu2@citrix.com    >
> >         Acked-by: Ian Campbell <    ian.campbell@citrix.com    >
> > 
> > which is 4.6 but not in 4.5 (where Michael was applying). So I think
> > this
> > is just an issue for backport (should return NULL instead) and not for
> > applying now.
> > 
> > So shall we go ahead with this for 4.6 or is there more
> > testing/discussion/whatever needed?
> > 
> 
> Yes, of course.

"Yes, of course, go ahead" or "Yes, of course, more
testing/discusion/whatever is needed" ?

Yes is not a helpful answer to a question which offers a choice ;-)

Ian.

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Wei Liu]

On Thu, Sep 17, 2015 at 12:49:41PM +0100, Ian Campbell wrote:
[...]
> > > So shall we go ahead with this for 4.6 or is there more
> > > testing/discussion/whatever needed?
> > > 
> > 
> > Yes, of course.
> 
> "Yes, of course, go ahead" or "Yes, of course, more
> testing/discusion/whatever is needed" ?
> 
> Yes is not a helpful answer to a question which offers a choice ;-)
> 

I think this patch should be applied to 4.6 and staging. The patch
itself is correct.

Wei.

> Ian.

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen"):
> So shall we go ahead with this for 4.6 or is there more
> testing/discussion/whatever needed?

I think we can commit it to 4.6.  I would like to issue the actual
advisory though.

I see no-one commented.

It is definitely wrong at least in that it asserts that the same patch
works for 4.5.  Would anyone care to supply a patch for 4.5 and
earlier or shall I prepare one ?

Ian.

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Ian Campbell]

On Thu, 2015-09-17 at 18:58 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v2 for-4.6] libxl: handle read-only
> drives with qemu-xen"):
> > So shall we go ahead with this for 4.6 or is there more
> > testing/discussion/whatever needed?
> 
> I think we can commit it to 4.6.  I would like to issue the actual
> advisory though.
> 
> I see no-one commented.
> 
> It is definitely wrong at least in that it asserts that the same patch
> works for 4.5.  Would anyone care to supply a patch for 4.5 and
> earlier or shall I prepare one ?

I think it should be just s/ERROR_FOO/NULL/g?

> 
> Ian.

Re: [PATCH v2 for-4.6] libxl: handle read-only drives with qemu-xen

[Ian Campbell]

On Tue, 2015-09-15 at 17:22 +0100, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH v2 for-4.6] libxl: handle read-only
> drives with qemu-xen"):
> > The current libxl code doesn't deal with read-only drives at all.
> > 
> > Upstream QEMU and qemu-xen only support read-only cdrom drives: make
> > sure to specify "readonly=on" for cdrom drives and return error in case
> > the user requested a non-cdrom read-only drive.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I applied to staging and staging-4.6 with this Ack and Wei's say so
elsewhere.