Re: [PATCH] udevadm-info: Don't access sysfs 'resource<N>' files
From: Don Dutile
Date: Mon Mar 18 2013 - 10:50:58 EST
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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/