[PATCH v3 1/5] kmod: make request_module() return an error when autoloading is disabled

[PATCH v3 1/5] kmod: make request_module() return an error when autoloading is disabled

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

It's long been possible to disable kernel module autoloading completely
(while still allowing manual module insertion) by setting
/proc/sys/kernel/modprobe to the empty string.  This can be preferable
to setting it to a nonexistent file since it avoids the overhead of an
attempted execve(), avoids potential deadlocks, and avoids the call to
security_kernel_module_request() and thus on SELinux-based systems
eliminates the need to write SELinux rules to dontaudit module_request.

However, when module autoloading is disabled in this way,
request_module() returns 0.  This is broken because callers expect 0 to
mean that the module was successfully loaded.

Apparently this was never noticed because this method of disabling
module autoloading isn't used much, and also most callers don't use the
return value of request_module() since it's always necessary to check
whether the module registered its functionality or not anyway.  But
improperly returning 0 can indeed confuse a few callers, for example
get_fs_type() in fs/filesystems.c where it causes a WARNING to be hit:

	if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
		fs = __get_fs_type(name, len);
		WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no fs?\n", len, name);
	}

This is easily reproduced with:

	echo > /proc/sys/kernel/modprobe
	mount -t NONEXISTENT none /

It causes:

	request_module fs-NONEXISTENT succeeded, but still no fs?
	WARNING: CPU: 1 PID: 1106 at fs/filesystems.c:275 get_fs_type+0xd6/0xf0
	[...]

This should actually use pr_warn_once() rather than WARN_ONCE(), since
it's also user-reachable if userspace immediately unloads the module.
Regardless, request_module() should correctly return an error when it
fails.  So let's make it return -ENOENT, which matches the error when
the modprobe binary doesn't exist.

I've also sent patches to document and test this case.

Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Cc: stable@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jeff Vander Stoep <jeffv@google.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: NeilBrown <neilb@suse.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 kernel/kmod.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index bc6addd9152b4..a2de58de6ab62 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -120,7 +120,7 @@ static int call_modprobe(char *module_name, int wait)
  * invoke it.
  *
  * If module auto-loading support is disabled then this function
- * becomes a no-operation.
+ * simply returns -ENOENT.
  */
 int __request_module(bool wait, const char *fmt, ...)
 {
@@ -137,7 +137,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	WARN_ON_ONCE(wait && current_is_async());
 
 	if (!modprobe_path[0])
-		return 0;
+		return -ENOENT;
 
 	va_start(args, fmt);
 	ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
-- 
2.25.1

Re: [PATCH v3 1/5] kmod: make request_module() return an error when autoloading is disabled

[Jessica Yu]

+++ Eric Biggers [14/03/20 14:34 -0700]:
>From: Eric Biggers <ebiggers@google.com>
>
>It's long been possible to disable kernel module autoloading completely
>(while still allowing manual module insertion) by setting
>/proc/sys/kernel/modprobe to the empty string.  This can be preferable
>to setting it to a nonexistent file since it avoids the overhead of an
>attempted execve(), avoids potential deadlocks, and avoids the call to
>security_kernel_module_request() and thus on SELinux-based systems
>eliminates the need to write SELinux rules to dontaudit module_request.
>
>However, when module autoloading is disabled in this way,
>request_module() returns 0.  This is broken because callers expect 0 to
>mean that the module was successfully loaded.
>
>Apparently this was never noticed because this method of disabling
>module autoloading isn't used much, and also most callers don't use the
>return value of request_module() since it's always necessary to check
>whether the module registered its functionality or not anyway.  But
>improperly returning 0 can indeed confuse a few callers, for example
>get_fs_type() in fs/filesystems.c where it causes a WARNING to be hit:
>
>	if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
>		fs = __get_fs_type(name, len);
>		WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no fs?\n", len, name);
>	}
>
>This is easily reproduced with:
>
>	echo > /proc/sys/kernel/modprobe
>	mount -t NONEXISTENT none /
>
>It causes:
>
>	request_module fs-NONEXISTENT succeeded, but still no fs?
>	WARNING: CPU: 1 PID: 1106 at fs/filesystems.c:275 get_fs_type+0xd6/0xf0
>	[...]
>
>This should actually use pr_warn_once() rather than WARN_ONCE(), since
>it's also user-reachable if userspace immediately unloads the module.
>Regardless, request_module() should correctly return an error when it
>fails.  So let's make it return -ENOENT, which matches the error when
>the modprobe binary doesn't exist.
>
>I've also sent patches to document and test this case.
>
>Acked-by: Luis Chamberlain <mcgrof@kernel.org>
>Cc: stable@vger.kernel.org
>Cc: Alexei Starovoitov <ast@kernel.org>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Jeff Vander Stoep <jeffv@google.com>
>Cc: Jessica Yu <jeyu@kernel.org>
>Cc: Kees Cook <keescook@chromium.org>
>Cc: NeilBrown <neilb@suse.com>
>Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Jessica Yu <jeyu@kernel.org>

[PATCH v3 2/5] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

After request_module(), nothing is stopping the module from being
unloaded until someone takes a reference to it via try_get_module().

The WARN_ONCE() in get_fs_type() is thus user-reachable, via userspace
running 'rmmod' concurrently.

Since WARN_ONCE() is for kernel bugs only, not for user-reachable
situations, downgrade this warning to pr_warn_once().

Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Cc: stable@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jeff Vander Stoep <jeffv@google.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: NeilBrown <neilb@suse.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/filesystems.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/filesystems.c b/fs/filesystems.c
index 77bf5f95362da..90b8d879fbaf3 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -272,7 +272,9 @@ struct file_system_type *get_fs_type(const char *name)
 	fs = __get_fs_type(name, len);
 	if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
 		fs = __get_fs_type(name, len);
-		WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no fs?\n", len, name);
+		if (!fs)
+			pr_warn_once("request_module fs-%.*s succeeded, but still no fs?\n",
+				     len, name);
 	}
 
 	if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
-- 
2.25.1

Re: [PATCH v3 2/5] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once()

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.5.9, v5.4.25, v4.19.109, v4.14.173, v4.9.216, v4.4.216.

v5.5.9: Build OK!
v5.4.25: Build OK!
v4.19.109: Build OK!
v4.14.173: Build OK!
v4.9.216: Failed to apply! Possible dependencies:
    41124db869b7 ("fs: warn in case userspace lied about modprobe return")

v4.4.216: Failed to apply! Possible dependencies:
    41124db869b7 ("fs: warn in case userspace lied about modprobe return")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

Re: [PATCH v3 2/5] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once()

[Jessica Yu]

+++ Sasha Levin [17/03/20 22:30 +0000]:
>Hi
>
>[This is an automated email]
>
>This commit has been processed because it contains a -stable tag.
>The stable tag indicates that it's relevant for the following trees: all
>
>The bot has tested the following trees: v5.5.9, v5.4.25, v4.19.109, v4.14.173, v4.9.216, v4.4.216.
>
>v5.5.9: Build OK!
>v5.4.25: Build OK!
>v4.19.109: Build OK!
>v4.14.173: Build OK!
>v4.9.216: Failed to apply! Possible dependencies:
>    41124db869b7 ("fs: warn in case userspace lied about modprobe return")
>
>v4.4.216: Failed to apply! Possible dependencies:
>    41124db869b7 ("fs: warn in case userspace lied about modprobe return")
>
>
>NOTE: The patch will not be queued to stable trees until it is upstream.
>
>How should we proceed with this patch?

Since commit 41124db869b7 was introduced v4.13, I guess we should
change the stable tag to:

Cc: stable@vger.kernel.org # v4.13+

Re: [PATCH v3 2/5] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once()

[Eric Biggers]

On Wed, Mar 18, 2020 at 04:09:26PM +0100, Jessica Yu wrote:
> +++ Sasha Levin [17/03/20 22:30 +0000]:
> > Hi
> > 
> > [This is an automated email]
> > 
> > This commit has been processed because it contains a -stable tag.
> > The stable tag indicates that it's relevant for the following trees: all
> > 
> > The bot has tested the following trees: v5.5.9, v5.4.25, v4.19.109, v4.14.173, v4.9.216, v4.4.216.
> > 
> > v5.5.9: Build OK!
> > v5.4.25: Build OK!
> > v4.19.109: Build OK!
> > v4.14.173: Build OK!
> > v4.9.216: Failed to apply! Possible dependencies:
> >    41124db869b7 ("fs: warn in case userspace lied about modprobe return")
> > 
> > v4.4.216: Failed to apply! Possible dependencies:
> >    41124db869b7 ("fs: warn in case userspace lied about modprobe return")
> > 
> > 
> > NOTE: The patch will not be queued to stable trees until it is upstream.
> > 
> > How should we proceed with this patch?
> 
> Since commit 41124db869b7 was introduced v4.13, I guess we should
> change the stable tag to:
> 
> Cc: stable@vger.kernel.org # v4.13+
> 

It should use:

 Fixes: 41124db869b7 ("fs: warn in case userspace lied about modprobe return")
 Cc: <stable@vger.kernel.org> # v4.13+

I'll add it.

Re: [PATCH v3 2/5] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once()

[Jessica Yu]

+++ Eric Biggers [14/03/20 14:34 -0700]:
>From: Eric Biggers <ebiggers@google.com>
>
>After request_module(), nothing is stopping the module from being
>unloaded until someone takes a reference to it via try_get_module().
>
>The WARN_ONCE() in get_fs_type() is thus user-reachable, via userspace
>running 'rmmod' concurrently.
>
>Since WARN_ONCE() is for kernel bugs only, not for user-reachable
>situations, downgrade this warning to pr_warn_once().
>
>Acked-by: Luis Chamberlain <mcgrof@kernel.org>
>Cc: stable@vger.kernel.org
>Cc: Alexei Starovoitov <ast@kernel.org>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Jeff Vander Stoep <jeffv@google.com>
>Cc: Jessica Yu <jeyu@kernel.org>
>Cc: Kees Cook <keescook@chromium.org>
>Cc: NeilBrown <neilb@suse.com>
>Signed-off-by: Eric Biggers <ebiggers@google.com>
>---
> fs/filesystems.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/fs/filesystems.c b/fs/filesystems.c
>index 77bf5f95362da..90b8d879fbaf3 100644
>--- a/fs/filesystems.c
>+++ b/fs/filesystems.c
>@@ -272,7 +272,9 @@ struct file_system_type *get_fs_type(const char *name)
> 	fs = __get_fs_type(name, len);
> 	if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
> 		fs = __get_fs_type(name, len);
>-		WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no fs?\n", len, name);
>+		if (!fs)
>+			pr_warn_once("request_module fs-%.*s succeeded, but still no fs?\n",
>+				     len, name);

Hm, what was the rationale for warning only once again? It might be useful
for debugging issues to see each instance of request_module() failure
(and with which fs). However, I don't have a concrete use case to
support this argument, so:

Reviewed-by: Jessica Yu <jeyu@kernel.org>

Re: [PATCH v3 2/5] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once()

[Eric Biggers]

On Wed, Mar 18, 2020 at 04:43:15PM +0100, Jessica Yu wrote:
> +++ Eric Biggers [14/03/20 14:34 -0700]:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > After request_module(), nothing is stopping the module from being
> > unloaded until someone takes a reference to it via try_get_module().
> > 
> > The WARN_ONCE() in get_fs_type() is thus user-reachable, via userspace
> > running 'rmmod' concurrently.
> > 
> > Since WARN_ONCE() is for kernel bugs only, not for user-reachable
> > situations, downgrade this warning to pr_warn_once().
> > 
> > Acked-by: Luis Chamberlain <mcgrof@kernel.org>
> > Cc: stable@vger.kernel.org
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jeff Vander Stoep <jeffv@google.com>
> > Cc: Jessica Yu <jeyu@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: NeilBrown <neilb@suse.com>
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> > fs/filesystems.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/filesystems.c b/fs/filesystems.c
> > index 77bf5f95362da..90b8d879fbaf3 100644
> > --- a/fs/filesystems.c
> > +++ b/fs/filesystems.c
> > @@ -272,7 +272,9 @@ struct file_system_type *get_fs_type(const char *name)
> > 	fs = __get_fs_type(name, len);
> > 	if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
> > 		fs = __get_fs_type(name, len);
> > -		WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no fs?\n", len, name);
> > +		if (!fs)
> > +			pr_warn_once("request_module fs-%.*s succeeded, but still no fs?\n",
> > +				     len, name);
> 
> Hm, what was the rationale for warning only once again? It might be useful
> for debugging issues to see each instance of request_module() failure
> (and with which fs). However, I don't have a concrete use case to
> support this argument, so:
> 
> Reviewed-by: Jessica Yu <jeyu@kernel.org>

This was discussed on v2, see
https://lkml.kernel.org/lkml/20200313010053.GS11244@42.do-not-panic.com/.
If the warning triggers, then it indicates a broken modprobe program.
Printing the warning multiple times wouldn't really add any new information.

And in any case, it's printed once both before and after this patch.

- Eric