linux-kernel - Re: [PATCH v7 10/22] sched: Reject CPU affinity changes based on task_cpu_possible

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: <20210526185949.GC20055@willie-the-truck>
Date:   Wed, 26 May 2021 19:59:49 +0100
From:   Will Deacon <will@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-arch@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Catalin Marinas <catalin.marinas@....com>,
        Marc Zyngier <maz@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Morten Rasmussen <morten.rasmussen@....com>,
        Qais Yousef <qais.yousef@....com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Quentin Perret <qperret@...gle.com>, Tejun Heo <tj@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        kernel-team@...roid.com
Subject: Re: [PATCH v7 10/22] sched: Reject CPU affinity changes based on
 task_cpu_possible_mask()

On Wed, May 26, 2021 at 07:56:51PM +0200, Peter Zijlstra wrote:
> On Wed, May 26, 2021 at 05:12:49PM +0100, Will Deacon wrote:
> > On Wed, May 26, 2021 at 05:15:51PM +0200, Peter Zijlstra wrote:
> > > On Tue, May 25, 2021 at 04:14:20PM +0100, Will Deacon wrote:
> > > > Reject explicit requests to change the affinity mask of a task via
> > > > set_cpus_allowed_ptr() if the requested mask is not a subset of the
> > > > mask returned by task_cpu_possible_mask(). This ensures that the
> > > > 'cpus_mask' for a given task cannot contain CPUs which are incapable of
> > > > executing it, except in cases where the affinity is forced.
> > > > 
> > > > Reviewed-by: Quentin Perret <qperret@...gle.com>
> > > > Signed-off-by: Will Deacon <will@...nel.org>
> > > > ---
> > > >  kernel/sched/core.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > > index 00ed51528c70..8ca7854747f1 100644
> > > > --- a/kernel/sched/core.c
> > > > +++ b/kernel/sched/core.c
> > > > @@ -2346,6 +2346,7 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
> > > >  				  u32 flags)
> > > >  {
> > > >  	const struct cpumask *cpu_valid_mask = cpu_active_mask;
> > > > +	const struct cpumask *cpu_allowed_mask = task_cpu_possible_mask(p);
> > > >  	unsigned int dest_cpu;
> > > >  	struct rq_flags rf;
> > > >  	struct rq *rq;
> > > > @@ -2366,6 +2367,9 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
> > > >  		 * set_cpus_allowed_common() and actually reset p->cpus_ptr.
> > > >  		 */
> > > >  		cpu_valid_mask = cpu_online_mask;
> > > > +	} else if (!cpumask_subset(new_mask, cpu_allowed_mask)) {
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > >  	}
> > > 
> > > So what about the case where the 32bit task is in-kernel and in
> > > migrate-disable ? surely we ought to still validate the new mask against
> > > task_cpu_possible_mask.
> > 
> > That's a good question.
> > 
> > Given that 32-bit tasks in the kernel are running in 64-bit mode, we can
> > actually tolerate them moving around arbitrarily as long as they _never_ try
> > to return to userspace on a 64-bit-only CPU. I think this should be the case
> > as long as we don't try to return to userspace with migration disabled, no?
> 
> Consider:
> 
> 	8 CPUs, lower 4 have 32bit, higher 4 do not
> 
> 	A - a 32 bit task		B
> 
> 	sys_foo()
> 	  migrate_disable()
> 	  				sys_sched_setaffinity(A, 0xf0)
> 					  if (.. | migration_disabled(A))
> 					    // not checking nothing
> 
> 					  __do_set_cpus_allowed();
> 
> 	  migrate_enable()
> 	    __set_cpus_allowed(SCA_MIGRATE_ENABLE)
> 	      // frob outselves somewhere in 0xf0
> 	  sysret
> 	  *BOOM*
> 
> 
> That is, I'm thinking we ought to disallow that sched_setaffinity() with
> -EINVAL for 0xf0 has no intersection with 0x0f.

I *think* the cpuset_cpus_allowed() check in sys_sched_setaffinity()
will save us here by reducing the 0xf0 mask to 0x0 early on. However,
after seeing this example I'd be much happier checking this in SCA anyway so
I'll add that for the next version!

Thanks,

Will