From f1962207150c8b602e980616f04b37ea4e64bb9f Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Tue, 4 Jul 2023 06:37:32 -0700 Subject: module: fix init_module_from_file() error handling Vegard Nossum pointed out two different problems with the error handling in init_module_from_file(): (a) the idempotent loading code didn't clean up properly in some error cases, leaving the on-stack 'struct idempotent' element still in the hash table (b) failure to read the module file would nonsensically update the 'invalid_kread_bytes' stat counter with the error value The first error is quite nasty, in that it can then cause subsequent idempotent loads of that same file to access stale stack contents of the previous failure. The case may not happen in any normal situation (explaining all the "Tested-by's on the original change), and requires admin privileges, but syzkaller triggers random bad behavior as a result: BUG: soft lockup in sys_finit_module BUG: unable to handle kernel paging request in init_module_from_file general protection fault in init_module_from_file INFO: task hung in init_module_from_file KASAN: out-of-bounds Read in init_module_from_file KASAN: slab-out-of-bounds Read in init_module_from_file ... The second error is fairly benign and just leads to nonsensical stats (and has been around since the debug stats were added). Vegard also provided a patch for the idempotent loading issue, but I'd rather re-organize the code and make it more legible using another level of helper functions than add the usual "goto out" error handling. Link: https://lore.kernel.org/lkml/20230704100852.23452-1-vegard.nossum@oracle.com/ Fixes: 9b9879fc0327 ("modules: catch concurrent module loads, treat them as idempotent") Reported-by: Vegard Nossum Reported-by: Harshit Mogalapalli Reported-by: syzbot+9c2bdc9d24e4a7abe741@syzkaller.appspotmail.com Signed-off-by: Linus Torvalds --- kernel/module/main.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/kernel/module/main.c b/kernel/module/main.c index 834de86ebe35..59b1d067e528 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -3092,7 +3092,7 @@ static bool idempotent(struct idempotent *u, const void *cookie) * remove everybody - which includes ourselves - fill in the return * value, and then complete the operation. */ -static void idempotent_complete(struct idempotent *u, int ret) +static int idempotent_complete(struct idempotent *u, int ret) { const void *cookie = u->cookie; int hash = hash_ptr(cookie, IDEM_HASH_BITS); @@ -3109,27 +3109,18 @@ static void idempotent_complete(struct idempotent *u, int ret) complete(&pos->complete); } spin_unlock(&idem_lock); + return ret; } static int init_module_from_file(struct file *f, const char __user * uargs, int flags) { - struct idempotent idem; struct load_info info = { }; void *buf = NULL; - int len, ret; - - if (!f || !(f->f_mode & FMODE_READ)) - return -EBADF; - - if (idempotent(&idem, file_inode(f))) { - wait_for_completion(&idem.complete); - return idem.ret; - } + int len; len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE); if (len < 0) { mod_stat_inc(&failed_kreads); - mod_stat_add_long(len, &invalid_kread_bytes); return len; } @@ -3146,9 +3137,25 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int info.len = len; } - ret = load_module(&info, uargs, flags); - idempotent_complete(&idem, ret); - return ret; + return load_module(&info, uargs, flags); +} + +static int idempotent_init_module(struct file *f, const char __user * uargs, int flags) +{ + struct idempotent idem; + + if (!f || !(f->f_mode & FMODE_READ)) + return -EBADF; + + /* See if somebody else is doing the operation? */ + if (idempotent(&idem, file_inode(f))) { + wait_for_completion(&idem.complete); + return idem.ret; + } + + /* Otherwise, we'll do it and complete others */ + return idempotent_complete(&idem, + init_module_from_file(f, uargs, flags)); } SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags) @@ -3168,7 +3175,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags) return -EINVAL; f = fdget(fd); - err = init_module_from_file(f.file, uargs, flags); + err = idempotent_init_module(f.file, uargs, flags); fdput(f); return err; } -- cgit v1.2.3