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

[Jason Andryuk <andryuk@xxxxxxxx>]

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@xxxxxxxx>
>> ---
>> 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?

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

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