From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v8][PATCH 02/17] introduce XEN_DOMCTL_set_rdm Date: Tue, 09 Dec 2014 10:38:07 +0800 Message-ID: <5486608F.1050700@intel.com> References: <1417425875-9634-1-git-send-email-tiejun.chen@intel.com> <1417425875-9634-3-git-send-email-tiejun.chen@intel.com> <54808CC3020000780004CCC8@mail.emea.novell.com> <54853FE3.9030703@intel.com> <548572B5020000780004D9BC@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <548572B5020000780004D9BC@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, tim@xen.org, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/12/8 16:43, Jan Beulich wrote: >>>> On 08.12.14 at 07:06, wrote: >> On 2014/12/4 23:33, Jan Beulich wrote: >>>>>> On 01.12.14 at 10:24, wrote: >> Looks this could be fine, >> >> d->arch.hvm_domain.pci_force = xdsr->flags & PCI_DEV_RDM_CHECK; > > Which is correct only because PCI_DEV_RDM_CHECK happens to be > 1. Such hidden dependencies shouldn't be introduced though, in > particular to avoid others then cloning the code for a flag that's not > 1. The canonical form (found in many places throughout the treei Right. > > d->arch.hvm_domain.pci_force = !!(xdsr->flags & PCI_DEV_RDM_CHECK); Fixed. > >>>> + d->arch.hvm_domain.pcidevs = NULL; >>>> + >>>> + if ( xdsr->num_pcidevs ) >>>> + { >>>> + pcidevs = xmalloc_array(xen_guest_pcidev_info_t, >>>> + xdsr->num_pcidevs); >>> >>> New domctl-s must not represent security risks: xdsr->num_pcidevs >>> can be (almost) arbitrarily large - do you really want to allow such >>> huge allocations? A reasonable upper bound could for example be >> >> Sorry, as you know this num_pcidevs is from tools, and actually it share >> that result from that existing hypercall, assign_device, while parsing >> 'pci=[]', so I couldn't understand why this can be (almost" arbitrarily >> large. > > You imply well behaved tools, which you shouldn't when viewing > things from a security perspective. > >>> the total number of PCI devices the hypervisor knows about. >> >> I take a quick look at this but looks we have no this exact value that >> we can get directly. > > You need some upper bound. Whether you introduce a properly In theory, we may have at most the number, domain(16bit) x bus(8bit) x device(5bit) x function(3bit), 2^16 x 2^8 x 2^5 x 2^3 = 0x1000000, so could we define a macro like this, #define PCI_DEVICES_NUM_UP 0x1000000 > maintained count or a suitable estimate thereof doesn't matter. > >>>> --- a/xen/include/asm-x86/hvm/domain.h >>>> +++ b/xen/include/asm-x86/hvm/domain.h >>>> @@ -90,6 +90,10 @@ struct hvm_domain { >>>> /* Cached CF8 for guest PCI config cycles */ [snip] > > I really didn't necessarily mean individual comments - one for the whole > group would suffice. > > Also I don't think pci_force is really the right name - all_pcidevs or > some such would seem more suitable following the entire series. > And finally, I'm generally advocating for avoiding redundant data > items - I'm sure this "all" notion can be encoded reasonable with > just the other two field (and a suitable checking macro). Yeah. > Are you saying this way? #define PCI_DEVS_NUM_UP 0x1000000 #define ALL_PCI_DEVS (0x1 << 31) #define is_all_pcidevs(d) ((d)->arch.hvm_domain.num_pcidevs & ALL_PCI_DEVS) #define is_valid_pcidevs_num(d) \ ((d)->arch.hvm_domain.num_pcidevs <= PCI_DEVS_NUM_UP) /* * num_pcidevs: * bit31 indicates if all devices need to be checked/reserved * Reserved Device Memory. * bit30 ~ bit25 are reserved now. * bit24 ~ bit0 represent actually how many pci devices we * need to check/reserve Reserved Device Memory. They are * valid just when bit31 is zero. * * pcidevs represent these pci device instances associated to * bit42 ~ bit0. */ uint32_t num_pcidevs; struct xen_guest_pcidev_info *pcidevs; Thanks Tiejun