Re: [PATCH] lock down hypercall continuation encoding masks

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/12/14 11:31, Jan Beulich wrote:
> Andrew validly points out that even if these masks aren't a formal part
> of the hypercall interface, we aren't free to change them: A guest
> suspended for migration in the middle of a continuation would fail to
> work if resumed on a hypervisor using a different value. Hence add
> respective comments to their definitions.
>
> Additionally, to help future extensibility as well as in the spirit of
> reducing undefined behavior as much as possible, refuse hypercalls made
> with the respective bits non-zero when the respective sub-ops don't
> make use of those bits.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

General principle looks good.  A couple of issues.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4661,9 +4661,8 @@ int xenmem_add_to_physmap_one(
>  long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
>      int rc;
> -    int op = cmd & MEMOP_CMD_MASK;

This needs a blanket start_iter check, as do_memory_op() has not done so.

The ARM code also needs one, as the caller has applied partial checks.

>  
> -    switch ( op )
> +    switch ( cmd )
>      {
>      case XENMEM_set_memory_map:
>      {
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -906,9 +906,8 @@ long subarch_memory_op(unsigned long cmd
>      xen_pfn_t mfn, last_mfn;
>      unsigned int i;
>      long rc = 0;
> -    int op = cmd & MEMOP_CMD_MASK;

It is probably best to have a blanket check here even if
arch_memory_op() has a check.  It will reduce the chance of the check
being missed if/when arch_memory_op() gains a presentable subop.

>  
> -    switch ( op )
> +    switch ( cmd )
>      {
>      case XENMEM_machphys_mfn_list:
>          if ( copy_from_guest(&xmml, arg, 1) )
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -977,6 +992,9 @@ long do_memory_op(unsigned long cmd, XEN
>          unsigned int dom_vnodes, dom_vranges, dom_vcpus;
>          struct vnuma_info tmp;
>  
> +        if ( unlikely(start_extent) )
> +            return -ENOSYS;
> +
>          /*
>           * Guest passes nr_vnodes, number of regions and nr_vcpus thus
>           * we know how much memory guest has allocated.

XENMEM_get_vnumainfo needs a guard.

~Andrew