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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.12.14 at 16:01, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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;

I'm sorry - the default case of sub_arch_memory_op() will ensure
this.

>>> 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.

Again, the default case results in -ENOSYS for any with the high
bits set.

Jan


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