Re: [PATCH 01/11] x86: Use asm goto to implement bettermodify_and_test() functions

From: Ingo Molnar
Date: Thu Sep 19 2013 - 05:40:07 EST



* Andi Kleen <ak@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/