Re: [PATCH] libxc: Protect xc_domain_resume from clobbering domain registers

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

On 20/05/14 13:51, Jason Andryuk wrote:
> On 5/20/2014 5:53 AM, Andrew Cooper wrote:
>> On 19/05/14 19:37, Jason Andryuk wrote:
>>> xc_domain_resume() expects the guest to be in state SHUTDOWN_suspend.
>>> However, nothing verifies the state before modify_returncode() modifies
>>> the domain's registers.  This will crash guest processes or the kernel
>>> itself.
>>>
>>> This can be demonstrated with `LIBXL_SAVE_HELPER=/bin/false xl migrate`.
>>>
>>> Signed-off-by: Jason Andryuk <andryuk@aero.org>
>>> ---
>>> Changes since RFC:
>>>   - Return -1 from modify_returncode
>>>   - Set errno to EINVAL
>>> ---
>>>   tools/libxc/xc_resume.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
>>> index 18b4818..2163ad9 100644
>>> --- a/tools/libxc/xc_resume.c
>>> +++ b/tools/libxc/xc_resume.c
>>> @@ -39,6 +39,13 @@ static int modify_returncode(xc_interface *xch, uint32_t domid)
>>>           return -1;
>>>       }
>> Having looked at this more closely, there is also a bug in the hunk above.
>>
>> xc_domain_getinfo() being the 'special' function that it is doesn't
>> always return the domain specified.  If the given domid doesn't exist in
>> the system, you will get back the first domain with a higher domid.
>>
>> The only safe way to use it is
>>
>> if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 ||
>>       info.domid != domid )
>> {
>>      error...
>> }
> I am ok making this change; I wasn't aware of the quirks of the function.
>
> This patch copy & pasted the check from xc_domain_save.c:suspend_and_state().  That and other locations throughout libxc fail to compare info.domid to the requested domid, so there are lots of places that should be fixed up.  Maybe a wrapper xc_domain_getinfo_one(xch, domid, &info) that checks the domid would be useful?

It certainly would be useful, although probably as a separate patch.

>
>>>   
>>> +    if ( !info.shutdown || (info.shutdown_reason != SHUTDOWN_suspend) )
>>> +    {
>>> +        ERROR("Domain not in suspended state");
>> ERROR("Dom %d not suspended: (shutdown %d, reason %d)", domid,
>> info.shutdown, info.shutdown_reason));
>>
>> This way, someone unexpectedly finding this error message gets slightly
>> more information than "something wasn't how I expected it to be".
> Yes, this is more informative.  suspend_and_state could also be updated to the same message.
>
> -Jason

Probably no need to worry about suspend_and_state().  It is about to
disappear with the new migration protocol work.

~Andrew