Re: [PATCH V2] device-assignment pci: correct pci config size default for cap version 2 endpoints

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 25, 2011 at 02:14:28PM -0600, Alex Williamson wrote:
> On Sun, 2011-07-24 at 11:36 +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 21, 2011 at 11:02:53AM -0600, Alex Williamson wrote:
> > > This is crazy, why would we only test this for PCI_CAP_ID_EXP?  If the
> > > test is going to go in device-assignment, we need to wrap
> > > pci_add_capability and do it for all callers.  However, maybe we can get
> > > MST to take it in pci_add_capability() if we make the test more
> > > complete...  something like:
> > > 
> > > if ((pos < 256 && size > 256 - pos) ||
> > >     (pci_config_size() > 256 && pos > 256 &&
> > >      size > pci_config_size() - pos)) {
> > >    ... badness
> > > 
> > > Thanks,
> > > 
> > > Alex
> > 
> > We expect device assignment to be able to corrupt
> > guest memory but not qemu memory. So we must validate
> 
> Gee, thanks.

Ugh, sorry, not device assignment per se.
I really meant an assigned device controlled by
a malicious guest.

> > whatever we get from the device, and I think this
> > validation belongs in device-assignment.c:
> > 
> > IOW I think input should be validated where it's
> > input, while we still know it's untrusted,
> > instead of relying on core to validate parameters.
> > 
> > Makes sense?
> 
> No, not really.  Why should the core "trust" anything?

We generally don't validate most function parameters.
We trust callers to a level because they can corrupt our memory anyway.

> This is a basic, simply sanity test, why push it out to the leaf
> driver when pushing it
> into the core provides the sanity test for everyone?  Is it beneath an
> emulated driver to pass such a test?  Thanks,
> 
> Alex

It's easy to add this to the core but please don't
rely on this: functions validating parameters is
a debugging aid. Validating untrusted input from a guest-controlled
device is a security feature.


-- 
MST