From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH V2] device-assignment pci: correct pci config size default for cap version 2 endpoints Date: Tue, 26 Jul 2011 14:42:43 +0300 Message-ID: <20110726114243.GC30261@redhat.com> References: <20110721163733.661.22067.stgit@dddsys0.bos.redhat.com> <1311267773.26867.7.camel@ul30vt> <20110724083637.GE24483@redhat.com> <1311624869.2653.154.camel@bling.home> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Donald Dutile , kvm@vger.kernel.org To: Alex Williamson Return-path: Received: from mx1.redhat.com ([209.132.183.28]:8585 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752284Ab1GZLmM (ORCPT ); Tue, 26 Jul 2011 07:42:12 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p6QBgCp3011790 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 26 Jul 2011 07:42:12 -0400 Content-Disposition: inline In-Reply-To: <1311624869.2653.154.camel@bling.home> Sender: kvm-owner@vger.kernel.org List-ID: 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