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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.01.14 at 12:34, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/01/14 11:33, Jan Beulich wrote:
>>>>> On 27.12.13 at 15:57, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> CC: Keir Fraser <keir@xxxxxxx>
>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>> ---
>>>  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.
> 
> There is a 'break' removed from the if statement, so there is no change
> to the conditions during which copyback gets set.

Ah, true. But nevertheless, the code would be more correct if
the clearing of copyback was left where it was.

Jan


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