Re: [Xen-devel] [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 10/20/2017 02:14 AM, Jan Beulich wrote:
> > > > On 19.10.17 at 19:36, <dgdegra@xxxxxxxxxxxxx> wrote:
> > On 10/19/2017 07:58 AM, Jan Beulich wrote:
> > > > > > On 19.10.17 at 04:36, <blackskygg@xxxxxxxxx> wrote:
> > > > --- a/xen/include/xsm/dummy.h
> > > > +++ b/xen/include/xsm/dummy.h
> > > > @@ -516,7 +516,8 @@ static XSM_INLINE int
> > xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
> > > >    static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
> > *d, struct domain *t)
> > > >    {
> > > >        XSM_ASSERT_ACTION(XSM_TARGET);
> > > > -    return xsm_default_action(action, d, t);
> > > > +    return xsm_default_action(action, current->domain, d) ?:
> > > > +        xsm_default_action(action, current->domain, t);
> > > >    }
> > > When all three domains are different, how does the changed
> > > policy reflect the original "d has privilege over t" requirement?
> > > I understand you want to relax the current condition, but this
> > > shouldn't come at the price of granting access when access
> > > should be denied. Nor the inverse - the current domain not
> > > having privilege over both does also not mean d doesn't have
> > > the necessary privilege over t.
> > >
> > > I continue to think that you can't validly retrofit the new
> > > intended functionality onto the existing hypercall, even if
> > > nothing except the permission check needs to be different.
> > >
> > > Jan
> > If this operation is going to be allowed at all (and I agree it has
> > valid use cases), then there won't be a privilege relationship between
> > (d) and (t) to check - they'll both be (somewhat related) domUs as far
> > as Xen can tell.  If this hypercall isn't used, adding a new hypercall
> > (subop) is the only way I'd see to do it - and that seems very redundant
> > as it'd need to do all the same checks except for the one about the
> > relationship between (d) and (t).  I don't see the reason why the
> > existing hypercall should deny being used for that purpose once it's
> > possible using other means.
> One problem is, as you mention here, ...
>
> > The only possible problem that springs to mind is a restricted kernel
> > interface (such as the one used by QEMU in dom0 that restricts to a
> > single target domain) that now doesn't realize it's relaying an
> > operation that also requires permission over (t) after only checking
> > that the origin is allowed to modify (d).
> ... the delegation of privilege checking responsibility to a
> possibly untrusted environment. Plus, as explained before,
> current callers expect privilege of d over t to be validated,
> which isn't happening anymore with the proposed change. If
> the existing sub-op was to be modified, I think we'd need
> (with c representing the current domain)
> - (d over t) || ((c over d) && (c over t)) for not regressing
>    the pre-existing use case,
> - only (c over d) && (c over t) for not permitting something
>    that isn't intended to be permitted in the new use case.
> Unless the sub-op has room for adding a flag to indicate
> which of the two is meant (I didn't check), I don't see a way
> around adding another sub-op, no matter how similar this
> would end up being.
>
> Jan
I would say the current lack of a check for (c over t) is an oversight,
which mostly doesn't matter because the ability to modify arbitrary
memory in your target is transitive in almost any security model (c can
modify d's code to modify t, so a malicious c can compromise t anyway).
If the three domains are all different, the only way this can happen in
non-XSM is for (c) to be dom0 or for your device model to have a device
model (which I don't think is forbidden, but doubt anyone uses).

I now agree that this deserves a new subop, since this code is reached
via the stable memory_op and not just a domctl.

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