Re: [Xen-devel] [PATCH v4 07/14] argo: implement the register op

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Tue, Jan 15, 2019 at 6:41 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Tue, Jan 15, 2019 at 01:27:39AM -0800, Christopher Clark wrote:
> > The register op is used by a domain to register a region of memory for
> > receiving messages from either a specified other domain, or, if specifying a
> > wildcard, any domain.
> >
> > This operation creates a mapping within Xen's private address space that
> > will remain resident for the lifetime of the ring. In subsequent commits,
> > the hypervisor will use this mapping to copy data from a sending domain into
> > this registered ring, making it accessible to the domain that registered the
> > ring to receive data.
> >
> > Wildcard any-sender rings are default disabled and registration will be
> > refused with EPERM unless they have been specifically enabled with the
> > argo-mac boot option introduced here. The reason why the default for
>   ^ nit: argo-mac-permissive

ack, thanks - fixed here and below.

>
> > wildcard rings is 'deny' is that there is currently no means to protect the
> > ring from DoS by a noisy domain spamming the ring, affecting other domains
> > ability to send to it. This will be addressed with XSM policy controls in
> > subsequent work.
> >
> > Since denying access to any-sender rings is a significant functional
> > constraint, a new bootparam is provided to enable overriding this:
> >  "argo-mac" variable has allowed values: 'permissive' and 'enforcing'.
> > Even though this is a boolean variable, use these descriptive strings in
> > order to make it obvious to an administrator that this has potential
> > security impact.
> >
> > The p2m type of the memory supplied by the guest for the ring must be
> > p2m_ram_rw and the memory will be pinned as PGT_writable_page while the ring
> > is registered.
> >
> > xen_argo_gfn_t type is defined and is 64-bit on all architectures which
> > assists with avoiding the need for compat code to translate hypercall args.
> > This hypercall op and its interface currently only supports 4K-sized pages.
> >
> > array_index_nospec is used to guard the result of the ring id hash function.
> > This is out of an abundance of caution, since this is a very basic hash
> > function and it operates upon values supplied by the guest just before
> > being used as an array index.
> >
> > Signed-off-by: Christopher Clark <christopher.clark6@xxxxxxxxxxxxxx>
> >
> > -This version contains FIXMEs for 4.12:
> >  * find_ring_mfn: investigate using check_get_page_from_gfn()
> >    and rewrite this function using it or with adopted logic
> >
> >  * shrink critical sections: move acquire/release of the global lock.
> >  * simplify the out label path when lock release has been moved.
> >
> >  * - drop use of unsigned long type as hypercall args: not compat-friendly
> >  * - drop UL suffix on XEN_ARGO_REGISTER_FLAG_MASK
> >  * - guard XEN_ARGO_REGISTER_FLAG_MASK (perhaps framed by "#ifdef __XEN__")
> >  * - define XEN_ARGO_REGISTER_FLAG_MASK in terms of other flags defined
> >
> >  * register_ring: pull write_unlock up above the cleanup actions above
> >    and add another label to aborb the two separate put_domain() calls on
> >    the error paths.
>
> Thanks, would you agree to add a FIXME to look into using vmap in
> order to map the ring pages into contiguous virtual address space in
> order to simplify access to the rings? That would likely apply to the
> code in ring_map_page, and IMO doesn't need to be done for 4.12, can
> be left for later if there are time constrains.

Ack - agreed, done.

> The rest LGTM.

thanks!

Christopher

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel