linux-kernel - Re: arc_usr_cmpxchg and preemption

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: <1521221606.4805.16.camel@synopsys.com>
Date:   Fri, 16 Mar 2018 17:33:27 +0000
From:   Alexey Brodkin <Alexey.Brodkin@...opsys.com>
To:     "peterz@...radead.org" <peterz@...radead.org>,
        Vineet Gupta <Vineet.Gupta1@...opsys.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "linux-snps-arc@...ts.infradead.org" 
        <linux-snps-arc@...ts.infradead.org>
Subject: Re: arc_usr_cmpxchg and preemption

Hi Peter, Vineet,

On Wed, 2018-03-14 at 18:53 +0100, Peter Zijlstra wrote:
> On Wed, Mar 14, 2018 at 09:58:19AM -0700, Vineet Gupta wrote:
> 
> > Well it is broken wrt the semantics the syscall is supposed to provide.
> > Preemption disabling is what prevents a concurrent thread from coming in and
> > modifying the same location (Imagine a variable which is being cmpxchg
> > concurrently by 2 threads).
> > 
> > One approach is to do it the MIPS way, emulate the llsc flag - set it under
> > preemption disabled section and clear it in switch_to
> 
> *shudder*... just catch the -EFAULT, force the write fault and retry.

More I look at this initially quite simple thing more it looks like
a can of worms...

> Something like:
> 
> int sys_cmpxchg(u32 __user *user_ptr, u32 old, u32 new)
> {

That functions is supposed to return old value stored in memory.
At least that's how it is used in case of ARC and M68K.

Remember there's already libc that relies on that established API
and we cannot just change it... even though it might be a good idea.
For example return "errno" and pass old value via pointer in an argument.
But now I guess it's better to use what we have now.

> 	u32 val;
> 	int ret;
> 
> again:
> 	ret = 0;
> 
> 	preempt_disable();
> 	val = get_user(user_ptr);

What if get_user() fails?
In Peter's implementation we will return 0, in Vineet's
we will return -EFAULT... and who knows what kind of unexpected behavior happens
further down the line in user-space... so I think it would be safer to kill
the process then.

And that's my take:
-------------------------->8------------------------
int sys_cmpxchg(u32 __user *user_ptr, u32 old, u32 new)
{
	u32 val;
	int ret;

again:
	ret = 0;

	preempt_disable();

	ret = get_user(val, user_ptr);
	if(ret == -EFAULT) {
		struct page *page;

		preempt_enable();
		ret = get_user_pages_fast((unsigned long)user_ptr, 1, 1, &page);
		if (ret < 0) {
			force_sig(SIGSEGV, current);
			return ret;
		}

		put_page(page);
		goto again;
	}

	if (val == old)
		ret = put_user(new, user_ptr);

	preempt_enable();

	if (ret == -EFAULT) {
		struct page *page;

		ret = get_user_pages_fast((unsigned long)user_ptr, 1, 1, &page);
		if (ret < 0) {
			force_sig(SIGSEGV, current);
			return ret;
		}

		put_page(page);
		goto again;
	}

	return ret;
}
-------------------------->8------------------------

-Alexey