Jann Horn [off-list ref] writes:

On Mon, Nov 23, 2020 at 2:45 PM Arnd Bergmann [off-list ref] wrote:

quoted

On Mon, Nov 23, 2020 at 12:15 PM Naresh Kamboju
[off-list ref] wrote:

quoted

While booting arm64 kernel the following kernel BUG noticed on several arm64
devices running linux next 20201123 tag kernel.


$ git log --oneline next-20201120..next-20201123 -- kernel/seccomp.c
5c5c5fa055ea Merge remote-tracking branch 'seccomp/for-next/seccomp'
bce6a8cba7bf Merge branch 'linus'
7ef95e3dbcee Merge branch 'for-linus/seccomp' into for-next/seccomp
fab686eb0307 seccomp: Remove bogus __user annotations
0d8315dddd28 seccomp/cache: Report cache data through /proc/pid/seccomp_cache
8e01b51a31a1 seccomp/cache: Add "emulator" to check if filter is constant allow
f9d480b6ffbe seccomp/cache: Lookup syscall allowlist bitmap for fast path
23d67a54857a seccomp: Migrate to use SYSCALL_WORK flag


Please find these easy steps to reproduce the kernel build and boot.

Adding Gabriel Krisman Bertazi to Cc, as the last patch (23d67a54857a) here
seems suspicious: it changes

diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 02aef2844c38..47763f3999f7 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h

@@ -42,7 +42,7 @@ struct seccomp {
 extern int __secure_computing(const struct seccomp_data *sd);
 static inline int secure_computing(void)
 {
-       if (unlikely(test_thread_flag(TIF_SECCOMP)))
+       if (unlikely(test_syscall_work(SECCOMP)))
                return  __secure_computing(NULL);
        return 0;
 }

which is in the call chain directly before

int __secure_computing(const struct seccomp_data *sd)
{
       int mode = current->seccomp.mode;

...
        switch (mode) {
        case SECCOMP_MODE_STRICT:
                __secure_computing_strict(this_syscall);  /* may call do_exit */
                return 0;
        case SECCOMP_MODE_FILTER:
                return __seccomp_filter(this_syscall, sd, false);
        default:
                BUG();
        }
}

Clearly, current->seccomp.mode is set to something other
than SECCOMP_MODE_STRICT or SECCOMP_MODE_FILTER
while the test_syscall_work(SECCOMP) returns true, and this
must have not been the case earlier.

Ah, I think the problem is actually in
3136b93c3fb2b7c19e853e049203ff8f2b9dd2cd ("entry: Expose helpers to
migrate TIF to SYSCALL_WORK flag"). In the !GENERIC_ENTRY case, it
adds this code:

+#define set_syscall_work(fl)                                           \
+       set_ti_thread_flag(current_thread_info(), SYSCALL_WORK_##fl)
+#define test_syscall_work(fl) \
+       test_ti_thread_flag(current_thread_info(), SYSCALL_WORK_##fl)
+#define clear_syscall_work(fl) \
+       clear_ti_thread_flag(current_thread_info(), SYSCALL_WORK_##fl)
+
+#define set_task_syscall_work(t, fl) \
+       set_ti_thread_flag(task_thread_info(t), TIF_##fl)
+#define test_task_syscall_work(t, fl) \
+       test_ti_thread_flag(task_thread_info(t), TIF_##fl)
+#define clear_task_syscall_work(t, fl) \
+       clear_ti_thread_flag(task_thread_info(t), TIF_##fl)

but the SYSCALL_WORK_FLAGS are not valid on !GENERIC_ENTRY, we'll mix
up (on arm64) SYSCALL_WORK_BIT_SECCOMP (==0) and TIF_SIGPENDING (==0).

As part of fixing this, it might be a good idea to put "enum
syscall_work_bit" behind a "#ifdef CONFIG_GENERIC_ENTRY" to avoid
future accidents like this?

Hi Jan, Arnd,

That is correct.  This is a copy pasta mistake.  My apologies.  I didn't
have a !GENERIC_ENTRY device to test, but just the ifdef would have
caught it.

-- 
Gabriel Krisman Bertazi