Re: [Xen-devel] [v8][PATCH 02/17] introduce XEN_DOMCTL_set_rdm

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.12.14 at 07:06, <tiejun.chen@xxxxxxxxx> wrote:
> On 2014/12/4 23:33, Jan Beulich wrote:
>>>>> On 01.12.14 at 10:24, <tiejun.chen@xxxxxxxxx> 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

    d->arch.hvm_domain.pci_force = !!(xdsr->flags & PCI_DEV_RDM_CHECK);

>>> +        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
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 */
>>>       uint32_t                pci_cf8;
>>>
>>> +    bool_t                  pci_force;
>>> +    uint32_t                num_pcidevs;
>>> +    struct xen_guest_pcidev_info      *pcidevs;
>>
>> Without a comment all these field names are pretty questionable.
> 
> Yeah, I try to add some comments,
> 
>      /* A global flag, we need to check/reserve all Reserved Device 
> Memory. */
>      bool_t                  pci_force;
>      /*
>       * If pci_force is 0, this represents how many pci devices we need
>       * to check/reserve Reserved Device Memory.
>       * If pci_force is 1, this is always 0.
>       */
>      uint32_t                num_pcidevs;
>      /* This represents those pci devices instances when num_pcidevs != 
> 0. */
>      struct xen_guest_pcidev_info      *pcidevs;

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).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel