Re: [PATCH 1/4] common/sysctl: Don't leak status in SYSCTL_page_offline_op

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

On 07/01/14 11:33, Jan Beulich wrote:
>>>> On 27.12.13 at 15:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Also fix the indentation of the arguments to copy_to_guest() to help clarify
>> that the 'ret = -EFAULT' is not part of the condition.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> ---
>>  xen/common/sysctl.c |   10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>> index 117e095..cd6184a 100644
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -230,15 +230,13 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
>> u_sysctl)
>>          }
>>  
>>          if ( copy_to_guest(
>> -            op->u.page_offline.status, status,
>> -            op->u.page_offline.end - op->u.page_offline.start + 1) )
>> -        {
>> +                 op->u.page_offline.status, status,
>> +                 op->u.page_offline.end - op->u.page_offline.start + 1) )
>>              ret = -EFAULT;
>> -            break;
>> -        }
>> +        else
>> +            copyback = 0;
>>  
>>          xfree(status);
>> -        copyback = 0;
> This is wrong (and not covered by the title or description) - there's
> nothing to copy back here (apart from "status"), so this should
> remain unconditional.
>
> Jan
>

There is a 'break' removed from the if statement, so there is no change
to the conditions during which copyback gets set.

~Andrew