On Mon, 2013-03-18 at 10:50 -0400, Don Dutile wrote: > On 03/17/2013 06:28 PM, Alex Williamson wrote: > > On Sun, 2013-03-17 at 08:33 -0600, Myron Stowe wrote: > >> On Sun, 2013-03-17 at 07:38 -0600, Alex Williamson wrote: > >>> On Sat, 2013-03-16 at 22:36 -0700, Greg KH wrote: > >>>> On Sat, Mar 16, 2013 at 10:11:22PM -0600, Alex Williamson wrote: > >>>>> On Sat, 2013-03-16 at 18:03 -0700, Greg KH wrote: > >>>>>> On Sat, Mar 16, 2013 at 05:50:53PM -0600, Myron Stowe wrote: > >>>>>>> On Sat, 2013-03-16 at 15:11 -0700, Greg KH wrote: > >>>>>>>> On Sat, Mar 16, 2013 at 03:35:19PM -0600, Myron Stowe wrote: > >>>>>>>>> Sysfs includes entries to memory that backs a PCI device's BARs, both I/O > >>>>>>>>> Port space and MMIO. This memory regions correspond to the device's > >>>>>>>>> internal status and control registers used to drive the device. > >>>>>>>>> > >>>>>>>>> Accessing these registers from userspace such as "udevadm info > >>>>>>>>> --attribute-walk --path=/sys/devices/..." does can not be allowed as > >>>>>>>>> such accesses outside of the driver, even just reading, can yield > >>>>>>>>> catastrophic consequences. > >>>>>>>>> > >>>>>>>>> Udevadm-info skips parsing a specific set of sysfs entries including > >>>>>>>>> 'resource'. This patch extends the set to include the additional > >>>>>>>>> 'resource<N>' entries that correspond to a PCI device's BARs. > >>>>>>>> > >>>>>>>> Nice, are you also going to patch bash to prevent a user from reading > >>>>>>>> these sysfs files as well? :) > >>>>>>>> > >>>>>>>> And pciutils? > >>>>>>>> > >>>>>>>> You get my point here, right? The root user just asked to read all of > >>>>>>>> the data for this device, so why wouldn't you allow it? Just like > >>>>>>>> 'lspci' does. Or bash does. > >>>>>>> > >>>>>>> Yes :P , you raise a very good point, there are a lot of way a user can > >>>>>>> poke around in those BARs. However, there is a difference between > >>>>>>> shooting yourself in the foot and getting what you deserve versus > >>>>>>> unknowingly executing a common command such as udevadm and having the > >>>>>>> system hang. > >>>>>>>> > >>>>>>>> If this hardware has a problem, then it needs to be fixed in the kernel, > >>>>>>>> not have random band-aids added to various userspace programs to paper > >>>>>>>> over the root problem here. Please fix the kernel driver and all should > >>>>>>>> be fine. No need to change udevadm. > >>>>>>> > >>>>>>> Xiangliang initially proposed a patch within the PCI core. Ignoring the > >>>>>>> specific issue with the proposal which I pointed out in the > >>>>>>> https://lkml.org/lkml/2013/3/7/242 thread, that just doesn't seem like > >>>>>>> the right place to effect a change either as PCI's core isn't concerned > >>>>>>> with the contents or access limitations of those regions, those are > >>>>>>> issues that the driver concerns itself with. > >>>>>>> > >>>>>>> So things seem to be gravitating towards the driver. I'm fairly > >>>>>>> ignorant of this area but as Robert succinctly pointed out in the > >>>>>>> originating thread - the AHCI driver only uses the device's MMIO region. > >>>>>>> The I/O related regions are for legacy SFF-compatible ATA ports and are > >>>>>>> not used to driver the device. This, coupled with the observance that > >>>>>>> userspace accesses such as udevadm, and others like you additionally > >>>>>>> point out, do not filter through the device's driver for seems to > >>>>>>> suggest that changes to the driver will not help here either. > >>>>>> > >>>>>> A PCI quirk should handle this properly, right? Why not do that? Worse > >>>>>> thing, the quirk could just not expose these sysfs files for this > >>>>>> device, which would solve all userspace program issues, right? > >>>>> > >>>>> Not exactly. I/O port access through pci-sysfs was added for userspace > >>>>> programs, specifically qemu-kvm device assignment. We use the I/O port > >>>>> resource# files to access device owned I/O port registers using file > >>>>> permissions rather than global permissions such as iopl/ioperm. File > >>>>> permissions also prevent random users from accessing device registers > >>>>> through these files, but of course can't stop a privileged app that > >>>>> chooses to ignore the purpose of these files. A quirk would therefore > >>>>> remove a file that actually has a useful purpose for one app just so > >>>>> another app that has no particular reason for dumping the contents can > >>>>> run unabated. Thanks, > >>>> > >>>> The quirk would only be for this one specific device, which obviously > >>>> can't handle this type of access, so why would you want the sysfs files > >>>> even present for it at all? > >>> > >>> I'm assuming that the device only breaks because udevadm is dumping the > >>> full I/O port register space of the device and that if an actual driver > >>> was interacting with it through this interface that it would work. > >> > >> Correct: > >> the AHCI driver only uses the device's MMIO region. The I/O > >> related regions are for legacy SFF-compatible ATA ports and are > >> not used to driver the device. This, coupled with the > >> observance that userspace accesses such as udevadm, and others > >> like Greg additionally pointed out, do not filter through the > >> device's driver seems to suggest that changes to the driver will > >> not help here either. > > > > That may be true of our AHCI driver, but when it's assigned to a guest > > we're potentially using a completely different stack and cannot make > > that assumption. A guest running in compatibility mode or the option > > ROM for the device may still use I/O port regions. Thanks, > > > > Alex > > > > > > In quick summary: > (1)reading a device's registers may have side effects > on the device operation, e.g., a register maps to a device's FIFO register. > (2) Having two threads read such device registers can cause unknown results, > i.e., driver & user-app. > (3) It may be valid for a user-app to read device regs, e.g., > qemu-kvm assigned device > > So, can't it be solved by: > (a) if no driver is configured for the device, than it's valid for a user-app > to read the device regs ? > -- although diff. user apps doing so still exposes the problem, and > can't be distinguished, e.g., qemu-kvm + udevadm > -- or can file permissions (set by libvirt driving qemu-kvm > device assignment) block multiple user-app reading ? > i.e., basically, a user-level version of a driver allocating > the device, which in the case of qemu-kvm device-assignment, > is what is actually happening! :) > (b) if driver is configured, need a quirk-registration, or generic, optional, > driver function to check for user-app reading approval. > > ok, bash away... I think concurrency is a secondary issue. The primary issue is whether read() is somehow so special in sysfs that all files need to be regarded as o+r. If that's true, then indeed there are concurrency issues. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
