[PATCH v3] smp: Fix a potential usage of stale nr_cpus

[PATCH v3] smp: Fix a potential usage of stale nr_cpus

[Muchun Song]

When the cmdline of "nr_cpus" is not valid, the @nr_cpu_ids is assigned
a stale value. The nr_cpus is only valid when get_option() return 1. So
check the return value to prevent this.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
changelog in v3:
 1) Return -EINVAL when the parameter is bogus. 

changelog in v2:
 1) Rework the commit log.
 2) Rework the return value check.

 kernel/smp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index a5a66fc28f4e..0dacfcfcf00b 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -772,9 +772,13 @@ static int __init nrcpus(char *str)
 {
 	int nr_cpus;
 
-	get_option(&str, &nr_cpus);
+	if (get_option(&str, &nr_cpus) != 1)
+		return -EINVAL;
+
 	if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
 		nr_cpu_ids = nr_cpus;
+	else
+		return -EINVAL;
 
 	return 0;
 }
-- 
2.11.0

Re: [PATCH v3] smp: Fix a potential usage of stale nr_cpus

[Ingo Molnar]

* Muchun Song <songmuchun@bytedance.com> wrote:

> When the cmdline of "nr_cpus" is not valid, the @nr_cpu_ids is assigned
> a stale value. The nr_cpus is only valid when get_option() return 1. So
> check the return value to prevent this.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
> changelog in v3:
>  1) Return -EINVAL when the parameter is bogus. 
> 
> changelog in v2:
>  1) Rework the commit log.
>  2) Rework the return value check.
> 
>  kernel/smp.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/smp.c b/kernel/smp.c
> index a5a66fc28f4e..0dacfcfcf00b 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -772,9 +772,13 @@ static int __init nrcpus(char *str)
>  {
>  	int nr_cpus;
>  
> -	get_option(&str, &nr_cpus);
> +	if (get_option(&str, &nr_cpus) != 1)
> +		return -EINVAL;
> +
>  	if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
>  		nr_cpu_ids = nr_cpus;
> +	else
> +		return -EINVAL;

Exactly what does 'not valid' mean, and why doesn't get_option() 
return -EINVAL in that case?

Thanks,

	Ingo

Re: [PATCH v3] smp: Fix a potential usage of stale nr_cpus

[Thomas Gleixner]

Ingo Molnar <mingo@kernel.org> writes:
>> -	get_option(&str, &nr_cpus);
>> +	if (get_option(&str, &nr_cpus) != 1)
>> +		return -EINVAL;
>> +
>>  	if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
>>  		nr_cpu_ids = nr_cpus;
>> +	else
>> +		return -EINVAL;
>
> Exactly what does 'not valid' mean, and why doesn't get_option() 
> return -EINVAL in that case?

What's unclear about invalid? If you specify nr_cpus=-1 or
nr_cpus=2000000 the its obviously invalid.

How should get_option() know that this is invalid? get_option() is a
number parser and does not know about any restrictions on the parsed
value obviously.

get_option() returns string parsing information:

       0 -> not integer found
       1 -> integer found, no trailing comma or hyphen
       2 -> integer found and trailing comma
       3 -> integer found and traling hyphen (range parsing)

And that's what is checked in if (get_option() != 1), i.e. anything else
than a plain integer is invalid for this command line option.

Thanks,

        tglx

Re: [PATCH v3] smp: Fix a potential usage of stale nr_cpus

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> Ingo Molnar <mingo@kernel.org> writes:
> >> -	get_option(&str, &nr_cpus);
> >> +	if (get_option(&str, &nr_cpus) != 1)
> >> +		return -EINVAL;
> >> +
> >>  	if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
> >>  		nr_cpu_ids = nr_cpus;
> >> +	else
> >> +		return -EINVAL;
> >
> > Exactly what does 'not valid' mean, and why doesn't get_option() 
> > return -EINVAL in that case?
> 
> What's unclear about invalid? If you specify nr_cpus=-1 or
> nr_cpus=2000000 the its obviously invalid.

So this was the old (buggy) code:

>  {
>       int nr_cpus;
>
>       get_option(&str, &nr_cpus);
>       if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
>               nr_cpu_ids = nr_cpus;

And this was the explanation given in the changelog:

>> When the cmdline of "nr_cpus" is not valid, the @nr_cpu_ids is 
>> assigned a stale value. The nr_cpus is only valid when get_option() 
>> return 1. So check the return value to prevent this.

The answer to my question is that the bug is that the return value of 
get_option() wasn't checked properly, and if get_option() returns an 
error then the nr_cpus local variable is not set - but we used it in 
the old code, which can result in essentially a random value for 
nr_cpu_ids.

> How should get_option() know that this is invalid? get_option() is a 
> number parser and does not know about any restrictions on the parsed 
> value obviously.

But that's apparently not the bug here, 'invalid' here was meant as 
per the parser's syntax. If nr_cpus is out of range (like the 2000000 
example you gave), then nr_cpu_ids might not be set at all, and 
remains at the 0 initialized value. Which isn't good but not 'stale' 
either.

This is why I was puzzled where a 'stale' value might come from, at 
first sight I was assuming that some large value was written, like 
your 200000 example. The "stale value" happens if it's invalid syntax 
and get_option() returns an error, in which case 'nr_cpus' remains 
uninitialized.

And this is the explanation I didn't find at first reading, and which 
explanation future changelogs should perhaps include.

The new code does this:

        int nr_cpus;

        if (get_option(&str, &nr_cpus) != 1)
                return -EINVAL;
 
        if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
                nr_cpu_ids = nr_cpus;
        else
                return -EINVAL;

Which does all the proper error handling and fixes the uninitialized 
'nr_cpus' local variable usage. So I agree with the fix:

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo