* [PATCH v2 1/4] kmod: make request_module() return an error when autoloading is disabled [not found] <20200312202552.241885-1-ebiggers@kernel.org> @ 2020-03-12 20:25 ` Eric Biggers 2020-03-13 0:57 ` Luis Chamberlain 2020-03-12 20:25 ` [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once() Eric Biggers 1 sibling, 1 reply; 6+ messages in thread From: Eric Biggers @ 2020-03-12 20:25 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook, Luis Chamberlain, NeilBrown, stable 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. 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: Luis Chamberlain <mcgrof@kernel.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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/4] kmod: make request_module() return an error when autoloading is disabled 2020-03-12 20:25 ` [PATCH v2 1/4] kmod: make request_module() return an error when autoloading is disabled Eric Biggers @ 2020-03-13 0:57 ` Luis Chamberlain 0 siblings, 0 replies; 6+ messages in thread From: Luis Chamberlain @ 2020-03-13 0:57 UTC (permalink / raw) To: Eric Biggers Cc: linux-kernel, linux-fsdevel, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook, NeilBrown, stable On Thu, Mar 12, 2020 at 01:25:49PM -0700, Eric Biggers wrote: > 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. > > 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: Luis Chamberlain <mcgrof@kernel.org> > Cc: NeilBrown <neilb@suse.com> > Signed-off-by: Eric Biggers <ebiggers@google.com> Acked-by: Luis Chamberlain <mcgrof@kernel.org> Luis ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once() [not found] <20200312202552.241885-1-ebiggers@kernel.org> 2020-03-12 20:25 ` [PATCH v2 1/4] kmod: make request_module() return an error when autoloading is disabled Eric Biggers @ 2020-03-12 20:25 ` Eric Biggers 2020-03-12 22:06 ` NeilBrown 2020-03-13 0:58 ` Luis Chamberlain 1 sibling, 2 replies; 6+ messages in thread From: Eric Biggers @ 2020-03-12 20:25 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook, Luis Chamberlain, NeilBrown, stable 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(). 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: Luis Chamberlain <mcgrof@kernel.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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once() 2020-03-12 20:25 ` [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once() Eric Biggers @ 2020-03-12 22:06 ` NeilBrown 2020-03-13 1:00 ` Luis Chamberlain 2020-03-13 0:58 ` Luis Chamberlain 1 sibling, 1 reply; 6+ messages in thread From: NeilBrown @ 2020-03-12 22:06 UTC (permalink / raw) To: Eric Biggers, linux-kernel Cc: linux-fsdevel, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook, Luis Chamberlain, NeilBrown, stable [-- Attachment #1: Type: text/plain, Size: 1915 bytes --] On Thu, Mar 12 2020, Eric Biggers wrote: > 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(). > > 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: Luis Chamberlain <mcgrof@kernel.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); I strongly support the replacement of "WARN" by "pr_warn". I wonder if we really want the "once" now. Possibly using rate_limited would be justified, but I think that in general we should see a warning every time this event happens. Thanks, NeilBrown > } > > if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) { > -- > 2.25.1 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once() 2020-03-12 22:06 ` NeilBrown @ 2020-03-13 1:00 ` Luis Chamberlain 0 siblings, 0 replies; 6+ messages in thread From: Luis Chamberlain @ 2020-03-13 1:00 UTC (permalink / raw) To: NeilBrown Cc: Eric Biggers, linux-kernel, linux-fsdevel, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook, NeilBrown, stable [-- Attachment #1: Type: text/plain, Size: 2195 bytes --] On Fri, Mar 13, 2020 at 09:06:46AM +1100, NeilBrown wrote: > On Thu, Mar 12 2020, Eric Biggers wrote: > > > 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(). > > > > 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: Luis Chamberlain <mcgrof@kernel.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); > > I strongly support the replacement of "WARN" by "pr_warn". > I wonder if we really want the "once" now. Possibly using rate_limited > would be justified, but I think that in general we should see a warning > every time this event happens. Since the usefulness of the print is at boot, I think pr_warn_once() is good right now but just because I cannot think of a case where multiple prints are currently desirable, or where this should be possible post-boot. Can you? Luis [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once() 2020-03-12 20:25 ` [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once() Eric Biggers 2020-03-12 22:06 ` NeilBrown @ 2020-03-13 0:58 ` Luis Chamberlain 1 sibling, 0 replies; 6+ messages in thread From: Luis Chamberlain @ 2020-03-13 0:58 UTC (permalink / raw) To: Eric Biggers Cc: linux-kernel, linux-fsdevel, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook, NeilBrown, stable On Thu, Mar 12, 2020 at 01:25:50PM -0700, Eric Biggers wrote: > 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(). > > 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: Luis Chamberlain <mcgrof@kernel.org> > Cc: NeilBrown <neilb@suse.com> > Signed-off-by: Eric Biggers <ebiggers@google.com> Acked-by: Luis Chamberlain <mcgrof@kernel.org> Luis ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-13 1:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200312202552.241885-1-ebiggers@kernel.org>
2020-03-12 20:25 ` [PATCH v2 1/4] kmod: make request_module() return an error when autoloading is disabled Eric Biggers
2020-03-13 0:57 ` Luis Chamberlain
2020-03-12 20:25 ` [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once() Eric Biggers
2020-03-12 22:06 ` NeilBrown
2020-03-13 1:00 ` Luis Chamberlain
2020-03-13 0:58 ` Luis Chamberlain
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).