tjh.dev / kernel
Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

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 <vegard.nossum@oracle.com>
Reported-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Reported-by: syzbot+9c2bdc9d24e4a7abe741@syzkaller.appspotmail.com
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Linus Torvalds f1962207 b5641a5d

+23 -16
+23 -16
kernel/module/main.c
··· 3092 3092 * remove everybody - which includes ourselves - fill in the return 3093 3093 * value, and then complete the operation. 3094 3094 */ 3095 - static void idempotent_complete(struct idempotent *u, int ret) 3095 + static int idempotent_complete(struct idempotent *u, int ret) 3096 3096 { 3097 3097 const void *cookie = u->cookie; 3098 3098 int hash = hash_ptr(cookie, IDEM_HASH_BITS); ··· 3109 3109 complete(&pos->complete); 3110 3110 } 3111 3111 spin_unlock(&idem_lock); 3112 + return ret; 3112 3113 } 3113 3114 3114 3115 static int init_module_from_file(struct file *f, const char __user * uargs, int flags) 3115 3116 { 3116 - struct idempotent idem; 3117 3117 struct load_info info = { }; 3118 3118 void *buf = NULL; 3119 - int len, ret; 3120 - 3121 - if (!f || !(f->f_mode & FMODE_READ)) 3122 - return -EBADF; 3123 - 3124 - if (idempotent(&idem, file_inode(f))) { 3125 - wait_for_completion(&idem.complete); 3126 - return idem.ret; 3127 - } 3119 + int len; 3128 3120 3129 3121 len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE); 3130 3122 if (len < 0) { 3131 3123 mod_stat_inc(&failed_kreads); 3132 - mod_stat_add_long(len, &invalid_kread_bytes); 3133 3124 return len; 3134 3125 } 3135 3126 ··· 3137 3146 info.len = len; 3138 3147 } 3139 3148 3140 - ret = load_module(&info, uargs, flags); 3141 - idempotent_complete(&idem, ret); 3142 - return ret; 3149 + return load_module(&info, uargs, flags); 3150 + } 3151 + 3152 + static int idempotent_init_module(struct file *f, const char __user * uargs, int flags) 3153 + { 3154 + struct idempotent idem; 3155 + 3156 + if (!f || !(f->f_mode & FMODE_READ)) 3157 + return -EBADF; 3158 + 3159 + /* See if somebody else is doing the operation? */ 3160 + if (idempotent(&idem, file_inode(f))) { 3161 + wait_for_completion(&idem.complete); 3162 + return idem.ret; 3163 + } 3164 + 3165 + /* Otherwise, we'll do it and complete others */ 3166 + return idempotent_complete(&idem, 3167 + init_module_from_file(f, uargs, flags)); 3143 3168 } 3144 3169 3145 3170 SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags) ··· 3175 3168 return -EINVAL; 3176 3169 3177 3170 f = fdget(fd); 3178 - err = init_module_from_file(f.file, uargs, flags); 3171 + err = idempotent_init_module(f.file, uargs, flags); 3179 3172 fdput(f); 3180 3173 return err; 3181 3174 }