[PATCH] sysctl: Don't overwrite array size variable when it is set on error earlier

[PATCH] sysctl: Don't overwrite array size variable when it is set on error earlier

[Boris Ostrovsky]

When querying CPU topology, if caller-provided array size is smaller than
number of online CPUs then, in addition to returning -ENOBUFS, sysctl is
expected to provide back this number. However, this value, stored in 'i',
is overwritten in the subsequent loop's control statement.

Make sure we don't do this by converting the loop to 'while'.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/common/sysctl.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index a8c629f..b83d230 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -338,8 +338,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
                 ret = -ENOBUFS;
                 i = num_cpus;
             }
+            else
+                i = 0;
 
-            for ( i = 0; i < num_cpus; i++ )
+            while ( i < num_cpus )
             {
                 xen_sysctl_cputopo_t cputopo;
 
@@ -363,6 +365,8 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
                     ret = -EFAULT;
                     break;
                 }
+
+                i++;
             }
         }
         else
-- 
1.7.1

Re: [PATCH] sysctl: Don't overwrite array size variable when it is set on error earlier

[Andrew Cooper]

On 25/03/15 17:09, Boris Ostrovsky wrote:
> When querying CPU topology, if caller-provided array size is smaller than
> number of online CPUs then, in addition to returning -ENOBUFS, sysctl is
> expected to provide back this number. However, this value, stored in 'i',
> is overwritten in the subsequent loop's control statement.
>
> Make sure we don't do this by converting the loop to 'while'.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>   xen/common/sysctl.c |    6 +++++-
>   1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index a8c629f..b83d230 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -338,8 +338,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>                   ret = -ENOBUFS;
>                   i = num_cpus;
>               }
> +            else
> +                i = 0;
>   
> -            for ( i = 0; i < num_cpus; i++ )
> +            while ( i < num_cpus )

This would be fine to keep as "for ( ; i < num_cpus; i++)", and helps 
avoid an issue if someone introduces a continue; in the future.

As for the fix itself, Reviewed-by: Andrew Cooper 
<andrew.cooper3@citrix.com>

>               {
>                   xen_sysctl_cputopo_t cputopo;
>   
> @@ -363,6 +365,8 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>                       ret = -EFAULT;
>                       break;
>                   }
> +
> +                i++;
>               }
>           }
>           else

Re: [PATCH] sysctl: Don't overwrite array size variable when it is set on error earlier

[Jan Beulich]

>>> On 25.03.15 at 18:12, <andrew.cooper3@citrix.com> wrote:
> On 25/03/15 17:09, Boris Ostrovsky wrote:
>> When querying CPU topology, if caller-provided array size is smaller than
>> number of online CPUs then, in addition to returning -ENOBUFS, sysctl is
>> expected to provide back this number. However, this value, stored in 'i',
>> is overwritten in the subsequent loop's control statement.
>>
>> Make sure we don't do this by converting the loop to 'while'.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>>   xen/common/sysctl.c |    6 +++++-
>>   1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>> index a8c629f..b83d230 100644
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -338,8 +338,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>>                   ret = -ENOBUFS;
>>                   i = num_cpus;
>>               }
>> +            else
>> +                i = 0;
>>   
>> -            for ( i = 0; i < num_cpus; i++ )
>> +            while ( i < num_cpus )
> 
> This would be fine to keep as "for ( ; i < num_cpus; i++)", and helps 
> avoid an issue if someone introduces a continue; in the future.

And that's also what I've been intending to change it to before
committing.

Jan