linux-kernel - Re: [RFC PATCH v3 1/5] getcpu_cache system call: cache CPU number of running thread

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]

Date:	Thu, 28 Jan 2016 22:52:13 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
cc:	Paul Turner <pjt@...gle.com>, Andrew Hunter <ahh@...gle.com>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
	Andy Lutomirski <luto@...capital.net>,
	Andi Kleen <andi@...stfloor.org>,
	Dave Watson <davejwatson@...com>, Chris Lameter <cl@...ux.com>,
	Ingo Molnar <mingo@...hat.com>, Ben Maurer <bmaurer@...com>,
	Steven Rostedt <rostedt@...dmis.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Josh Triplett <josh@...htriplett.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Russell King <linux@....linux.org.uk>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Michael Kerrisk <mtk.manpages@...il.com>
Subject: Re: [RFC PATCH v3 1/5] getcpu_cache system call: cache CPU number
 of running thread

On Thu, 28 Jan 2016, Mathieu Desnoyers wrote:

> +static int __get_cpu_cache_ptr(int32_t __user **cpu_cache,

cpu_cache is __user ????

> +		int32_t __user * __user *cpu_cachep)
> +{
> +#ifdef CONFIG_COMPAT
> +	if (is_compat_task()) {
> +		compat_uptr_t *compat_cachep = (compat_uptr_t *) cpu_cachep;
> +		compat_uptr_t compat_cache;
> +
> +		if (get_user(compat_cache, compat_cachep))
> +			return -EFAULT;
> +		*cpu_cache = compat_ptr(compat_cache);

sparse should have told you that :)

> +		return 0;
> +	}
> +#endif
> +	return get_user(*cpu_cache, cpu_cachep);
> +}
> +
> +#define get_cpu_cache_ptr(cpu_cache, cpu_cachep)	\
> +	__get_cpu_cache_ptr(&(cpu_cache), cpu_cachep)
> +
> +static int put_cpu_cache_ptr(int32_t __user *cpu_cache,

Ditto

> +		int32_t __user * __user *cpu_cachep)
> +{
> +#ifdef CONFIG_COMPAT
> +	if (is_compat_task()) {
> +		compat_uptr_t compat_cache = ptr_to_compat(cpu_cache);
> +		compat_uptr_t *compat_cachep = (compat_uptr_t *) cpu_cachep;
> +
> +		return put_user(compat_cache, compat_cachep);
> +	}
> +#endif
> +	return put_user(cpu_cache, cpu_cachep);
> +}
> +		current->cpu_cache = cpu_cache;
> +		/*
> +		 * Migration checks the getcpu cache to see whether the
> +		 * notify_resume flag should be set.
> +		 * Therefore, we need to ensure that the scheduler sees
> +		 * the getcpu cache pointer update before we update the getcpu
> +		 * cache content with the current CPU number.
> +		 */
> +		barrier();

And how does that barrier ensure this? Not at all. And why would the scheduler
care? All the scheduler cares about is tsk->cpu_cache.

> +		/*
> +		 * Do an initial cpu cache update to ensure we won't hit
> +		 * SIGSEGV if put_user() fails in the resume notifier.
> +		 */

If you get migrated before that call, then you SIGSEGV nevertheless.

You need that call here for the case you are NOT migrated before returning to
user space because otherwise the variable is not updated.

If you want to verify that user address without a potential SIGSEGV, then you
need to do this before setting current->cpu_cache. You still need the update
after setting current->cpu_cache.

> +		if (getcpu_cache_update(cpu_cache)) {
> +			current->cpu_cache = NULL;
> +			return -EFAULT;
> +		}
> +		return 0;

Thanks,

	tglx