lkml.org 
[lkml]   [2017]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v8 1/4] syscalls: Verify address limit before returning to user-mode

* Thomas Garnier <thgarnie@google.com> wrote:

> +
> +/*
> + * Called before coming back to user-mode. Returning to user-mode with an
> + * address limit different than USER_DS can allow to overwrite kernel memory.
> + */
> +static inline void addr_limit_check_syscall(void)
> +{
> + BUG_ON(!segment_eq(get_fs(), USER_DS));
> +}
> +
> +#ifndef CONFIG_ADDR_LIMIT_CHECK
> +#define __CHECK_USERMODE_SYSCALL() \
> + bool user_caller = segment_eq(get_fs(), USER_DS)
> +#define __VERIFY_ADDR_LIMIT() \
> + if (user_caller) addr_limit_check_syscall()
> +#else
> +#define __CHECK_USERMODE_SYSCALL()
> +#define __VERIFY_ADDR_LIMIT()
> +asmlinkage void addr_limit_check_failed(void) __noreturn;
> +#endif

_Please_ harmonize all the externally exposed names and symbols.

There's no reason for this mismash of names:

CONFIG_ADDR_LIMIT_CHECK

__CHECK_USERMODE_SYSCALL
__VERIFY_ADDR_LIMIT

When we could just as easily name them consistently, along the existing pattern:

CONFIG_ADDR_LIMIT_CHECK

__SYSCALL_ADDR_LIMIT_CHECK
__ADDR_LIMIT_CHECK

which should fit into existing nomenclature:

> #define __SYSCALL_DEFINEx(x, name, ...) \

But even with that fixed, the whole construct still looks pretty weird:

> { \
> - long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
> + long ret; \
> + __CHECK_USERMODE_SYSCALL(); \
> + ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
> + __ADDR_LIMIT_CHECK(); \
> __MAP(x,__SC_TEST,__VA_ARGS__); \
> __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
> return ret; \

I think something like this would be more natural to read:

> + ADDR_LIMIT_CHECK_PRE(); \
> + ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
> + ADDR_LIMIT_CHECK_POST(); \

it's a clear pre/post construct. Also note the lack of double underscores.

BTW., a further simplification would be:

#ifndef ADDR_LIMIT_CHECK_PRE
# define ADDR_LIMIT_CHECK_PRE ...
#endif

This way architectures could override this generic functionality simply by
defining the helpers. Architectures that don't do that get the generic version.

Thanks,

Ingo

\
 
 \ /
  Last update: 2017-04-27 08:50    [W:1.733 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site