On Tue, 2011-07-26 at 14:42 +0300, Michael S. Tsirkin wrote:
> 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.
Huh? A malicious guest can't redefine PCI config space for an assigned
device. This is an initialization phase of parsing the device config
space.
> > > 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 device init code though, adding sanity checks doesn't hurt
anything and makes sure emulated device developers don't inadvertently
ignore PCI requirements.
> > 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.
Some might call it robustness to validate parameters and protect qemu
against all callers. I hadn't realized that device assignment was such
a second class citizen. Thanks,
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
