linux-kernel - Re: [External] Re: [PATCH RESEND] smp: Fix a potential usage of stale nr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMZfGtX7gxky5MVFwz3k=BN7p1tL+PKN43TENbuCfa+AkSbzZg@mail.gmail.com>
Date:   Sat, 18 Jul 2020 10:29:11 +0800
From:   Muchun Song <songmuchun@...edance.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Peter Zijlstra <peterz@...radead.org>, mingo@...nel.org,
        bigeasy@...utronix.de, namit@...are.com,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [External] Re: [PATCH RESEND] smp: Fix a potential usage of stale nr_cpus

On Sat, Jul 18, 2020 at 4:15 AM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> Muchun,
>
> Muchun Song <songmuchun@...edance.com> writes:
>
> > The get_option() maybe return 0, it means that the nr_cpus is
> > not initialized.
>
> Good catch, but see below.
>
> > Then we will use the stale nr_cpus to initialize
>
> We use nothing. Please describe your changes in technical neutral
> language.
>
> > the nr_cpu_ids. So fix it.
>
> 'So fix it.' is not much valuable information. What about:
>
>     Check the return value to prevent this.
>
> Hmm?

Looks fine to me. Thanks.

>
> > Signed-off-by: Muchun Song <songmuchun@...edance.com>
> > ---
> >  kernel/smp.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/kernel/smp.c b/kernel/smp.c
> > index 472c2b274c65..2a9a04acf123 100644
> > --- a/kernel/smp.c
> > +++ b/kernel/smp.c
> > @@ -634,8 +634,7 @@ static int __init nrcpus(char *str)
> >  {
> >       int nr_cpus;
> >
> > -     get_option(&str, &nr_cpus);
> > -     if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
> > +     if (get_option(&str, &nr_cpus) && nr_cpus > 0 && nr_cpus < nr_cpu_ids)
> >               nr_cpu_ids = nr_cpus;
> >
> >       return 0;
>
> get_option() can return 0 - 3:
>
>  *      0 - no int in string
>  *      1 - int found, no subsequent comma
>  *      2 - int found including a subsequent comma
>  *      3 - hyphen found to denote a range
>
> For this parameter exists only one valid format: '1 - int found, no comma',
> right?

Yeah.

>
> So why fixing it just half and why returning '0' aka success if the
> parameter is bogus?

Thanks. Will fix it.

>
> Thanks,
>
>         tglx



-- 
Yours,
Muchun