Re: [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection

Le 20/03/2019 à 14:04, Christophe Leroy a écrit :

Le 20/03/2019 à 13:57, Michael Ellerman a écrit :

quoted

Christophe Leroy [off-list ref] writes:

quoted

Le 08/03/2019 à 02:16, Michael Ellerman a écrit :

quoted

From: Christophe Leroy <redacted>

This patch implements a framework for Kernel Userspace Access
Protection.

Then subarches will have the possibility to provide their own
implementation by providing setup_kuap() and
allow/prevent_user_access().

Some platforms will need to know the area accessed and whether it is
accessed from read, write or both. Therefore source, destination and
size and handed over to the two functions.

mpe: Rename to allow/prevent rather than unlock/lock, and add
read/write wrappers. Drop the 32-bit code for now until we have an
implementation for it. Add kuap to pt_regs for 64-bit as well as
32-bit. Don't split strings, use pr_crit_ratelimited().

Signed-off-by: Christophe Leroy <redacted>
Signed-off-by: Russell Currey <redacted>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5: Futex ops need read/write so use allow_user_acccess() there.
      Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
      Allow subarch to override allow_read/write_from/to_user().

Those little helpers that will just call allow_user_access() when
distinct read/write handling is not performed looks overkill to me.

Can't the subarch do it by itself based on the nullity of from/to ?

static inline void allow_user_access(void __user *to, const void __user
*from,
                     unsigned long size)
{
    if (to & from)
        set_kuap(0);
    else if (to)
        set_kuap(AMR_KUAP_BLOCK_READ);
    else if (from)
        set_kuap(AMR_KUAP_BLOCK_WRITE);
}

You could implement it that way, but it reads better at the call sites
if we have:

    allow_write_to_user(uaddr, sizeof(*uaddr));
vs:
    allow_user_access(uaddr, NULL, sizeof(*uaddr));

So I'm inclined to keep them. It should all end up inlined and generate
the same code at the end of the day.

I was not suggesting to completly remove allow_write_to_user(), I fully 
agree that it reads better at the call sites.

I was just thinking that allow_write_to_user() could remain generic and 
call the subarch specific allow_user_access() instead of making multiple 
subarch's allow_write_to_user()

Otherwise, could we do something like the following, so that subarches 
may implement it or not ?

#ifndef allow_read_from_user
static inline void allow_read_from_user(const void __user *from, 
unsigned long size)
{
	allow_user_access(NULL, from, size);
}
#endif

#ifndef allow_write_from_user
static inline void allow_write_to_user(void __user *to, unsigned long size)
{
	allow_user_access(to, NULL, size);
}
#endif

Christophe

But both solution are OK for me at the end.

Christophe