On Wed, Apr 26, 2017 at 11:49 PM, Ingo Molnar [off-list ref] wrote:

* Thomas Garnier [off-list ref] wrote:

quoted

+
+/*
+ * 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:

quoted

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

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

quoted

      {                                                               \
-             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:

quoted

+             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.

I think this construct makes more sense because the first macro check
if the syscall was called by user-mode. I will send an update for this
on this thread.

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.

I don't think architectures need to do that. The optimizations are
embedding the checks on their architecture-specific code to make it
faster and remove the size impact. The pre/post is fine for the rest.

Thanks,

        Ingo

-- 
Thomas