| Message ID | 20230704100852.23452-1-vegard.nossum@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | module: always complete idempotent loads | expand |
On Tue, 4 Jul 2023 at 03:09, Vegard Nossum <vegard.nossum@oracle.com> wrote: > > Commit 9b9879fc0327 added a hashtable storing lists of concurrent module > loads. However, it didn't fix up all the error paths in > init_module_from_file(); this would lead to leaving the function while an > on-stack 'struct idempotent' element is still in the hash table, which > leads to all sorts of badness as spotted by syzkaller: You are of course 100% right. However, I'd rather just use a wrapper function and make this thing much clearer. Like I should have done originally. So I'd be inclined towards a patch like the attached instead. Works for you? Linus
On 7/4/23 15:37, Linus Torvalds wrote: > On Tue, 4 Jul 2023 at 03:09, Vegard Nossum <vegard.nossum@oracle.com> wrote: >> >> Commit 9b9879fc0327 added a hashtable storing lists of concurrent module >> loads. However, it didn't fix up all the error paths in >> init_module_from_file(); this would lead to leaving the function while an >> on-stack 'struct idempotent' element is still in the hash table, which >> leads to all sorts of badness as spotted by syzkaller: > > You are of course 100% right. > > However, I'd rather just use a wrapper function and make this thing > much clearer. Like I should have done originally. > > So I'd be inclined towards a patch like the attached instead. Works for you? Looks mostly good. This bit is now included inside the concurrency check: if (!f || !(f->f_mode & FMODE_READ)) return -EBADF; Since the cookie is file_inode(f) I think that means that you could have one caller without FMODE_READ hit this check and it would potentially return -EBADF even for other callers who did open the file properly. Maybe just do the f_mode check in finit_module()? Or... new helper, fdget_mode()?? Apart from this, there is another bit that looks a bit weird: 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); I don't think we should be adding error codes to byte counts. Vegard
On Tue, 4 Jul 2023 at 08:12, Vegard Nossum <vegard.nossum@oracle.com> wrote: > > Maybe just do the f_mode check in finit_module()? Or... new helper, > fdget_mode()?? I actually wanted to do a fdget_read/write() helper long ago: we already basically pass in a "this mode is not ok" flag for the FMODE_PATH case, and it's sad how many extra "do we have FMODE_READ/WRITE" tests we have when it would actually make tons of sense to just check it at fdget() time. But I created the patch, and then decided it was just churn for something that didn't matter a lot. The existing "mode" argument to __fget_light() and __fget() would end up split into "error if these bits are set" (existing) and "error if these bits are _not_ set" (new) arguments. It was straightforward, but I looked at the patch, and then looked at all the existing users that currently check the error separately, and went "bah", and threw it all away. Which is not to say it's not the right thing to do. Maybe we should at some point. But doing it right (ie doing it in that helper before we even bother with reference counting etc) is just a bit too much work. The alternative is, of course, to just have a *truly* stupid wrapper that does the error checking and does the "fdput()" if FMODE_READ wasn't set. But that's just disgusting. Anyway, for this module case, I just moved it out to the caller. > Apart from this, there is another bit that looks a bit weird: > > 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); > > I don't think we should be adding error codes to byte counts. Ack. I fixed that at the same time as a "multiple problems with the error paths". Linus
On 7/4/23 15:37, Linus Torvalds wrote: > On Tue, 4 Jul 2023 at 03:09, Vegard Nossum <vegard.nossum@oracle.com> wrote: >> >> Commit 9b9879fc0327 added a hashtable storing lists of concurrent module >> loads. However, it didn't fix up all the error paths in >> init_module_from_file(); this would lead to leaving the function while an >> on-stack 'struct idempotent' element is still in the hash table, which >> leads to all sorts of badness as spotted by syzkaller: > > You are of course 100% right. > > However, I'd rather just use a wrapper function and make this thing > much clearer. Like I should have done originally. > > So I'd be inclined towards a patch like the attached instead. Works for you? Harshit tells me there's still a crash... and indeed, with your wrapped version we call file_inode() on a NULL pointer before checking the fdget() return value. It's likely you already changed this with the f_mode changes I commented on, so maybe it's no longer a problem, though. Vegard
On Tue, 4 Jul 2023 at 11:29, Vegard Nossum <vegard.nossum@oracle.com> wrote: > > It's likely you already changed this with the f_mode changes I commented > on, so maybe it's no longer a problem, though. Yes I did. Let me just push out the stuff I have pending, including that fixed-up commit. [ Short time passes ] Done. Linus
diff --git a/kernel/module/main.c b/kernel/module/main.c index d6de66a6a1ac..6100d0373f2f 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -3121,39 +3121,42 @@ static void idempotent_complete(struct idempotent *u, int 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; } 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; + ret = len; + goto out; } if (flags & MODULE_INIT_COMPRESSED_FILE) { int err = module_decompress(&info, buf, len); vfree(buf); /* compressed data is no longer needed */ if (err) { mod_stat_inc(&failed_decompress); mod_stat_add_long(len, &invalid_decompress_bytes); - return err; + ret = err; + goto out; } } else { info.hdr = buf; info.len = len; } ret = load_module(&info, uargs, flags); +out: idempotent_complete(&idem, ret); return ret; }
Commit 9b9879fc0327 added a hashtable storing lists of concurrent module loads. However, it didn't fix up all the error paths in init_module_from_file(); this would lead to leaving the function while an on-stack 'struct idempotent' element is still in the hash table, which leads to all sorts of badness as spotted by syzkaller: 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 KASAN: slab-use-after-free Read in init_module_from_file KASAN: slab-use-after-free Read in set_powered_sync KASAN: stack-out-of-bounds Read in init_module_from_file KASAN: use-after-free in init_module_from_file KASAN: use-after-free Read in init_module_from_file Fixes: 9b9879fc0327 ("modules: catch concurrent module loads, treat them as idempotent") Reported-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> Reported-by: syzbot+9c2bdc9d24e4a7abe741@syzkaller.appspotmail.com Tested-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> Cc: Johan Hovold <johan@kernel.org> Cc: Johan Hovold <johan@kernel.org> Cc: Luis Chamberlain <mcgrof@kernel.org> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Rudi Heitbaum <rudi@heitbaum.com> Cc: David Hildenbrand <david@redhat.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com> --- kernel/module/main.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) --