Re: [PATCH] lock down hypercall continuation encoding masks

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

On 05/12/14 15:18, Jan Beulich wrote:
>>>> On 05.12.14 at 16:01, <andrew.cooper3@citrix.com> wrote:
>> On 05/12/14 14:47, Jan Beulich wrote:
>>>>>> On 05.12.14 at 15:36, <andrew.cooper3@citrix.com> 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@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.
>>> 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.

Ah - I see now.  That is subtle.  Better remember to double check the
first patch which needs to add a preemptible subop.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>