linux-kernel - Re: [PATCH] sched/isolation: don't do unbounded chomp on bootarg string

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] [day] [month] [year] [list]

Message-ID: <20210421060243.GC194948@windriver.com>
Date:   Wed, 21 Apr 2021 02:02:43 -0400
From:   Paul Gortmaker <paul.gortmaker@...driver.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Marcelo Tosatti <mtosatti@...hat.com>
Subject: Re: [PATCH] sched/isolation: don't do unbounded chomp on bootarg
 string

[Re: [PATCH] sched/isolation: don't do unbounded chomp on bootarg string] On 19/04/2021 (Mon 16:38) Peter Xu wrote:

> On Sun, Apr 18, 2021 at 05:54:26PM -0400, Paul Gortmaker wrote:
> > After commit 3662daf02350 ("sched/isolation: Allow "isolcpus=" to skip
> > unknown sub-parameters") the isolcpus= string is walked to skip over what
> > might be any future flag comma separated additions.
> > 
> > However, there is a logic error, and so as can clearly be seen below, it
> > will ignore its own arg len and search to the end of the bootarg string.
> > 
> >  $ dmesg|grep isol
> >  Command line: BOOT_IMAGE=/boot/bzImage isolcpus=xyz pleasedontparseme=1 root=/dev/sda1 ro
> >  isolcpus: Skipped unknown flag xyz
> >  isolcpus: Invalid flag pleasedontparseme=1 root=/dev/sda1 ro
> > 
> > This happens because the flag "skip" code does an unconditional
> > increment, which skips over the '\0' check the loop body looks for. If
> > the isolcpus= happens to be the last bootarg, then you'd never notice?
> > 
> > So we only increment if the skipped flag is followed by a comma, as per
> > what the existing "continue" flag matching code does.
> > 
> > Note that isolcpus= was declared deprecated as of v4.15 (b0d40d2b22fe),
> > so we might want to revisit that if we are trying to future-proof it
> > as recently as a year ago for as yet unseen new flags.
> 
> Thanks for report the issue.
> 
> Is cpuset going to totally replace "isolcpus="?  It seems most hk_flags will be
> handled by nohz_full=, and HK_FLAG_DOMAIN can be done by cpuset.  However it
> seems still the only place to set the new flag HK_FLAG_MANAGED_IRQ.  If one day
> we'll finally obsolete isolcpus= we may need to think about where to put it?

It is probably overly optimistic that we'll ever get to retire it, based
on past history of trying to remove old use cases out of the kernel.

> 
> When I looked at it, I also noticed I see no caller to set HK_FLAG_SCHED at
> all.  Is it really used anywhere?

I guess that would be a Frederic question.  In the commit log of
de201559df (back in 2017) he did note flags might be too fine grained...

> 
> Regarding this patch...
> 
> > 
> > Cc: Peter Xu <peterx@...hat.com>
> > Cc: Ingo Molnar <mingo@...hat.com>
> > Cc: Thomas Gleixner <tglx@...utronix.de>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Frederic Weisbecker <frederic@...nel.org>
> > Fixes: 3662daf02350 ("sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters")
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@...driver.com>
> > 
> > diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> > index 5a6ea03f9882..9652dba7e938 100644
> > --- a/kernel/sched/isolation.c
> > +++ b/kernel/sched/isolation.c
> > @@ -188,7 +188,8 @@ static int __init housekeeping_isolcpus_setup(char *str)
> >  		}
> >  
> >  		pr_info("isolcpus: Skipped unknown flag %.*s\n", len, par);
> > -		str++;
> > +		if (str[1] == ',')	/* above continue; match on "flag," */
> 
> .. wondering why it is not "str[0] == ','" instead?

Apparently I need to add some more convenient self tests, so I don't
manage to hurt myself with string handling.  Thanks for spotting it.

Paul.
--

> 
> Thanks,
> 
> > +			str++;
> >  	}
> >  
> >  	/* Default behaviour for isolcpus without flags */
> > -- 
> > 2.25.1
> > 
> 
> -- 
> Peter Xu
>