[PATCH] libxc: Protect xc_domain_resume from clobbering domain registers

[PATCH] libxc: Protect xc_domain_resume from clobbering domain registers

[Jason Andryuk]

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;
     }
 
+    if ( !info.shutdown || (info.shutdown_reason != SHUTDOWN_suspend) )
+    {
+        ERROR("Domain not in suspended state");
+        errno = EINVAL;
+        return -1;
+    }
+
     if ( info.hvm )
     {
         /* HVM guests without PV drivers have no return code to modify. */
-- 
1.8.3.1

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

[Andrew Cooper]

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

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

~Andrew

> +        errno = EINVAL;
> +        return -1;
> +    }
> +
>      if ( info.hvm )
>      {
>          /* HVM guests without PV drivers have no return code to modify. */

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

[Jason Andryuk]

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?

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

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

[Andrew Cooper]

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