Re: [Xen-devel] [PATCH] lock down hypercall continuation encoding masks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/12/14 14:47, Jan Beulich wrote:
>>>> On 05.12.14 at 15:36, <andrew.cooper3@xxxxxxxxxx> wrote:
>> 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@xxxxxxxxxx>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> 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.
> Not sure what you're asking for - why is removing the masking not
> sufficient?

There is no check to ensure that a non-preemptible arch_memoy_op is not
called with a non-zero start_iter.

This location needs something like

if ( cmd & ~MEMOP_CMD_MASK )
    return -ENOSYS;

>
>> The ARM code also needs one, as the caller has applied partial checks.
> The ARM code never applied a mask.

But the common code does, so the ARM code must follow suit for consistency.

Otherwise, we end up with ARM non-preemptible memory subops not failing
with -ENOSYS where primary memory ops would.

>
>>> --- 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.
> Again - I don't understand what you're asking for: The hunk above
> is modifying the XENMEM_get_vnumainfo case.

My apologies - I can't see now why I identified get_vnumainfo as missing
a check.

~Andrew


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