Messages in this thread | | | Date | Thu, 27 Apr 2017 08:49:17 +0200 | From | Ingo Molnar <> | Subject | Re: [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
| |