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

[Alex Williamson <alex.williamson@redhat.com>]

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