From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [PATCH] device-assignment: chmod the rom file before opening read/write Date: Tue, 04 Jan 2011 11:36:38 -0700 Message-ID: <1294166198.14851.0.camel@x201> References: <20110104180649.23471.81148.stgit@s20.home> <20110104183012.GG483@x200.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Chris Wright Return-path: Received: from mx1.redhat.com ([209.132.183.28]:24405 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751037Ab1ADSgk (ORCPT ); Tue, 4 Jan 2011 13:36:40 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id p04IaeC6013694 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 4 Jan 2011 13:36:40 -0500 In-Reply-To: <20110104183012.GG483@x200.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: 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 > > Acked-by: Chris Wright > > > --- > > > > 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); > > } > >