[PATCH] device-assignment: chmod the rom file before opening read/write

[PATCH] device-assignment: chmod the rom file before opening read/write

[Alex Williamson]

The PCI sysfs rom file is exposed read-only by default, but we need
to write to it to enable and disable the ROM around the read.  When
running as root, the code works fine as is, but when running
de-privileged via libvirt, the fopen("r+") will fail if the file
doesn't have owner write permissions.  libvirt already gives us
ownership of the file, so we can toggle this around the short
usage window ourselves.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/device-assignment.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 8446cd4..da0a4d7 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1866,16 +1866,18 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
         return;
     }
 
-    if (access(rom_file, F_OK)) {
-        fprintf(stderr, "pci-assign: Insufficient privileges for %s\n",
-                rom_file);
+    /* The ROM file is typically mode 0400, ensure that it's at least 0600
+     * for the following fopen to succeed when qemu is de-privileged. */
+    if (chmod(rom_file, (st.st_mode & ALLPERMS) | S_IRUSR | S_IWUSR)) {
+        fprintf(stderr, "pci-assign: Insufficient privileges for %s (%s)\n",
+                rom_file, strerror(errno));
         return;
     }
 
     /* Write "1" to the ROM file to enable it */
     fp = fopen(rom_file, "r+");
     if (fp == NULL) {
-        return;
+        goto restore_rom;
     }
     val = 1;
     if (fwrite(&val, 1, 1, fp) != 1) {
@@ -1895,17 +1897,20 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
                 "or load from file with romfile=\n", rom_file);
         qemu_ram_free(dev->dev.rom_offset);
         dev->dev.rom_offset = 0;
-        goto close_rom;
+        goto disable_rom;
     }
 
     pci_register_bar(&dev->dev, PCI_ROM_SLOT,
                      st.st_size, 0, pci_map_option_rom);
-close_rom:
+disable_rom:
     /* Write "0" to disable ROM */
     fseek(fp, 0, SEEK_SET);
     val = 0;
     if (!fwrite(&val, 1, 1, fp)) {
         DEBUG("%s\n", "Failed to disable pci-sysfs rom file");
     }
+close_rom:
     fclose(fp);
+restore_rom:
+    chmod(rom_file, st.st_mode & ALLPERMS);
 }

Re: [PATCH] device-assignment: chmod the rom file before opening read/write

[Chris Wright]

* Alex Williamson (alex.williamson@redhat.com) wrote:
> The PCI sysfs rom file is exposed read-only by default, but we need
> to write to it to enable and disable the ROM around the read.  When
> running as root, the code works fine as is, but when running
> de-privileged via libvirt, the fopen("r+") will fail if the file
> doesn't have owner write permissions.  libvirt already gives us
> ownership of the file, so we can toggle this around the short
> usage window ourselves.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Acked-by: Chris Wright <chrisw@redhat.com>

> ---
> 
>  hw/device-assignment.c |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 8446cd4..da0a4d7 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -1866,16 +1866,18 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
>          return;
>      }
>  
> -    if (access(rom_file, F_OK)) {
> -        fprintf(stderr, "pci-assign: Insufficient privileges for %s\n",
> -                rom_file);
> +    /* The ROM file is typically mode 0400, ensure that it's at least 0600
> +     * for the following fopen to succeed when qemu is de-privileged. */
> +    if (chmod(rom_file, (st.st_mode & ALLPERMS) | S_IRUSR | S_IWUSR)) {
> +        fprintf(stderr, "pci-assign: Insufficient privileges for %s (%s)\n",
> +                rom_file, strerror(errno));
>          return;
>      }
>  
>      /* Write "1" to the ROM file to enable it */
>      fp = fopen(rom_file, "r+");
>      if (fp == NULL) {
> -        return;
> +        goto restore_rom;
>      }
>      val = 1;
>      if (fwrite(&val, 1, 1, fp) != 1) {
> @@ -1895,17 +1897,20 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
>                  "or load from file with romfile=\n", rom_file);
>          qemu_ram_free(dev->dev.rom_offset);
>          dev->dev.rom_offset = 0;
> -        goto close_rom;
> +        goto disable_rom;
>      }
>  
>      pci_register_bar(&dev->dev, PCI_ROM_SLOT,
>                       st.st_size, 0, pci_map_option_rom);
> -close_rom:
> +disable_rom:
>      /* Write "0" to disable ROM */
>      fseek(fp, 0, SEEK_SET);
>      val = 0;
>      if (!fwrite(&val, 1, 1, fp)) {

Nitpick...could you unify this? (!= 1, like the enabling write check)

>          DEBUG("%s\n", "Failed to disable pci-sysfs rom file");
>      }
> +close_rom:
>      fclose(fp);
> +restore_rom:
> +    chmod(rom_file, st.st_mode & ALLPERMS);
>  }
> 

Re: [PATCH] device-assignment: chmod the rom file before opening read/write

[Alex Williamson]

On Tue, 2011-01-04 at 10:30 -0800, Chris Wright wrote:
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > The PCI sysfs rom file is exposed read-only by default, but we need
> > to write to it to enable and disable the ROM around the read.  When
> > running as root, the code works fine as is, but when running
> > de-privileged via libvirt, the fopen("r+") will fail if the file
> > doesn't have owner write permissions.  libvirt already gives us
> > ownership of the file, so we can toggle this around the short
> > usage window ourselves.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Acked-by: Chris Wright <chrisw@redhat.com>
> 
> > ---
> > 
> >  hw/device-assignment.c |   17 +++++++++++------
> >  1 files changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > index 8446cd4..da0a4d7 100644
> > --- a/hw/device-assignment.c
> > +++ b/hw/device-assignment.c
> > @@ -1866,16 +1866,18 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
> >          return;
> >      }
> >  
> > -    if (access(rom_file, F_OK)) {
> > -        fprintf(stderr, "pci-assign: Insufficient privileges for %s\n",
> > -                rom_file);
> > +    /* The ROM file is typically mode 0400, ensure that it's at least 0600
> > +     * for the following fopen to succeed when qemu is de-privileged. */
> > +    if (chmod(rom_file, (st.st_mode & ALLPERMS) | S_IRUSR | S_IWUSR)) {
> > +        fprintf(stderr, "pci-assign: Insufficient privileges for %s (%s)\n",
> > +                rom_file, strerror(errno));
> >          return;
> >      }
> >  
> >      /* Write "1" to the ROM file to enable it */
> >      fp = fopen(rom_file, "r+");
> >      if (fp == NULL) {
> > -        return;
> > +        goto restore_rom;
> >      }
> >      val = 1;
> >      if (fwrite(&val, 1, 1, fp) != 1) {
> > @@ -1895,17 +1897,20 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
> >                  "or load from file with romfile=\n", rom_file);
> >          qemu_ram_free(dev->dev.rom_offset);
> >          dev->dev.rom_offset = 0;
> > -        goto close_rom;
> > +        goto disable_rom;
> >      }
> >  
> >      pci_register_bar(&dev->dev, PCI_ROM_SLOT,
> >                       st.st_size, 0, pci_map_option_rom);
> > -close_rom:
> > +disable_rom:
> >      /* Write "0" to disable ROM */
> >      fseek(fp, 0, SEEK_SET);
> >      val = 0;
> >      if (!fwrite(&val, 1, 1, fp)) {
> 
> Nitpick...could you unify this? (!= 1, like the enabling write check)

Sure, I'll send a quick re-spin.  Thanks,

Alex

> >          DEBUG("%s\n", "Failed to disable pci-sysfs rom file");
> >      }
> > +close_rom:
> >      fclose(fp);
> > +restore_rom:
> > +    chmod(rom_file, st.st_mode & ALLPERMS);
> >  }
> > 

[PATCH v2] device-assignment: chmod the rom file before opening read/write

[Alex Williamson]

The PCI sysfs rom file is exposed read-only by default, but we need
to write to it to enable and disable the ROM around the read.  When
running as root, the code works fine as is, but when running
de-privileged via libvirt, the fopen("r+") will fail if the file
doesn't have owner write permissions.  libvirt already gives us
ownership of the file, so we can toggle this around the short
usage window ourselves.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Acked-by: Chris Wright <chrisw@redhat.com>
---

 hw/device-assignment.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 8446cd4..81c77ae 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1866,16 +1866,18 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
         return;
     }
 
-    if (access(rom_file, F_OK)) {
-        fprintf(stderr, "pci-assign: Insufficient privileges for %s\n",
-                rom_file);
+    /* The ROM file is typically mode 0400, ensure that it's at least 0600
+     * for the following fopen to succeed when qemu is de-privileged. */
+    if (chmod(rom_file, (st.st_mode & ALLPERMS) | S_IRUSR | S_IWUSR)) {
+        fprintf(stderr, "pci-assign: Insufficient privileges for %s (%s)\n",
+                rom_file, strerror(errno));
         return;
     }
 
     /* Write "1" to the ROM file to enable it */
     fp = fopen(rom_file, "r+");
     if (fp == NULL) {
-        return;
+        goto restore_rom;
     }
     val = 1;
     if (fwrite(&val, 1, 1, fp) != 1) {
@@ -1895,17 +1897,20 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
                 "or load from file with romfile=\n", rom_file);
         qemu_ram_free(dev->dev.rom_offset);
         dev->dev.rom_offset = 0;
-        goto close_rom;
+        goto disable_rom;
     }
 
     pci_register_bar(&dev->dev, PCI_ROM_SLOT,
                      st.st_size, 0, pci_map_option_rom);
-close_rom:
+disable_rom:
     /* Write "0" to disable ROM */
     fseek(fp, 0, SEEK_SET);
     val = 0;
-    if (!fwrite(&val, 1, 1, fp)) {
+    if (fwrite(&val, 1, 1, fp) != 1) {
         DEBUG("%s\n", "Failed to disable pci-sysfs rom file");
     }
+close_rom:
     fclose(fp);
+restore_rom:
+    chmod(rom_file, st.st_mode & ALLPERMS);
 }

Re: [PATCH v2] device-assignment: chmod the rom file before opening read/write

[Avi Kivity]

On 01/04/2011 08:45 PM, Alex Williamson wrote:
> The PCI sysfs rom file is exposed read-only by default, but we need
> to write to it to enable and disable the ROM around the read.  When
> running as root, the code works fine as is, but when running
> de-privileged via libvirt, the fopen("r+") will fail if the file
> doesn't have owner write permissions.  libvirt already gives us
> ownership of the file, so we can toggle this around the short
> usage window ourselves.

Why is qemu in the business of chmod()ing resources?  If qemu needs 
write access to some resource, the user needs to provide that access.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2] device-assignment: chmod the rom file before opening read/write

[Alex Williamson]

On Wed, 2011-01-05 at 10:57 +0200, Avi Kivity wrote:
> On 01/04/2011 08:45 PM, Alex Williamson wrote:
> > The PCI sysfs rom file is exposed read-only by default, but we need
> > to write to it to enable and disable the ROM around the read.  When
> > running as root, the code works fine as is, but when running
> > de-privileged via libvirt, the fopen("r+") will fail if the file
> > doesn't have owner write permissions.  libvirt already gives us
> > ownership of the file, so we can toggle this around the short
> > usage window ourselves.
> 
> Why is qemu in the business of chmod()ing resources?  If qemu needs 
> write access to some resource, the user needs to provide that access.

A valid argument.  I think it could also be argued that the user is
providing ownership of the file and writing to the file is part of the
low level details of the sysfs rom file API and should be handled by the
user of that API.  We basically have 3 places we could put this:

     A. kernel - Why is this file mode 0400 by default anyway if using
        it requires write access?  Set it to mode 0600 here by default.
     B. libvirt - Already does chown, why not do chmod too?  chmod and
        restore here.
     C. qemu - Owns file, chmod is trivial and part of the sysfs rom
        file API?  chmod around usage.

I chose qemu because it seemed to have the least chance of side-effects
and has the smallest usage window.  Do you prefer libvirt or kernel?
Thanks,

Alex

Re: [PATCH v2] device-assignment: chmod the rom file before opening read/write

[Avi Kivity]

On 01/05/2011 04:57 PM, Alex Williamson wrote:
> A valid argument.  I think it could also be argued that the user is
> providing ownership of the file and writing to the file is part of the
> low level details of the sysfs rom file API and should be handled by the
> user of that API.  We basically have 3 places we could put this:
>
>       A. kernel - Why is this file mode 0400 by default anyway if using
>          it requires write access?  Set it to mode 0600 here by default.
>       B. libvirt - Already does chown, why not do chmod too?  chmod and
>          restore here.
>       C. qemu - Owns file, chmod is trivial and part of the sysfs rom
>          file API?  chmod around usage.
>

qemu might not actually own the file, just have rw permissions.  Or it 
might own the file and selinux may prevent it from changing the 
permissions.  Or it may die before the reverse chmod and leave things 
not as they were.

> I chose qemu because it seemed to have the least chance of side-effects
> and has the smallest usage window.  Do you prefer libvirt or kernel?

No idea really.  What's the kernel's motivation for keeping it ro?  Sanity?

I'd guess libvirt is the one to do it, but someone more familiar with 
device assignment / pci (you?) should weigh in on this.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2] device-assignment: chmod the rom file before opening read/write

[Daniel P. Berrange]

On Wed, Jan 05, 2011 at 05:14:55PM +0200, Avi Kivity wrote:
> On 01/05/2011 04:57 PM, Alex Williamson wrote:
> >A valid argument.  I think it could also be argued that the user is
> >providing ownership of the file and writing to the file is part of the
> >low level details of the sysfs rom file API and should be handled by the
> >user of that API.  We basically have 3 places we could put this:
> >
> >      A. kernel - Why is this file mode 0400 by default anyway if using
> >         it requires write access?  Set it to mode 0600 here by default.
> >      B. libvirt - Already does chown, why not do chmod too?  chmod and
> >         restore here.
> >      C. qemu - Owns file, chmod is trivial and part of the sysfs rom
> >         file API?  chmod around usage.
> >
> 
> qemu might not actually own the file, just have rw permissions.  Or
> it might own the file and selinux may prevent it from changing the
> permissions.  Or it may die before the reverse chmod and leave
> things not as they were.

Agreed, I don't think we can rely on QEMU being able to chmod() the
file in general.

> 
> >I chose qemu because it seemed to have the least chance of side-effects
> >and has the smallest usage window.  Do you prefer libvirt or kernel?
> 
> No idea really.  What's the kernel's motivation for keeping it ro?  Sanity?
> 
> I'd guess libvirt is the one to do it, but someone more familiar
> with device assignment / pci (you?) should weigh in on this.

I've no real objection to libvirt setting the 0600 permissions
on it, if that's required for correct operation.

BTW, what is the failure scenario seen when the file is 0400.
I want to know how to diagnose/triage this if it gets reported
by users in BZ...

Regards,
Daniel

Re: [PATCH v2] device-assignment: chmod the rom file before opening read/write

[Alex Williamson]

On Wed, 2011-01-05 at 15:26 +0000, Daniel P. Berrange wrote:
> On Wed, Jan 05, 2011 at 05:14:55PM +0200, Avi Kivity wrote:
> > On 01/05/2011 04:57 PM, Alex Williamson wrote:
> > >A valid argument.  I think it could also be argued that the user is
> > >providing ownership of the file and writing to the file is part of the
> > >low level details of the sysfs rom file API and should be handled by the
> > >user of that API.  We basically have 3 places we could put this:
> > >
> > >      A. kernel - Why is this file mode 0400 by default anyway if using
> > >         it requires write access?  Set it to mode 0600 here by default.
> > >      B. libvirt - Already does chown, why not do chmod too?  chmod and
> > >         restore here.
> > >      C. qemu - Owns file, chmod is trivial and part of the sysfs rom
> > >         file API?  chmod around usage.
> > >
> > 
> > qemu might not actually own the file, just have rw permissions.  Or
> > it might own the file and selinux may prevent it from changing the
> > permissions.  Or it may die before the reverse chmod and leave
> > things not as they were.
> 
> Agreed, I don't think we can rely on QEMU being able to chmod() the
> file in general.
> 
> > 
> > >I chose qemu because it seemed to have the least chance of side-effects
> > >and has the smallest usage window.  Do you prefer libvirt or kernel?
> > 
> > No idea really.  What's the kernel's motivation for keeping it ro?  Sanity?
> > 
> > I'd guess libvirt is the one to do it, but someone more familiar
> > with device assignment / pci (you?) should weigh in on this.
> 
> I've no real objection to libvirt setting the 0600 permissions
> on it, if that's required for correct operation.

I'll try the kernel first, digging through history it looks more like a
kernel bug.

> BTW, what is the failure scenario seen when the file is 0400.
> I want to know how to diagnose/triage this if it gets reported
> by users in BZ...

Nothing terrible; on older versions the option ROM contents won't
contain the actual option rom, on newer versions, the ROM BAR doesn't
even get added when the fopen fails.  Thanks,

Alex

Re: [PATCH v2] device-assignment: chmod the rom file before opening read/write

[Alex Williamson]

On Wed, 2011-01-05 at 17:14 +0200, Avi Kivity wrote:
> On 01/05/2011 04:57 PM, Alex Williamson wrote:
> > A valid argument.  I think it could also be argued that the user is
> > providing ownership of the file and writing to the file is part of the
> > low level details of the sysfs rom file API and should be handled by the
> > user of that API.  We basically have 3 places we could put this:
> >
> >       A. kernel - Why is this file mode 0400 by default anyway if using
> >          it requires write access?  Set it to mode 0600 here by default.
> >       B. libvirt - Already does chown, why not do chmod too?  chmod and
> >          restore here.
> >       C. qemu - Owns file, chmod is trivial and part of the sysfs rom
> >          file API?  chmod around usage.
> >
> 
> qemu might not actually own the file, just have rw permissions.  Or it 
> might own the file and selinux may prevent it from changing the 
> permissions.  Or it may die before the reverse chmod and leave things 
> not as they were.
> 
> > I chose qemu because it seemed to have the least chance of side-effects
> > and has the smallest usage window.  Do you prefer libvirt or kernel?
> 
> No idea really.  What's the kernel's motivation for keeping it ro?  Sanity?
> 
> I'd guess libvirt is the one to do it, but someone more familiar with 
> device assignment / pci (you?) should weigh in on this.

I think I'll try the kernel first.  Digging back through bitkeeper, this
file has been read-only since it was introduced.  It looks like maybe
the enabling/disabling write was a request by Linus, so perhaps it's
simply a bug that the file never got S_IWUSR permission when the extra
logic was added.  If so, there's no point in qemu or libvirt working
around a trivial kernel bug.  Thanks,

Alex