linux-kernel - Re: [PATCH 01/11] x86: Use asm goto to implement better modify_and

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, 19 Sep 2013 11:39:54 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Andi Kleen <ak@...ux.intel.com>
Cc:	"H. Peter Anvin" <hpa@...or.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Mike Galbraith <bitbucket@...ine.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>
Subject: Re: [PATCH 01/11] x86: Use asm goto to implement better
 modify_and_test() functions


* Andi Kleen <ak@...ux.intel.com> wrote:

> On Wed, Sep 18, 2013 at 02:02:37PM -0500, H. Peter Anvin wrote:
>
> > Yes, a bit sad.  We allow bracketing with the get/put_user_try/catch 
> > blocks, but that is x86-specific. I don't think a generic option is 
> > possible without compiler support, but it might be possible to do 
> > better than we do know.
> 
> Letting the compiler do it is a bit risky, because it may open it up for 
> really large blocks, thus defeating the security advantages.

Yeah, the compiler could cover other pointer dereferences in the put_user 
block and that won't result in any visible breakage, so it's difficult to 
prevent the compiler doing it accidentally or even intentionally.

Then again the many repeated STAC/CLAC sequences are really not nice.

So maybe we could add some macro magic to generate better assembly here - 
if we coded up a __put_user_2field() primitive then we could already 
optimize the filldir() case:

before:

        if (__put_user(d_ino, &dirent->d_ino))
                goto efault;
        if (__put_user(reclen, &dirent->d_reclen))
                goto efault;
        if (copy_to_user(dirent->d_name, name, namlen))
                goto efault;
        if (__put_user(0, dirent->d_name + namlen))
                goto efault;
        if (__put_user(d_type, (char __user *) dirent + reclen - 1))
                goto efault;

after:

        if (__put_user_2field(d_ino, &dirent->d_ino, reclen, &dirent->d_reclen))
                goto efault;
        if (copy_to_user(dirent->d_name, name, namlen))
                goto efault;
        if (__put_user_2field(0, dirent->d_name + namlen, d_type, (char __user *) dirent + reclen - 1)))
                goto efault;

That cuts down the inlined STAC/CLAC pairs from 4 to 2.

__put_user_2field() would be some truly disgusting (but hidden from most 
people) macro and assembly magic.

We could also add __put_user_4field() and slightly reorder filldir():

        if (__put_user_4field(	d_ino,		&dirent->d_ino,
				reclen,		&dirent->d_reclen,
        			0,		dirent->d_name + namlen,
				d_type,		(char __user *) dirent + reclen - 1)))
                goto efault;

        if (copy_to_user(dirent->d_name, name, namlen))
                goto efault;

That would reduce the inlined STAC/CLAC pairs to a minimal 1 (only one of 
which would be visible in the filldir() disassembly).

In theory we could do something generic:

        if (__put_user_fields(	4,
				d_ino,		&dirent->d_ino,
				reclen,		&dirent->d_reclen,
        			0,		 dirent->d_name + namlen,
				d_type,		(char __user *)dirent + reclen-1 ))
                goto efault;

        if (copy_to_user(dirent->d_name, name, namlen))
                goto efault;

and implement it up to 4 or so. It will be some truly disgusting lowlevel 
code (especially due to the size variations which could make it explode 
combinatorically), with some generic header fallback that utilizes 
existing put_user primitives.

But it's solvable IMO, if we want to solve it. On the high level it's also 
more readable in a fashion and hence perhaps a bit less fragile than our 
usual __put_user() patterns.

Btw., while at it we could also maybe fix the assignment ordering and use 
copy_to_user() naming:

        if (__copy_to_user_fields(4,

			&dirent->d_ino,				d_ino,
			&dirent->d_reclen,			reclen,		
        		dirent->d_name + namlen,		0,
			(char __user *)dirent + reclen-1,	d_type	))

                goto efault;

        if (copy_to_user(dirent->d_name, name, namlen))
                goto efault;

That would make it even more readable.

(Thinking about the macro tricks needed for something like this gave me a 
bad headache though.)

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/