Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1460099 > unrolled thread

Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-08-11 02:00 +0200
    Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access Alexander Duyck <alexander.duyck@gmail.com> - 2016-08-11 21:00 +0200
      Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access Alex Williamson <alex.williamson@redhat.com> - 2016-08-11 22:20 +0200
        Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-08-12 07:20 +0200
          Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access "Rustad, Mark D" <mark.d.rustad@intel.com> - 2016-08-15 20:10 +0200
            Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-08-16 00:30 +0200
              Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-08-16 00:40 +0200
                Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access "Rustad, Mark D" <mark.d.rustad@intel.com> - 2016-08-16 01:20 +0200
                  Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-08-16 02:40 +0200
      Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-08-16 03:50 +0200

#1460099 — Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access

On Wed, 2016-08-10 at 08:47 -0700, Alexander Duyck wrote:
> 
> The problem is if we don't do this it becomes possible for a guest to
> essentially cripple a device on the host by just accessing VPD
> regions that aren't actually viable on many devices. 

And ? We already can cripple the device in so many different ways
simpy because we have pretty much full BAR access to it...

>  We are much better off
> in terms of security and stability if we restrict access to what
> should be accessible. 

Bollox. I've heard that argument over and over again, it never stood
and still doesn't.

We have full BAR access for god sake. We can already destroy the device
in many cases (think: reflashing microcode, internal debug bus access
with a route to the config space, voltage/freq control ....).

We aren't protecting anything more here, we are just adding layers of
bloat, complication and bugs.

>  In this case what has happened is that the
> vendor threw in an extra out-of-spec block and just expected it to
> work.

Like vendors do all the time in all sort of places

I still completely fail to see the point in acting as a filtering
middle man.

> In order to work around it we just need to add a small function
> to drivers/pci/quirks.c that would update the VPD size reported so
> that it matches what the hardware is actually providing instead of
> what we can determine based on the VPD layout.
> 
> Really working around something like this is not much different than
> what we would have to do if the vendor had stuffed the data in some
> reserved section of their PCI configuration space.

It is, in both cases we shouldn't have VFIO or the host involved. We
should just let the guest config space accesses go through.

>   We end up needing
> to add special quirks any time a vendor goes out-of-spec for some
> one-off configuration interface that only they are ever going to use.

Cheers,
Ben.

[toc] | [next] | [standalone]

#1460741

On Wed, Aug 10, 2016 at 4:54 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2016-08-10 at 08:47 -0700, Alexander Duyck wrote:
>>
>> The problem is if we don't do this it becomes possible for a guest to
>> essentially cripple a device on the host by just accessing VPD
>> regions that aren't actually viable on many devices.
>
> And ? We already can cripple the device in so many different ways
> simpy because we have pretty much full BAR access to it...
>
>>  We are much better off
>> in terms of security and stability if we restrict access to what
>> should be accessible.
>
> Bollox. I've heard that argument over and over again, it never stood
> and still doesn't.
>
> We have full BAR access for god sake. We can already destroy the device
> in many cases (think: reflashing microcode, internal debug bus access
> with a route to the config space, voltage/freq control ....).
>
> We aren't protecting anything more here, we are just adding layers of
> bloat, complication and bugs.

To some extent I can agree with you.  I don't know if we should be
restricting the VFIO based interface the same way we restrict systemd
from accessing this region.  In the case of VFIO maybe we need to look
at a different approach for accessing this.  Perhaps we need a
privileged version of the VPD accessors that could be used by things
like VFIO and the cxgb3 driver since they are assumed to be a bit
smarter than those interfaces that were just trying to slurp up
something like 4K of VPD data.

>>  In this case what has happened is that the
>> vendor threw in an extra out-of-spec block and just expected it to
>> work.
>
> Like vendors do all the time in all sort of places
>
> I still completely fail to see the point in acting as a filtering
> middle man.

The problem is we are having to do some filtering because things like
systemd were using dumb accessors that were trying to suck down 4K of
VPD data instead of trying to parse through and read it a field at a
time.

>> In order to work around it we just need to add a small function
>> to drivers/pci/quirks.c that would update the VPD size reported so
>> that it matches what the hardware is actually providing instead of
>> what we can determine based on the VPD layout.
>>
>> Really working around something like this is not much different than
>> what we would have to do if the vendor had stuffed the data in some
>> reserved section of their PCI configuration space.
>
> It is, in both cases we shouldn't have VFIO or the host involved. We
> should just let the guest config space accesses go through.
>
>>   We end up needing
>> to add special quirks any time a vendor goes out-of-spec for some
>> one-off configuration interface that only they are ever going to use.
>
> Cheers,
> Ben.

If you have a suggestion on how to resolve this patches are always
welcome.  Otherwise I think the simpler approach to fixing this
without re-introducing the existing bugs is to just add the quirk.  I
will try to get to it sometime this weekend if nobody else does.  It
should be pretty straight foward, but I just don't have the time to
pull up a kernel and generate a patch right now.

- Alex

[toc] | [prev] | [next] | [standalone]

#1460776

On Thu, 11 Aug 2016 11:52:02 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> On Wed, Aug 10, 2016 at 4:54 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Wed, 2016-08-10 at 08:47 -0700, Alexander Duyck wrote:  
> >>
> >> The problem is if we don't do this it becomes possible for a guest to
> >> essentially cripple a device on the host by just accessing VPD
> >> regions that aren't actually viable on many devices.  
> >
> > And ? We already can cripple the device in so many different ways
> > simpy because we have pretty much full BAR access to it...
> >  
> >>  We are much better off
> >> in terms of security and stability if we restrict access to what
> >> should be accessible.  
> >
> > Bollox. I've heard that argument over and over again, it never stood
> > and still doesn't.
> >
> > We have full BAR access for god sake. We can already destroy the device
> > in many cases (think: reflashing microcode, internal debug bus access
> > with a route to the config space, voltage/freq control ....).
> >
> > We aren't protecting anything more here, we are just adding layers of
> > bloat, complication and bugs.  
> 
> To some extent I can agree with you.  I don't know if we should be
> restricting the VFIO based interface the same way we restrict systemd
> from accessing this region.  In the case of VFIO maybe we need to look
> at a different approach for accessing this.  Perhaps we need a
> privileged version of the VPD accessors that could be used by things
> like VFIO and the cxgb3 driver since they are assumed to be a bit
> smarter than those interfaces that were just trying to slurp up
> something like 4K of VPD data.
> 
> >>  In this case what has happened is that the
> >> vendor threw in an extra out-of-spec block and just expected it to
> >> work.  
> >
> > Like vendors do all the time in all sort of places
> >
> > I still completely fail to see the point in acting as a filtering
> > middle man.  
> 
> The problem is we are having to do some filtering because things like
> systemd were using dumb accessors that were trying to suck down 4K of
> VPD data instead of trying to parse through and read it a field at a
> time.

vfio isn't playing nanny here for the fun of it, part of the reason we
have vpd access functions is because folks have discovered that vpd
registers between PCI functions on multi-function devices may be
shared.  So pounding on vpd registers for function 1 may adversely
affect someone else reading from a different function.  This is a case
where I feel vfio needs to step in because if that's a user competing
with the host or two users stepping on each other, well that's exactly
what vfio tries to prevent.  A driver in userspace or a VM driver can't
very well determine these sorts of interactions when it only has
visibility to a subset of the functions and users and hardware folks
would throw a fit if I extended iommu groups to encompass all the
related devices rather than take the relatively simple step of
virtualizing these accesses and occasionally quirking devices that are
extra broken, as seems to be required here.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1460951

On Thu, 2016-08-11 at 14:17 -0600, Alex Williamson wrote:
> 
> vfio isn't playing nanny here for the fun of it, part of the reason we
> have vpd access functions is because folks have discovered that vpd
> registers between PCI functions on multi-function devices may be
> shared.  So pounding on vpd registers for function 1 may adversely
> > affect someone else reading from a different function.

A lot more than that may be shared... the bus for example. lots of
devices have backdoors into their own BARs, one partition can make its
BARs overlap the neighbour function, ... dang !

In the end, PCI assignment at a granularity smaller than a bus cannot
be done in a fully air tight way and shouldn't be relied upon in
environemnt where the isolation between partitions matters.

The only exception here is SR-IOV of course since it's designed
specifically so that the VFs can't be messed with.

>   This is a case
> where I feel vfio needs to step in because if that's a user competing
> with the host or two users stepping on each other, well that's exactly
> what vfio tries to prevent.  A driver in userspace or a VM driver can't
> very well determine these sorts of interactions when it only has
> visibility to a subset of the functions and users and hardware folks
> would throw a fit if I extended iommu groups to encompass all the
> related devices rather than take the relatively simple step of
> virtualizing these accesses and occasionally quirking devices that are
> extra broken, as seems to be required here.  Thanks,

We may want some kind of "strict" vs. "relaxed" model here to
differenciate the desktop user wanting to give a function to his/her
windows partition and doesn't care about strict isolation vs. the cloud
data center.

Cheers,
Ben.

[toc] | [prev] | [next] | [standalone]

#1463057

[Multipart message — attachments visible in raw view] — view raw

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> We may want some kind of "strict" vs. "relaxed" model here to
> differenciate the desktop user wanting to give a function to his/her
> windows partition and doesn't care about strict isolation vs. the cloud
> data center.

I don't think desktop users appreciate hangs any more than anyone else, and  
that is one of the symptoms that can arise here without the vfio  
coordination.

--
Mark Rustad, Networking Division, Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1463226

On Mon, 2016-08-15 at 17:59 +0000, Rustad, Mark D wrote:
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > 
> > We may want some kind of "strict" vs. "relaxed" model here to
> > differenciate the desktop user wanting to give a function to his/her
> > windows partition and doesn't care about strict isolation vs. the cloud
> > data center.
> 
> I don't think desktop users appreciate hangs any more than anyone else, and  
> that is one of the symptoms that can arise here without the vfio  
> coordination.

And can happen with it as well.... 

Put it this way, we have a hole the size of a football field and we are
spending all that time "plugging" it by putting a 3 foot gate in the
middle of it... ugh.

VFIO shouldn't try to get between the device driver and the HW, it doesn't
work. It cannot work.

Cheers,
Ben.

[toc] | [prev] | [next] | [standalone]

#1463233

On Tue, 2016-08-16 at 08:23 +1000, Benjamin Herrenschmidt wrote:
> > I don't think desktop users appreciate hangs any more than anyone else, and  
> > that is one of the symptoms that can arise here without the vfio  
> > coordination.
> 
> And can happen with it as well.... 

Oh and your response was completely besides the point I was trying
to make, just some passive aggressive noise, thank you.

The point was that if you want the sort of safety that we are trying
to aim for, without additional HW support, you basically have to do
the isolation on a granularity no smaller than a bridge/bus (with the
notable exception of SR-IOV of course).

Otherwise guest *will* be able to harm each other (or the host) in
all sorts of ways if anything because devices will have MMIO backdoors
into their own BAR space, or can be made to DMA to a neighbour, etc...

This is the only safe thing to do (and we are enforcing that on POWER
with our IOMMU groups).

Now that being said, if you want to keep the ability to assign 2 functions
of a device to different guests for your average non-critical desktop user,
that's where you may want to consider two models.

Generally speaking, filtering things for "safety" won't work.

Filtering things to work around bugs in existing guests to avoid crashes
is a different kettle of fish and could be justified but keep in mind that
in most cases a malicious guest will be able to exploit those HW flaws.

Assuming that a device coming back from a guest is functional and not
completely broken and can be re-used without a full PERST or power cycle
is a wrong assumption. It may or may not work, no amount of "filtering"
will fix the fundamental issue. If your HW won't give you access to PERST
well ... blame Intel for not specifying a standard way to generate it in
the first place :-)

Cheers,
Ben.

[toc] | [prev] | [next] | [standalone]

#1463263

[Multipart message — attachments visible in raw view] — view raw

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Filtering things to work around bugs in existing guests to avoid crashes
> is a different kettle of fish and could be justified but keep in mind that
> in most cases a malicious guest will be able to exploit those HW flaws.

Bugs in existing guests is an interesting case, but I have been focused on  
getting acceptable behavior from a properly functioning guest, in the face  
of hardware issues that can only be resolved in a single place.

I agree that a malicious guest can cause all kinds of havoc with  
directly-assigned devices. Consider a 4-port PHY chip on a shared MDIO bus,  
for instance. There is really nothing to be done about the potential for  
mischief with that kind of thing.

The VPD problem that I had been concerned about arises from a bad design in  
the PCI spec together with implementations that share the registers across  
functions. The hardware isn't going to change and I really doubt that the  
spec will either, so we address it the only place we can.

I am certain that we agree that not everything can or should be addressed  
in vfio. I did not mean to suggest it should try to address everything, but  
I think it should make it possible for correctly behaving guests to work. I  
think that is not unreasonable.

Perhaps the VPD range check should really just have been implemented for  
the sysfs interface, and left the vfio case unchecked. I don't know because  
I was not involved in that issue. Perhaps someone more intimately involved  
can comment on that notion.

> Assuming that a device coming back from a guest is functional and not
> completely broken and can be re-used without a full PERST or power cycle
> is a wrong assumption. It may or may not work, no amount of "filtering"
> will fix the fundamental issue. If your HW won't give you access to PERST
> well ... blame Intel for not specifying a standard way to generate it in
> the first place :-)

Yeah, I worry about the state that a malicious guest could leave a device  
in, but I consider direct assignment always risky anyway. I would just like  
it to at least work in the non-malicious guest cases.

I guess my previous response was really just too terse, I was just focused  
on unavoidable hangs and data corruption, which even were happening without  
any guest involvement. For me, guests were just an additional exposure of  
the same underlying issue.

With hindsight, it is easy to see that a standard reset would now be a  
pretty useful thing. I am sure that even if it existed, we would now have  
lots and lots of quirks around it as well! :-)

--
Mark Rustad, Networking Division, Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1463306

On Mon, 2016-08-15 at 23:16 +0000, Rustad, Mark D wrote:
> 
> Bugs in existing guests is an interesting case, but I have been focused on  
> getting acceptable behavior from a properly functioning guest, in the face  
> of hardware issues that can only be resolved in a single place.
> 
> I agree that a malicious guest can cause all kinds of havoc with  
> directly-assigned devices. Consider a 4-port PHY chip on a shared MDIO bus,  
> for instance. There is really nothing to be done about the potential for  
> mischief with that kind of thing.
> 
> The VPD problem that I had been concerned about arises from a bad design in  
> the PCI spec together with implementations that share the registers across  
> functions. The hardware isn't going to change and I really doubt that the  
> spec will either, so we address it the only place we can.

Right but as I mentioned, there are plenty of holes when it comes to
having multi function devices assigned to different guests, this is just
one of them.

Now, that being said, "working around" the bug to make the "non fully secure"
case work (in my example case the "desktop user") is probably an ok workaround
as long as we fully agree that this is all it is, it by no means provide
actual isolation or security.
 
> >   >   rtain that we agree that not everything can or should be addressed  
> in vfio. I did not mean to suggest it should try to address everything, but  
> I think it should make it possible for correctly behaving guests to work. I  
> think that is not unreasonable.

Again as long as there is no expectation of security here, such as a data
center giving PCI access to some devices.

> > Perhaps the VPD range check should really just have been implemented for  
> the sysfs interface, and left the vfio case unchecked. I don't know because  
> I was not involved in that issue. Perhaps someone more intimately involved  
> can comment on that notion.

That would have been my preferred approach... I think VFIO tries to do too much
which complicates things, causes other bugs, without briging actual safety. I
don't think it should stand in between a guest driver and its device unless
absolutely necessary to provide the functionality due to broken HW or design,
but with the full understanding that doing so remains unsafe from an isolation
standpoint.

> > > Assuming that a device coming back from a guest is functional and not
> > completely broken and can be re-used without a full PERST or power cycle
> > is a wrong assumption. It may or may not work, no amount of "filtering"
> > will fix the fundamental issue. If your HW won't give you access to PERST
> > well ... blame Intel for not specifying a standard way to generate it in
> > the first place :-)
> 
> Yeah, I worry about the state that a malicious guest could leave a device  
> in, but I consider direct assignment always risky anyway. I would just like  
> it to at least work in the non-malicious guest cases.

Right. Only SR-IOV which is somewhat designed for assignment is reasonably safe
in the general case.

On server POWER boxes, we have isolation at the bus level with usual per-slot
PERST control so we are in a much better situation but we also for all the
above reasons, only allow a slot granularity for pass-through.

> > I guess my previous response was really just too terse, I was just focused  
> on unavoidable hangs and data corruption, which even were happening without  
> any guest involvement. For me, guests were just an additional exposure of  
> the same underlying issue.
> 
> With hindsight, it is easy to see that a standard reset would now be a  
> pretty useful thing. I am sure that even if it existed, we would now have  
> lots and lots of quirks around it as well! :-)

Hehe yes, well, HW for you ...

Cheers,
Ben.

> > --
> Mark Rustad, Networking Division, Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1463341

On 12/08/16 04:52, Alexander Duyck wrote:
> On Wed, Aug 10, 2016 at 4:54 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>> On Wed, 2016-08-10 at 08:47 -0700, Alexander Duyck wrote:
>>>
>>> The problem is if we don't do this it becomes possible for a guest to
>>> essentially cripple a device on the host by just accessing VPD
>>> regions that aren't actually viable on many devices.
>>
>> And ? We already can cripple the device in so many different ways
>> simpy because we have pretty much full BAR access to it...
>>
>>>  We are much better off
>>> in terms of security and stability if we restrict access to what
>>> should be accessible.
>>
>> Bollox. I've heard that argument over and over again, it never stood
>> and still doesn't.
>>
>> We have full BAR access for god sake. We can already destroy the device
>> in many cases (think: reflashing microcode, internal debug bus access
>> with a route to the config space, voltage/freq control ....).
>>
>> We aren't protecting anything more here, we are just adding layers of
>> bloat, complication and bugs.
> 
> To some extent I can agree with you.  I don't know if we should be
> restricting the VFIO based interface the same way we restrict systemd
> from accessing this region.  In the case of VFIO maybe we need to look
> at a different approach for accessing this.  Perhaps we need a
> privileged version of the VPD accessors that could be used by things
> like VFIO and the cxgb3 driver since they are assumed to be a bit
> smarter than those interfaces that were just trying to slurp up
> something like 4K of VPD data.
> 
>>>  In this case what has happened is that the
>>> vendor threw in an extra out-of-spec block and just expected it to
>>> work.
>>
>> Like vendors do all the time in all sort of places
>>
>> I still completely fail to see the point in acting as a filtering
>> middle man.
> 
> The problem is we are having to do some filtering because things like
> systemd were using dumb accessors that were trying to suck down 4K of
> VPD data instead of trying to parse through and read it a field at a
> time.
> 
>>> In order to work around it we just need to add a small function
>>> to drivers/pci/quirks.c that would update the VPD size reported so
>>> that it matches what the hardware is actually providing instead of
>>> what we can determine based on the VPD layout.
>>>
>>> Really working around something like this is not much different than
>>> what we would have to do if the vendor had stuffed the data in some
>>> reserved section of their PCI configuration space.
>>
>> It is, in both cases we shouldn't have VFIO or the host involved. We
>> should just let the guest config space accesses go through.
>>
>>>   We end up needing
>>> to add special quirks any time a vendor goes out-of-spec for some
>>> one-off configuration interface that only they are ever going to use.
>>
>> Cheers,
>> Ben.
> 
> If you have a suggestion on how to resolve this patches are always
> welcome.  Otherwise I think the simpler approach to fixing this
> without re-introducing the existing bugs is to just add the quirk.  I
> will try to get to it sometime this weekend if nobody else does.  It
> should be pretty straight foward, but I just don't have the time to
> pull up a kernel and generate a patch right now.


How exactly is mine - https://lkml.org/lkml/2016/8/11/200 - bad (except
missing rb/ab from Chelsio folks)? Thanks.


-- 
Alexey

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web