Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 06/22/2017 05:40 AM, George Dunlap wrote:
> On 22/06/17 08:05, Jan Beulich wrote:
> > > > > On 21.06.17 at 18:36, <george.dunlap@xxxxxxxxxx> wrote:
> > > On 21/06/17 16:59, Jan Beulich wrote:
> > > > > > > On 21.06.17 at 16:38, <george.dunlap@xxxxxxxxxx> wrote:
> > > > > On 21/06/17 11:08, Jan Beulich wrote:
> > > > > > So far callers of the libxc interface passed in a domain ID which was
> > > > > > then ignored in the hypervisor. Instead, make the hypervisor honor it
> > > > > > (accepting DOMID_INVALID to obtain original behavior), allowing to
> > > > > > query whether a device is assigned to a particular domain. Ignore the
> > > > > > passed in domain ID at the libxc layer instead, in order to not break
> > > > > > existing callers. New libxc functions would need to be added if callers
> > > > > > wanted to leverage the new functionality.
> > > > > I don't think your modified description matches the name of the call at all.
> > > > >
> > > > > It looks like the callers expect "test_assign_device" to answer the
> > > > > question: "Can I assign a device to this domain"?
> > > > I don't think so - the question being answered by the original
> > > > operation is "Is this device assigned to any domain?" with the
> > > > implied inverse "Is this device available to be assigned to some
> > > > domain (i.e. it is currently unassigned or owned by Dom0)?"
> > > If the question were "Is this device assigned to any domain?", then I
> > > would expect:
> > > 1. The return value to be a boolean
> > > 2. It would always return, "No it's not assigned" in the case where
> > > there is no IOMMU.
> > >
> > > However, that's not what happens:
> > > 1. It returns "success" if there is an IOMMU and the device is *not*
> > > assigned, and returns an error if the device is assigned
> > > 2. It returns an error if there is no IOMMU.
> > >
> > > The only place in the code this is called 'for real' in the tree is in
> > > libxl_pci.c:libxl__device_pci_add()
> > >
> > >      if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
> > >          rc = xc_test_assign_device(ctx->xch, domid,
> > > pcidev_encode_bdf(pcidev));
> > >          if (rc) {
> > >              LOGD(ERROR, domid,
> > >                   "PCI device %04x:%02x:%02x.%u %s?",
> > >                   pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
> > >                   errno == ENOSYS ? "cannot be assigned - no IOMMU"
> > >                   : "already assigned to a different guest");
> > >              goto out;
> > >          }
> > >      }
> > >
> > > Here 'domid' is the domain to which libxl wants to assign the device.
> > > So libxl is now asking Xen, "Am I allowed to assign device $bdf to
> > > domain $domain?"
> > >
> > > Your description provides the *algorithm* by which Xen normally provides
> > > an answer: that is, normally the only thing Xen cares about is that it
> > > hasn't already been assigned to a domain.  But it still remains the case
> > > that what libxl is asking is, "Can I assign X to Y?"
> > Taking the log message into account that you quote, I do not
> > view the code's intention to be what you describe.
> Well, I'm not sure what to say, because in my view the log message
> supports my view. :-)  Note that there are two errors, both explaining
> why the domain cannot be assigned -- one is "no IOMMU", one is "already
> assigned to a different guest".
>
> Yes, at the moment it doesn't have a separate message for -EPERM (which
> is presumably what XSM would return if there were some other problem).
> But it also doesn't correctly report other potential errors: -ENODEV if
> you try to assign a DT device on a PCI-based system, or a PCI device on
> a DT-based system.  (Apparently we also retirn -EINVAL if you included
> inappropriate flags, *or* if the device didn't exist, *or* if the device
> was already assigned somehwere else.  As long as we're re-painting
> things we should probably change this as well.)
>
> But to make test_assign_device answer the question, "Is this assigned to
> a domU?", you'd have to have it return SUCCESS when there is no IOMMU
> (since the device is not, in fact, assigned to a domU); and thus libxl
> would have to make a separate call to find out if an IOMMU was present.
>
> > > > > It looks like it's meant to be used in XSM environments, to allow a
> > > > > policy to permit or forbid specific guests to have access to specific
> > > > > devices.  On a default (non-XSM) system, the answer to that question
> > > > > doesn't depend on the domain it's being assigned to, but only whether
> > > > > the device is already assigned to another domain; but on XSM systems the
> > > > > logic can presumably be more complicated.
> > > > >
> > > > > That sounds like a perfectly sane semantic to me, and this patch removes
> > > > > that ability.
> > > > And again I don't think so: Prior to the patch, do_domctl() at its
> > > > very top makes sure to entirely ignore the passed in domain ID.
> > > > This code sits ahead of the XSM check, so XSM has no way of
> > > > knowing which domain has been specified by the caller.
> > > Right, I see that now.
> > >
> > > Still, I assert that the original hypercall semantics is a very useful
> > > one, and what you're doing is changing the hypercall such that the
> > > question can no longer be asked.  It would be better to extend things so
> > > that XSM can actually deny device assignment based on both the bdf and
> > > the domain.
> > >
> > > Do you have a particular use case in mind for your alternate hypercall?
> > No - I'm open to any change to it which makes the currently ignored
> > argument no longer ignored, without breaking existing (known and
> > unknown) callers of the libxc wrapper. I.e. I'm in no way opposed to
> > make it work the way you think it was originally meant to work; it is
> > just that given its current use I've come to a different conclusion as
> > to what the original intention may have been.
> So the libxc library interface is not meant to be stable.  Before I
> looked at how libxl was using it, I was going to say that we should just
> remove the domid argument from that function entirely, rather than
> labelling it "unused".
>
> I suggest we ask the toolstack maintainers what kind of a function they
> think would be most useful, and then we can implement that.
>
> So, Wei and Ian (and Daniel if you're around):
>
> At the moment, xc_test_assign_device() accepts both a domid and a device
> identifier.  It will return -ENOSYS if there is no IOMMU, and
>
> The domid, however, is ignored by Xen, and there is no possibility even
> of XSM/Flask paying any attention to it because the domid is not passed
> to the XSM hook.
>
> Jan thinks the color of this shed is ugly and wants to repaint it.  I
> agree that the color of this shed is ugly, but think a different color
> would be more suitable.  Since the function is ultimately mainly for the
> benefit of toolstacks like libxl, we'd like your input:
>
> Option 1: Make xc_test_assign_device() explicitly take only a device
> identifier.  Add another xc function which takes a domain argument,
> which will return true if the BDF is assigned to that particular domain,
> and false otherwise.
>    1a: Leave the domid argument, but add a comment specifying it as ignored
>    1b: Remove the domid argument.
>
> Option 2: Pass the domain to the XSM callback, enabling XSM / Flask
> policies that can forbid specific devices from being assigned to
> specific guests.
>
> NB that neither of us has a particular requirement for the proposed
> additional functionality ("Device X is assigned to domid Y" in the case
> of Option 1, or "Flask policy can allow or forbid specific devices to
> specific domains" in the case of Option 2).
>
> Any preferences?
>
>   -George
One caveat to add to option 2: the XSM permission check done by test_assign
would only cover the PCI device resource (same as assign_device).  There
are also XSM checks that occur when assigning the MMIO and IO port regions
assigned to the device (defined by the BARs), and these ranges are not
passed to the hypervisor during the test function.  A proper test_assign
function as described above would also need to check the XSM permissions
for them, which would require adding test_io{port,mem,q}_permission
functions too.

Alternatively, you could assume that the PCI device and its associated
resources all have the same label (which will be almost always be true in a
properly configured system) and just use this as an early bail out to avoid
user mistakes.

--
Daniel De Graaf
National Security Agency

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