[PATCH] udevadm-info: Don't access sysfs entries backing device I/O port space

[Myron Stowe]

I've been working on identifying the root cause of an issue exposed by
'udevadm' that was first exposed on the linux-pci mail list [1] and
believe that there is now enough of an understanding to propose a fix.

What was originally witnessed was the platform hanging after "udevadm info
--attribute-walk --path=/sys/devices/pci0000:00/<...>/block/sda" is ran.
Xiangliang was able to isolate the failure to accesses involving a Marvell
9125 device's I/O BARs, or more specifically, accesses to the I/O port
space backing the device's I/O BARs (a.k.a. the device's I/O port
resources, or regions). With this knowledge he was able to reproduce the
hang targeting the device's sysfs 'resource<N>' entries, where N
represents an I/O BARs, with "cat /sys/devices/<...>/resource<N>".

In my research, looking for possible solutions, I noticed that kernel
commit 8633328 introduced sysfs based reading and writing of I/O port
related 'resource<N>' entries as part of adding virtualization based
device assignment functionality [2]. Note that these regions directly map
to the device's control and status registers [3].

Putting together these pieces of information we now understand that:
o udevadm based attribute walking initiates read accesses of all the
entries in a device's sysfs directory [4],
o sysfs 'resource<N>' entries correspond to the device's internal
status and control registers used for driving the device,
o If the 'resource<N>' entry corresponds to a device's I/O BAR, the
device's control and status registers are directly accessible by
userspace.

Allowing userspace access to a device's registers introduces potential
simultaneous interaction with the device by a second, competing, entity;
There is the device's driver, which believes it exclusively owns the
device, and an unknown, potential second entity, which can effect control
and status changes to the device asynchronously.

Device status and control registers being accessed from an entity that has
no idea what is being read or written is just asking for trouble. Even
just reading can have consequences as the register may be a "read once to
clear" or some similar type. I think we have just been lucky, or
blissfully ignorant, concerning problems that may have, and still could
be, occurring due to this situation.


There is an aspect at play here that I still do not understand (likely
something obvious that I'm just not seeing). The sysfs read routine for
accessing I/O port 'resource<N>' entries only supports 1, 2, and 4 byte
reads (which respectively correspond to inb/outb, inw/outw, and inl/outl
I/O port accessors). When "udevadm ..." executes, the udev internals
attempt reads of 4K byte chunks.

"udevadm info --attribute-walk --path=<pci_device_path>"

print_device_chain()
print_all_attributes()
...
udev_device_get_sysattr_value()
char value[4096];
...
size = read(fd, value, sizeof(value));
...

-- ^ userspace ^ -- v kernel v --

pci_read_resource_io(..., count) # sysfs read setup in pci_create_attr()
pci_resource_io(..., count, ...)
...
if (port + count - 1 > pci_resource_end(pdev, i))
return -EINVAL;
...

What I don't understand is how the device's I/O port space is successfully
getting read. It looks to me like 'pci_resource_io()' would fail the
access size check and return '-EINVAL' having never attempted the read's
access to I/O port space causing the system to hang.

I'm keep looking into this but I do *not* have access to a platform with a
Marvell 9125 device.


Reference(s)/Foot Note(s):
[1] https://lkml.org/lkml/2013/3/7/242
[2] Note that due to the implementation specifics only the 'resource<N>'
entries representing I/O BARs can be read or written via sysfs.
Sysfs' 'resource<N>' entries representing MMIO do not have sysfs
based read/write routines as only mmap'ing of these entries is
exposed (./drivers/pci/pci-sysfs.c::pci_create_attr()).
[3] The kernel's sysfs documentation states: "Attributes should be ASCII
text files..." (./Documentation/filesystems/sysfs.txt). I wonder if
this is just out-of-date infomation as sysfs obviously supports
creating binary files (./fs/sysfs/bin.c::sysfs_create_bin_file()).
[4] Note that udevadm-info does skip a specifically named set of entries
(./src/udevadm-info.c::skip_attribute()).
---

Myron Stowe (1):
udevadm-info: Don't access sysfs 'resource<N>' files


src/udevadm-info.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)

--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Myron Stowe]

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.

Reported-by: Xiangliang Yu <yuxi...@marvell.com>
Signed-off-by: Myron Stowe <myron...@redhat.com>
---

src/udevadm-info.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/src/udevadm-info.c b/src/udevadm-info.c
index ee9b59f..298acb5 100644
--- a/src/udevadm-info.c
+++ b/src/udevadm-info.c
@@ -37,13 +37,18 @@ static bool skip_attribute(const char *name)
"uevent",
"dev",
"modalias",
- "resource",
"driver",
"subsystem",
"module",
};
unsigned int i;

+ /*
+ * Skip any sysfs 'resource' entries, including 'resource<N>' entries
+ * that correspond to a device's I/O Port or MMIO space backed BARs.
+ */
+ if (strncmp((const char *)name, "resource", sizeof("resource")-1) == 0)
+ return true;
for (i = 0; i < ARRAY_SIZE(skip); i++)
if (strcmp(name, skip[i]) == 0)
return true;

[Greg KH]

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.

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.

greg k-h

[Bjorn Helgaas]

On Sat, Mar 16, 2013 at 4:11 PM, Greg KH <gre...@linuxfoundation.org> 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.
>
> 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.

I'm not sure that "udevadm info" (or bash) reading device registers is
a good idea because we don't know what the device is, and we don't
have any idea what the side effects of reading its registers will be.
Just to be clear, this is about device-specific I/O port registers,
not config space, so we can't expect any sort of consistency.

We could put a quirk in the kernel for this device (obviously the
issue is independent of whether the driver is loaded), but no doubt
other devices with I/O BARs will have access size restrictions, side
effects, or other issues. Adding quirks for them feels like a
never-ending job.

It might have been a mistake to put the resourceN files in sysfs in
the first place, or to make them read/writable, because users expect
sysfs files to contain ASCII. For memory BARs, resourceN only allows
mmap, not read/write, so at least we side-step similar issues there.

Bjorn

[Myron Stowe]

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.

That said, I was attempting to point out an interesting problem and get
the conversation started towards coming up with some type a solution.
Let's continue the conversation and see where things go.

Thanks,
Myron

[Greg KH]

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?

thanks,

[Alex Williamson]

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,

Alex

[Greg KH]

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?

greg k-h

[Alex Williamson]

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. Who
knows how many devices will have read side-effects by udevadm blindly
dumping these files. Thanks,

Alex

[Kay Sievers]

On Sun, Mar 17, 2013 at 2:38 PM, Alex Williamson
<alex.wi...@redhat.com> wrote:
> 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. Who
> knows how many devices will have read side-effects by udevadm blindly
> dumping these files. Thanks,

Sysfs is a too public interface to export things there which make
devices/driver choke on a simple read() of an attribute.

This is nothing specific to udevadm, any tool can do that. Udevadm
will never read any of the files during normal operation. The admin
explicitly asked udevadm with a specific command to dump all the stuff
the device offers.

The kernel driver needs to be fixed to allow that, in the worst case,
the attributes not exported at all. People should take more care what
they export in /sys, it's not a hidden and private ioctl what's
exported there, stuff is very visible and will be looked at.

Telling userspace not to use specific stuff in /sys I would not expect
to work as a strategy; there is too much weird stuff out there that
will always try to do that ...

Thanks,
Kay

[Myron Stowe]

The quirk you are suggesting would basically have to be a reversion of
commit 8633328 for the reasons that Bjorn pointed out so that we cover
all devices, not just this one particular device:

We could put a quirk in the kernel for this device (obviously
the
issue is independent of whether the driver is loaded), but no
doubt
other devices with I/O BARs will have access size restrictions,
side
effects, or other issues. Adding quirks for them feels like a
never-ending job.

I'm beginning to think that people have not read the analysis which was
the first mail entry of this thread (I meant for the Subject: to read
"PATCH 0/1] ...) which is at https://lkml.org/lkml/2013/3/16/168

It appears [*] that we are exposed to this potential conflict with
*every* PCI device's resource# files; not just this one particular
device (again see the analysis cover email, especially the three
paragraphs starting with "Putting together...").

[*] I carefully use the word "appears" due to the one aspect of this
whole issue that I still do not understand which I also expressed in the
cover - which is immediately below the section I just pointed out above.


So what I'd like to understand and why we are focusing on this one
particular instance/device when we *appear* to be at risk with all
devices and their resource# files?

Myron

[Myron Stowe]

On Sun, 2013-03-17 at 15:00 +0100, Kay Sievers wrote:
> On Sun, Mar 17, 2013 at 2:38 PM, Alex Williamson
> <alex.wi...@redhat.com> wrote:
> > 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. Who
> > knows how many devices will have read side-effects by udevadm blindly
> > dumping these files. Thanks,
>
> Sysfs is a too public interface to export things there which make
> devices/driver choke on a simple read() of an attribute.
>
> This is nothing specific to udevadm, any tool can do that. Udevadm
> will never read any of the files during normal operation. The admin
> explicitly asked udevadm with a specific command to dump all the stuff
> the device offers.
>
> The kernel driver needs to be fixed to allow that, in the worst case,
> the attributes not exported at all. People should take more care what
> they export in /sys, it's not a hidden and private ioctl what's
> exported there, stuff is very visible and will be looked at.
>
> Telling userspace not to use specific stuff in /sys I would not expect
> to work as a strategy; there is too much weird stuff out there that
> will always try to do that ...

Kay - could you comment on Foot Note 3 in
https://lkml.org/lkml/2013/3/16/168

With respect to 'udev', you are working on the assumption that all files
in sysfs must be readable with no consequences which may be implied by
the Documentation's sysfs.txt file's mentioning ASCII. If we are to
interpret that as strictly as you seem to want to then why is there
sysfs support for creating binary files?

Myron

[Myron Stowe]

On Sun, 2013-03-17 at 15:29 +0100, Kay Sievers wrote:

> They cannot be distinguished from outside, so there is nothing I know
> that could make a difference to userspace tools.

Agreed
>
> Tools -- no matter how useful they are not not, it's that they do that
> for many years already -- need to be able to read() the stuff in
> there, without causing any damage to the system.

So then, why are certain sysfs files skipped in udevadm-info's parsing
(./src/udevadm-info.c::skip_attribute())?

[Kay Sievers]

On Sun, Mar 17, 2013 at 3:20 PM, Myron Stowe <mst...@redhat.com> wrote:

They cannot be distinguished from outside, so there is nothing I know
that could make a difference to userspace tools.

Tools -- no matter how useful they are not not, it's that they do that
for many years already -- need to be able to read() the stuff in
there, without causing any damage to the system.

[Myron Stowe]

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.

[Kay Sievers]

Because they are not useful to use in udev rules, or are just not
recommended to use in rules because they break other assumptions and
would encode specific settings, which can rightfully change at
runtime, into rules.

The list is in no way a list to ensure a system/driver/device is not
choking on read().

[Alex Williamson]

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,

[Don Dutile]

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...

[Alex Williamson]

On Sun, 2013-03-17 at 15:00 +0100, Kay Sievers wrote:

> On Sun, Mar 17, 2013 at 2:38 PM, Alex Williamson
> <alex.wi...@redhat.com> wrote:
> > 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. Who
> > knows how many devices will have read side-effects by udevadm blindly
> > dumping these files. Thanks,
>
> Sysfs is a too public interface to export things there which make
> devices/driver choke on a simple read() of an attribute.

That's why the default permissions for the file do not allow users to
read it. I wish we could do something as clever as the MMIO resource
files, but I/O port spaces don't allow mmap for the predominant
architecture. Eventually VFIO is meant to replace this access and does
move device register access behind ioctls, but for now legacy KVM device
assignment relies on these files and so might some UIO drivers.

> This is nothing specific to udevadm, any tool can do that. Udevadm
> will never read any of the files during normal operation. The admin
> explicitly asked udevadm with a specific command to dump all the stuff
> the device offers.

Isn't it possible udevadm could drop privileges or filter out non-world
readable files?

> The kernel driver needs to be fixed to allow that, in the worst case,
> the attributes not exported at all. People should take more care what
> they export in /sys, it's not a hidden and private ioctl what's
> exported there, stuff is very visible and will be looked at.

File permissions...

> Telling userspace not to use specific stuff in /sys I would not expect
> to work as a strategy; there is too much weird stuff out there that
> will always try to do that ...

I agree, the kernel needs to protect itself from malicious apps, but if
you run a malicious app with admin access, how much can/should we do?
If we're going to ignore file permissions, why limit ourselves to
read(), should we make everything safe against write() as well? Thanks,

Alex

[Alex Williamson]

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

[Greg KH]

On Mon, Mar 18, 2013 at 10:24:40AM -0600, Alex Williamson wrote:
> On Sun, 2013-03-17 at 15:00 +0100, Kay Sievers wrote:
> > On Sun, Mar 17, 2013 at 2:38 PM, Alex Williamson
> > <alex.wi...@redhat.com> wrote:
> > > 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. Who
> > > knows how many devices will have read side-effects by udevadm blindly
> > > dumping these files. Thanks,
> >
> > Sysfs is a too public interface to export things there which make
> > devices/driver choke on a simple read() of an attribute.
>
> That's why the default permissions for the file do not allow users to
> read it. I wish we could do something as clever as the MMIO resource
> files, but I/O port spaces don't allow mmap for the predominant
> architecture. Eventually VFIO is meant to replace this access and does
> move device register access behind ioctls, but for now legacy KVM device
> assignment relies on these files and so might some UIO drivers.
>
> > This is nothing specific to udevadm, any tool can do that. Udevadm
> > will never read any of the files during normal operation. The admin
> > explicitly asked udevadm with a specific command to dump all the stuff
> > the device offers.
>
> Isn't it possible udevadm could drop privileges or filter out non-world
> readable files?

And you are going to do the same thing for bash? All other shells?

Come on, the user specifically asked to read this file, as root, and
udev did so. Just like bash would.

Please fix the kernel if this is a real problem, you aren't going to be
able to patch all userspace programs, that's not the proper solution
here.

thanks,

greg k-h

[Alex Williamson]

At least for KVM the kernel fix is the addition of the vfio driver which
gives us a non-sysfs way to do this. If this problem was found a few
years later and we were ready to make the switch I'd support just
removing these resource files. In the meantime we have userspace that
depends on this interface, so I'm open to suggestions how to fix it.

If we want to blacklist this specific device, that's fine, but as others
have pointed out it's really a class problem. Perhaps we report 1 byte
extra for the file length where EOF-1 is an enable byte? Is there
anything else in file ops that we could use to make it slightly more
complicated than open(), read() to access the device? Thanks,

Alex

[Bjørn Mork]

Alex Williamson <alex.wi...@redhat.com> writes:

> At least for KVM the kernel fix is the addition of the vfio driver which
> gives us a non-sysfs way to do this. If this problem was found a few
> years later and we were ready to make the switch I'd support just
> removing these resource files. In the meantime we have userspace that
> depends on this interface, so I'm open to suggestions how to fix it.

I am puzzled by a couple of things in this discussion:

1) do you seriously mean that a userspace application (any, not just
udevadm or qemu or whatever) should be able to read and write these
registers while the device is owned by a driver? How is that ever
going to work?

2) is it really so that a device can be so fundamentally screwed up by
reading some registers, that a later driver probe cannot properly
reinitialize it?

I would have thought that the solution to all this was to return -EINVAL
on any attemt to read or write these files while a driver is bound to
the device. If userspace is going to use the API, then the application
better unbind any driver first.

Or? Am I missing something here?

> If we want to blacklist this specific device, that's fine, but as others
> have pointed out it's really a class problem. Perhaps we report 1 byte
> extra for the file length where EOF-1 is an enable byte? Is there
> anything else in file ops that we could use to make it slightly more
> complicated than open(), read() to access the device? Thanks,

If there really are devices which cannot handle reading at all, and
cannot be reset to a sane state by later driver initialization, then a
blacklist could be added for those devices. This should not be a common
problem.



Bjørn

[Alex Williamson]

On Mon, 2013-03-18 at 18:20 +0100, Bjørn Mork wrote:
> Alex Williamson <alex.wi...@redhat.com> writes:
>
> > At least for KVM the kernel fix is the addition of the vfio driver which
> > gives us a non-sysfs way to do this. If this problem was found a few
> > years later and we were ready to make the switch I'd support just
> > removing these resource files. In the meantime we have userspace that
> > depends on this interface, so I'm open to suggestions how to fix it.
>
> I am puzzled by a couple of things in this discussion:
>
> 1) do you seriously mean that a userspace application (any, not just
> udevadm or qemu or whatever) should be able to read and write these
> registers while the device is owned by a driver? How is that ever
> going to work?

The expectation is that the user doesn't mess with the device through
pci-sysfs while it's running. This is really no different than config
space or MMIO space in that respect. You can use setpci to break your
PCI card while it's used by the driver today. The difference is that
MMIO spaces side-step the issue by only allowing mmap and config space
is known not to have read side-effects.

> 2) is it really so that a device can be so fundamentally screwed up by
> reading some registers, that a later driver probe cannot properly
> reinitialize it?

Never underestimate how broken hardware can be, though in this case
reading a device register seems to be causing a system hang/reset.

> I would have thought that the solution to all this was to return -EINVAL
> on any attemt to read or write these files while a driver is bound to
> the device. If userspace is going to use the API, then the application
> better unbind any driver first.
>
> Or? Am I missing something here?

That doesn't really solve anything though. Let's pretend the resource
files only work while the device is bound to pci-stub. Now what happens
when you run this udevadm command as admin while it's in use by the
userspace driver? All we've done is limit the scope of the problem.

> > If we want to blacklist this specific device, that's fine, but as others
> > have pointed out it's really a class problem. Perhaps we report 1 byte
> > extra for the file length where EOF-1 is an enable byte? Is there
> > anything else in file ops that we could use to make it slightly more
> > complicated than open(), read() to access the device? Thanks,
>
> If there really are devices which cannot handle reading at all, and
> cannot be reset to a sane state by later driver initialization, then a
> blacklist could be added for those devices. This should not be a common
> problem.

Yes, if these are dead registers, let's blacklist and move along. I
suspect though that these registers probably work fine if you access
them according to the device programming model, so blacklisting just
prevents full use through something like KVM device assignment. Thanks,

Alex

[Bjørn Mork]

Alex Williamson <alex.wi...@redhat.com> wrote:

>On Mon, 2013-03-18 at 18:20 +0100, Bjørn Mork wrote:
>> Alex Williamson <alex.wi...@redhat.com> writes:
>>
>> > At least for KVM the kernel fix is the addition of the vfio driver
>which
>> > gives us a non-sysfs way to do this. If this problem was found a
>few
>> > years later and we were ready to make the switch I'd support just
>> > removing these resource files. In the meantime we have userspace
>that
>> > depends on this interface, so I'm open to suggestions how to fix
>it.
>>
>> I am puzzled by a couple of things in this discussion:
>>
>> 1) do you seriously mean that a userspace application (any, not just
>> udevadm or qemu or whatever) should be able to read and write
>these
>> registers while the device is owned by a driver? How is that ever
>> going to work?
>
>The expectation is that the user doesn't mess with the device through
>pci-sysfs while it's running. This is really no different than config
>space or MMIO space in that respect.

But it is. That's the problem. As a user I expect to be able to run e.g "grep . /sys/devices/whatever/*" with no ill effects. This holds for config space or MMIO space. It does not for any reset-on-read register.

> You can use setpci to break your
>PCI card while it's used by the driver today. The difference is that
>MMIO spaces side-step the issue by only allowing mmap and config space
>is known not to have read side-effects.

Yes. And that is why there is no problem exporting those. This difference is fundamental.

>> 2) is it really so that a device can be so fundamentally screwed up
>by
>> reading some registers, that a later driver probe cannot properly
>> reinitialize it?
>
>Never underestimate how broken hardware can be,

True :)

> though in this case
>reading a device register seems to be causing a system hang/reset.

I understand that it does so if the ahci driver is bound to the device while reading the registers, but does it also hang the system with no bound driver? How does it do that? By killing the bus?

>> I would have thought that the solution to all this was to return
>-EINVAL
>> on any attemt to read or write these files while a driver is bound to
>> the device. If userspace is going to use the API, then the
>application
>> better unbind any driver first.
>>
>> Or? Am I missing something here?
>
>That doesn't really solve anything though. Let's pretend the resource
>files only work while the device is bound to pci-stub. Now what
>happens
>when you run this udevadm command as admin while it's in use by the
>userspace driver? All we've done is limit the scope of the problem.

Assuming that the system hangs without driver help and that this brokenness is widespread. I don't think any of those assumptions hold. Do they?

>> > If we want to blacklist this specific device, that's fine, but as
>others
>> > have pointed out it's really a class problem. Perhaps we report 1
>byte
>> > extra for the file length where EOF-1 is an enable byte? Is there
>> > anything else in file ops that we could use to make it slightly
>more
>> > complicated than open(), read() to access the device? Thanks,
>>
>> If there really are devices which cannot handle reading at all, and
>> cannot be reset to a sane state by later driver initialization, then
>a
>> blacklist could be added for those devices. This should not be a
>common
>> problem.
>
>Yes, if these are dead registers, let's blacklist and move along. I
>suspect though that these registers probably work fine if you access
>them according to the device programming model, so blacklisting just
>prevents full use through something like KVM device assignment.

Well, if the device is that broken then I think it will require the kernel to police the device programming. I don't see how you can leave a bomb like that because it might be useful in a rare and very theoretical case.

Easier to just blacklist it...


Bjørn

[Robert Brown]

On 03/18/13 13:54, Alex Williamson wrote:

> On Mon, 2013-03-18 at 18:20 +0100, Bj�rn Mork wrote:
>> Alex Williamson <alex.wi...@redhat.com> writes:
>>
>>> At least for KVM the kernel fix is the addition of the vfio driver which
>>> gives us a non-sysfs way to do this. If this problem was found a few
>>> years later and we were ready to make the switch I'd support just
>>> removing these resource files. In the meantime we have userspace that
>>> depends on this interface, so I'm open to suggestions how to fix it.
>> I am puzzled by a couple of things in this discussion:
>>
>> 1) do you seriously mean that a userspace application (any, not just
>> udevadm or qemu or whatever) should be able to read and write these
>> registers while the device is owned by a driver? How is that ever
>> going to work?
> The expectation is that the user doesn't mess with the device through
> pci-sysfs while it's running. This is really no different than config
> space or MMIO space in that respect. You can use setpci to break your
> PCI card while it's used by the driver today. The difference is that
> MMIO spaces side-step the issue by only allowing mmap and config space
> is known not to have read side-effects.
>
>> 2) is it really so that a device can be so fundamentally screwed up by
>> reading some registers, that a later driver probe cannot properly
>> reinitialize it?
> Never underestimate how broken hardware can be, though in this case
> reading a device register seems to be causing a system hang/reset.

The real problem is that PCI devices can be bus masters, which means
they can screw up *ANYTHING* (almost)!

[Alex Williamson]

As a non-admin user you can

> > You can use setpci to break your
> >PCI card while it's used by the driver today. The difference is that
> >MMIO spaces side-step the issue by only allowing mmap and config space
> >is known not to have read side-effects.
>
> Yes. And that is why there is no problem exporting those. This
> difference is fundamental.

So how do we side-step the problem with I/O port registers? If we
remove them then KVM needs to run with iopl which is a pretty serious
security hole should QEMU be exploited. We could activate the resource
files only when the device is bound to pci-assign, but that only limits
the scope and might break UIO drivers. We could modify the file to have
an enable sequence, but we can't do this without breaking current
userspace. As I mentioned, the VFIO driver is intended to replace KVM's
use of these files, but we're not ready to rip it out, perhaps not even
ready to declare it deprecated.

> >> 2) is it really so that a device can be so fundamentally screwed up
> >by
> >> reading some registers, that a later driver probe cannot properly
> >> reinitialize it?
> >
> >Never underestimate how broken hardware can be,
>
> True :)
>
> > though in this case
> >reading a device register seems to be causing a system hang/reset.
>
> I understand that it does so if the ahci driver is bound to the device
> while reading the registers, but does it also hang the system with no
> bound driver? How does it do that? By killing the bus?

I don't know, Myron?

> >> I would have thought that the solution to all this was to return
> >-EINVAL
> >> on any attemt to read or write these files while a driver is bound to
> >> the device. If userspace is going to use the API, then the
> >application
> >> better unbind any driver first.
> >>
> >> Or? Am I missing something here?
> >
> >That doesn't really solve anything though. Let's pretend the resource
> >files only work while the device is bound to pci-stub. Now what
> >happens
> >when you run this udevadm command as admin while it's in use by the
> >userspace driver? All we've done is limit the scope of the problem.
>
> Assuming that the system hangs without driver help and that this
> brokenness is widespread. I don't think any of those assumptions hold.
> Do they?

I thought it was true that for this device a system hang happened
regardless of the host driver, but haven't seen the original bug report.
As for widespread, this is the first I've heard of problems in the 2.5+
years that we've supported these I/O port resource files. The rest is
probably just FUD about random userspace apps trolling through device
registers.

> >> > If we want to blacklist this specific device, that's fine, but as
> >others
> >> > have pointed out it's really a class problem. Perhaps we report 1
> >byte
> >> > extra for the file length where EOF-1 is an enable byte? Is there
> >> > anything else in file ops that we could use to make it slightly
> >more
> >> > complicated than open(), read() to access the device? Thanks,
> >>
> >> If there really are devices which cannot handle reading at all, and
> >> cannot be reset to a sane state by later driver initialization, then
> >a
> >> blacklist could be added for those devices. This should not be a
> >common
> >> problem.
> >
> >Yes, if these are dead registers, let's blacklist and move along. I
> >suspect though that these registers probably work fine if you access
> >them according to the device programming model, so blacklisting just
> >prevents full use through something like KVM device assignment.
>
> Well, if the device is that broken then I think it will require the
> kernel to police the device programming. I don't see how you can leave
> a bomb like that because it might be useful in a rare and very
> theoretical case.
>
> Easier to just blacklist it...

Easier, yes. But it likely just kicks the problem down the road until
the next device. Thanks,

Alex

[Robert Hancock]

On 03/16/2013 07:03 PM, 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.

lspci doesn't randomly attempt to access device registers, AFAIK..

A PCI quirk implies there is something wrong with this device in
particular. This isn't the case. The device responds properly when it's
accessed as intended. The problem is that udevadm (or other processes,
like a random grep through sysfs for example) is effectively reading
registers willy-nilly. This is absolutely not safe to do on many devices
- and certainly not while a driver is attached to the device and has
claimed the port or MMIO regions that are being accessed. Blocking
access through these files to a device with an active driver that's
claimed the regions would significantly reduce the chances of something
like this causing problems.

[Robert Hancock]

On Mon, Mar 18, 2013 at 8:03 PM, Greg KH <gre...@linuxfoundation.org> wrote:

> On Mon, Mar 18, 2013 at 07:54:09PM -0600, Robert Hancock wrote:
>> On 03/16/2013 07:03 PM, 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.
>>
>> lspci doesn't randomly attempt to access device registers, AFAIK..
>

> Have you read the man page for the '-xxx' option to lspci? lspci can be
> quite intrusive, and I used to have a number of systems that it would
> trash very easily if you ran it on them as root.

> Then we need to fix that!
>
> In the kernel!
>
> Don't try to gloss over the problem by changing one random userspace
> program, you will never catch them all. Fix the root problem here
> people, that's all I'm asking for.

>
>> Blocking access through these files to a device with an active driver
>> that's claimed the regions would significantly reduce the chances of
>> something like this causing problems.
>

> Great, that's one possible solution, the other is just not creating the
> files at all for known problem devices, right?

I don't think one can reasonably enumerate all problem devices. There
are probably countless devices which can potentially break if their
resources (especially IO ports) are read in unexpected ways. Aside
from devices like this one, which apparently don't like certain IO
ports being read with certain access widths, there's every device in
existence with read-to-reset type registers. The fix to this needs to
apply to all devices.

>
> My main point here is, you aren't going to fix this in userspace, fix it
> in the kernel.

The kernel can help the situation by blocking access to devices with
an active driver, but it can't fix all cases. Suppose the device has
no driver loaded yet, how is the kernel supposed to tell the
difference between software with a legitimate need to access these
files for virtualization device assignment, etc. and something like
udevadm or a random grep command that's reading the files without any
idea what it's doing? udevadm does need to be fixed to avoid accessing
these files because it's unnecessary and dangerous.

[Greg KH]

On Mon, Mar 18, 2013 at 07:54:09PM -0600, Robert Hancock wrote:

> On 03/16/2013 07:03 PM, 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.
>
> lspci doesn't randomly attempt to access device registers, AFAIK..

Have you read the man page for the '-xxx' option to lspci? lspci can be
quite intrusive, and I used to have a number of systems that it would
trash very easily if you ran it on them as root.

Then we need to fix that!

In the kernel!

Don't try to gloss over the problem by changing one random userspace
program, you will never catch them all. Fix the root problem here
people, that's all I'm asking for.

> Blocking access through these files to a device with an active driver
> that's claimed the regions would significantly reduce the chances of
> something like this causing problems.

Great, that's one possible solution, the other is just not creating the
files at all for known problem devices, right?

My main point here is, you aren't going to fix this in userspace, fix it
in the kernel.

greg k-h

[Greg KH]

On Mon, Mar 18, 2013 at 08:09:22PM -0600, Robert Hancock wrote:
> > Great, that's one possible solution, the other is just not creating the
> > files at all for known problem devices, right?
>
> I don't think one can reasonably enumerate all problem devices. There
> are probably countless devices which can potentially break if their
> resources (especially IO ports) are read in unexpected ways. Aside
> from devices like this one, which apparently don't like certain IO
> ports being read with certain access widths, there's every device in
> existence with read-to-reset type registers. The fix to this needs to
> apply to all devices.
>
> >
> > My main point here is, you aren't going to fix this in userspace, fix it
> > in the kernel.
>
> The kernel can help the situation by blocking access to devices with
> an active driver, but it can't fix all cases. Suppose the device has
> no driver loaded yet, how is the kernel supposed to tell the
> difference between software with a legitimate need to access these
> files for virtualization device assignment, etc. and something like
> udevadm or a random grep command that's reading the files without any
> idea what it's doing? udevadm does need to be fixed to avoid accessing
> these files because it's unnecessary and dangerous.

Are you going to also fix grep? bash? cat?

Come on, be realistic. If these files are so dangerous then they need
to just be removed entirely from the kernel. You aren't going to be
able to patch grep for this.

greg k-h

[Robert Hancock]

Well, clearly not. Although accessing this file with grep, etc. is
really just another way root can shoot themselves in the foot, it
would be nice if this functionality could be provided in a way that
didn't leave this kind of exposed land mine.

[Myron Stowe]

Yes - the system hangs when BAR1's (and likely BAR3's) I/O port space is
read.

Here are the details that I've been able to put together from the two
linux-pci threads and various online sources -


From Robert Hancock - "... BAR5 is the MMIO region used by the AHCI
driver. BARs 0-4 are the legacy SFF-compatible ATA ports. Nothing
should be messing with those IO ports while AHCI is enabled. ..." This
likely explains why the system boots and runs fine as long as the
'udevadm ...' command is *not* ran (i.e. the driver never accesses the
I/O port BARs).

Using a SATA controller I have access to as an example for the details
(Note: I do not have access to a system with the Marvell 9125 device):
00:1f.2 SATA controller: Intel Corporation 5 Series/3400 Series Chipset 6 port SATA AHCI Controller (rev 06) (prog-if 01 [AHCI 1.0])
Subsystem: Lenovo Device 2168
Region 0: I/O ports at 1860 [size=8]
Region 1: I/O ports at 1814 [size=4]
Region 2: I/O ports at 1818 [size=8]
Region 3: I/O ports at 1810 [size=4]
Region 4: I/O ports at 1840 [size=32]
Region 5: Memory at f2827000 (32-bit, non-prefetchable) [size=2K]

I/O port registers [1][2]:
Primary IDE controller [0x1860-0x1867; 0x1814-0x1817]
BAR0 Base address for the command block registers for ATA Channel X
0x1860 (Read/Write): Data Register
0x1861 (Read): Error Register
0x1861 (Write): Features Register
0x1862 (Read/Write): Sector Count Register
0x1863 (Read/Write): LBA Low Register
0x1864 (Read/Write): LBA Mid Register
0x1865 (Read/Write): LBA High Register
0x1866 (Read/Write): Drive/Head Register
0x1867 (Read): Status Register
0x1867 (Write): Command Register
BAR1* Base address for the control register for ATA Channel X
0x1814 Reserved
0x1815 Reserved
0x1816 (Read): Alternate Status Register
0x1816 (Write): Device Control Register
0x1817 Reserved

* The base must be Dword aligned; a PCI requirement. The Device Control
and Alternate Status Registers are at ofset 0x2 from this base.

[1] www.t13.org/documents/UploadedDocuments/project/d1510r1-Host-Adapter.pdf
[2] lateblt.tripod.com/atapi.htm

From Xiangliang - executing 'udevadm ...' causes a 32-bit I/O port read
to BAR1's region. This is shown by the BE (Byte Enable) value of
0x1111. So apparently reads to this region that include any of reserved
Bytes causes "the chip will go bad."

So, only a Byte access at offset 2 is successful. I have not been able
to get any more details as to the exact cause of the hang. I would have
thought that the PCI transaction would have just timed out, or errored
out, or something but apparently the platform ends up hanging.

It appears that this device did not implement the reserved registers
such that they would return 0 on reads or something more similarly sane.

Since BARs 2 and 3 are not 0, indicating the device only supports one
channel, I expect the same issue will occur when accessing BAR3. Again,
I do not have access to a system with this device to test with.

[Myron Stowe]

Sorry - that wasn't very explicit. Just accessing BAR1's region as
udevadm does is enough to hang the system - even when no driver is
bound.