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

["Roger Pau Monné" <roger.pau@citrix.com>]

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

> 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@baesystems.com>
> 
> -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.

The rest LGTM.

Roger.

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