[PATCH] module: always complete idempotent loads

[PATCH] module: always complete idempotent loads

[Vegard Nossum]

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(-)

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;
 }
-- 

Re: [PATCH] module: always complete idempotent loads

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 672 bytes --]

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

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2367 bytes --]

 kernel/module/main.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 834de86ebe35..500a6a3c2987 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,23 +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;
+	int len;
 
 	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);
@@ -3146,9 +3141,22 @@ 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;
+
+	/* 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 +3176,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;
 }

Re: [PATCH] module: always complete idempotent loads

[Vegard Nossum]

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

Re: [PATCH] module: always complete idempotent loads

[Linus Torvalds]

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

Re: [PATCH] module: always complete idempotent loads

[Vegard Nossum]

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

Re: [PATCH] module: always complete idempotent loads

[Linus Torvalds]

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