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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2014/12/8 16:43, Jan Beulich wrote:
> > > > 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
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

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