* [PATCH] kmod: make request_module() return an error when autoloading is disabled @ 2020-03-10 22:37 Eric Biggers 2020-03-11 4:32 ` Luis Chamberlain 2020-03-11 17:28 ` Kees Cook 0 siblings, 2 replies; 15+ messages in thread From: Eric Biggers @ 2020-03-10 22:37 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook, Luis Chamberlain From: Eric Biggers <ebiggers@google.com> It's long been possible to disable kernel module autoloading completely 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 [...] Arguably this warning is broken and should be removed, since the module could have been unloaded already. However, request_module() should also 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. 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> 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 bc6addd9152b..a2de58de6ab6 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.481.gfbce0eb801-goog ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-10 22:37 [PATCH] kmod: make request_module() return an error when autoloading is disabled Eric Biggers @ 2020-03-11 4:32 ` Luis Chamberlain 2020-03-11 5:26 ` Eric Biggers 2020-03-11 5:55 ` Josh Triplett 2020-03-11 17:28 ` Kees Cook 1 sibling, 2 replies; 15+ messages in thread From: Luis Chamberlain @ 2020-03-11 4:32 UTC (permalink / raw) To: Eric Biggers, NeilBrown, Josh Triplett Cc: linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > It's long been possible to disable kernel module autoloading completely > by setting /proc/sys/kernel/modprobe to the empty string. This can be > preferable preferable but ... not documented. Or was this documented or recommended somewhere? > 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. However this is implicitly not true. For instance, as Neil recently chased down -- blacklisting a module today returns 0 as well, and so this corner case is implicitly set to return 0. > 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. Right, the de-facto practice of verification of a module to be loaded is for each caller to ensure with whatever heuristic it needs to ensure the module is loaded. > 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 > [...] Thanks for reporting this. > Arguably this warning is broken and should be removed, since the module > could have been unloaded already. No, the warning is present *because* debuggins issues for when the module which did not load is a rootfs is *really* hard to debug. Then, if the culprit of the issue is a userspace modprobe bug (it happens) this makes debugging *very* difficult as you won't know what failed at all, you just get a silent failed boot. > However, request_module() should also > 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. This is a user experience change though, and I wouldn't have on my radar who would use this, and expects the old behaviour. Josh, would you by chance? I'd like this to be more an RFC first so we get vetted parties to review. I take it this and Neil's case are cases we should revisit now, properly document as we didn't before, ensure we don't break anything, and also extend the respective kmod selftests to ensure we don't break these corner cases in the future. > 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> > 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 bc6addd9152b..a2de58de6ab6 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.481.gfbce0eb801-goog > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 4:32 ` Luis Chamberlain @ 2020-03-11 5:26 ` Eric Biggers 2020-03-11 6:31 ` Luis Chamberlain 2020-03-11 5:55 ` Josh Triplett 1 sibling, 1 reply; 15+ messages in thread From: Eric Biggers @ 2020-03-11 5:26 UTC (permalink / raw) To: Luis Chamberlain Cc: NeilBrown, Josh Triplett, linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook On Wed, Mar 11, 2020 at 04:32:21AM +0000, Luis Chamberlain wrote: > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > It's long been possible to disable kernel module autoloading completely > > by setting /proc/sys/kernel/modprobe to the empty string. This can be > > preferable > > preferable but ... not documented. Or was this documented or recommended > somewhere? > > > 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. Not that I know of, though I didn't look too hard. proc(5) mentions /proc/sys/kernel/modprobe but doesn't mention the empty string case. In any case, it's been supported for a long time, and it's useful for the reasons I mentioned. > > > > 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. > > However this is implicitly not true. For instance, as Neil recently > chased down -- blacklisting a module today returns 0 as well, and so > this corner case is implicitly set to return 0. That sounds like another similar bug, but in the modprobe program instead of in the kernel. Do you have a link to the discussion about it? > > > 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 > > [...] > > Thanks for reporting this. > > > Arguably this warning is broken and should be removed, since the module > > could have been unloaded already. > > No, the warning is present *because* debuggins issues for when the > module which did not load is a rootfs is *really* hard to debug. Then, > if the culprit of the issue is a userspace modprobe bug (it happens) > this makes debugging *very* difficult as you won't know what failed at > all, you just get a silent failed boot. I meant that it's broken to use WARN_ON(), because it's a userspace triggerable condition. WARN_ON() is for kernel bugs only. Of course, if it's a useful warning, it can still be left in as pr_warn(). > > However, request_module() should also > > 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. > > This is a user experience change though, and I wouldn't have on my radar > who would use this, and expects the old behaviour. Josh, would you by > chance? > > I'd like this to be more an RFC first so we get vetted parties to > review. I take it this and Neil's case are cases we should revisit now, > properly document as we didn't before, ensure we don't break anything, > and also extend the respective kmod selftests to ensure we don't break > these corner cases in the future. This patch only affects kernel internals, not the userspace API. So I don't see why it would be controversial? I already went through all callers of request_module() that check its return value, and they all appear to work better with -ENOENT, since they assume that 0 means the module was loaded. Incorrectly returning 0 typically causes unnecessary work (checking again whether the module's functionality is available) or misleading log messages. In fact, I can't think of a situation where kernel code would *want* 0 returned in this case, as it's ambiguous with the module being successfully loaded. Sure, I'll check whether it would be possible to add a test for this case in lib/test_kmod.c and tools/testing/selftests/kmod/. - Eric ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 5:26 ` Eric Biggers @ 2020-03-11 6:31 ` Luis Chamberlain 2020-03-11 17:35 ` Eric Biggers 0 siblings, 1 reply; 15+ messages in thread From: Luis Chamberlain @ 2020-03-11 6:31 UTC (permalink / raw) To: Eric Biggers Cc: NeilBrown, Josh Triplett, linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook On Tue, Mar 10, 2020 at 10:26:20PM -0700, Eric Biggers wrote: > On Wed, Mar 11, 2020 at 04:32:21AM +0000, Luis Chamberlain wrote: > > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > It's long been possible to disable kernel module autoloading completely > > > by setting /proc/sys/kernel/modprobe to the empty string. This can be > > > preferable > > > > preferable but ... not documented. Or was this documented or recommended > > somewhere? > > > > > 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. > > Not that I know of, though I didn't look too hard. proc(5) mentions > /proc/sys/kernel/modprobe but doesn't mention the empty string case. > > In any case, it's been supported for a long time, and it's useful for the > reasons I mentioned. Sure. I think then its important to document it as such then, or perhaps make a kconfig option which sets this to empty and document it on the kconfig entry. > > > 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. > > > > However this is implicitly not true. For instance, as Neil recently > > chased down -- blacklisting a module today returns 0 as well, and so > > this corner case is implicitly set to return 0. > > That sounds like another similar bug, but in the modprobe program instead of in > the kernel. Do you have a link to the discussion about it? Nothing public yet AFAICT. > > > 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 > > > [...] > > > > Thanks for reporting this. > > > > > Arguably this warning is broken and should be removed, since the module > > > could have been unloaded already. > > > > No, the warning is present *because* debuggins issues for when the > > module which did not load is a rootfs is *really* hard to debug. Then, > > if the culprit of the issue is a userspace modprobe bug (it happens) > > this makes debugging *very* difficult as you won't know what failed at > > all, you just get a silent failed boot. > > I meant that it's broken to use WARN_ON(), because it's a userspace triggerable > condition. This and the blacklist case are now two known cases, so yes I'a agree now. It was not widely known before. > WARN_ON() is for kernel bugs only. Of course, if it's a useful > warning, it can still be left in as pr_warn(). I'll send a patch. > > > However, request_module() should also > > > 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. > > > > This is a user experience change though, and I wouldn't have on my radar > > who would use this, and expects the old behaviour. Josh, would you by > > chance? > > > > I'd like this to be more an RFC first so we get vetted parties to > > review. I take it this and Neil's case are cases we should revisit now, > > properly document as we didn't before, ensure we don't break anything, > > and also extend the respective kmod selftests to ensure we don't break > > these corner cases in the future. > > This patch only affects kernel internals, not the userspace API. Ah yes, in that case this seems fine with me. > So I don't see > why it would be controversial? I already went through all callers of > request_module() that check its return value, and they all appear to work better > with -ENOENT, since they assume that 0 means the module was loaded. Thanks for doing that, but I note that getting 0 is not assurance either. The de-facto best practive for the request_module() call is to do your own in place verifier. > Incorrectly returning 0 typically causes unnecessary work (checking again > whether the module's functionality is available) or misleading log messages. Yes but returning 0 cannot be relied upon today for assuming the module is loaded. *If* we revisit that decision and want the kernel to do a generic verifier, then yes, we can get rid of all the caller specific verfifiers, but not today. > In > fact, I can't think of a situation where kernel code would *want* 0 returned in > this case, as it's ambiguous with the module being successfully loaded. Unfortunately that's just how the API (to my mind silly) grew out to. > Sure, I'll check whether it would be possible to add a test for this case in > lib/test_kmod.c and tools/testing/selftests/kmod/. Thanks! Luis ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 6:31 ` Luis Chamberlain @ 2020-03-11 17:35 ` Eric Biggers 2020-03-11 18:00 ` Luis Chamberlain 0 siblings, 1 reply; 15+ messages in thread From: Eric Biggers @ 2020-03-11 17:35 UTC (permalink / raw) To: Luis Chamberlain Cc: NeilBrown, Josh Triplett, linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook On Wed, Mar 11, 2020 at 06:31:30AM +0000, Luis Chamberlain wrote: > On Tue, Mar 10, 2020 at 10:26:20PM -0700, Eric Biggers wrote: > > On Wed, Mar 11, 2020 at 04:32:21AM +0000, Luis Chamberlain wrote: > > > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > > > > From: Eric Biggers <ebiggers@google.com> > > > > > > > > It's long been possible to disable kernel module autoloading completely > > > > by setting /proc/sys/kernel/modprobe to the empty string. This can be > > > > preferable > > > > > > preferable but ... not documented. Or was this documented or recommended > > > somewhere? > > > > > > > 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. > > > > Not that I know of, though I didn't look too hard. proc(5) mentions > > /proc/sys/kernel/modprobe but doesn't mention the empty string case. > > > > In any case, it's been supported for a long time, and it's useful for the > > reasons I mentioned. > > Sure. I think then its important to document it as such then, or perhaps > make a kconfig option which sets this to empty and document it on the > kconfig entry. I'll send a man-pages patch to document it in proc(5). Most users, including the one I have in mind, should just be able to run 'echo > /proc/sys/kernel/modprobe' early in the boot process. So I don't think the need for a kconfig option to control the default value has been clearly demonstrated yet. You're certainly welcome to send a patch for it if you believe it would be useful, though. > > So I don't see > > why it would be controversial? I already went through all callers of > > request_module() that check its return value, and they all appear to work better > > with -ENOENT, since they assume that 0 means the module was loaded. > > Thanks for doing that, but I note that getting 0 is not assurance > either. The de-facto best practive for the request_module() call is to > do your own in place verifier. > > > Incorrectly returning 0 typically causes unnecessary work (checking again > > whether the module's functionality is available) or misleading log messages. > > Yes but returning 0 cannot be relied upon today for assuming the module > is loaded. *If* we revisit that decision and want the kernel to do a > generic verifier, then yes, we can get rid of all the caller specific > verfifiers, but not today. > Sure, I understand all that; I think we're actually on the same page. Even if we make the return value of request_module() completely correct, nothing stops another process from loading or unloading the module immediately afterwards. However, callers do sometimes use the return value opportunisticly, like to log an appropriate message or to return early if module loading failed. Those seem like relatively appropriate uses. The thing which you really can't do, which I didn't see anyone doing, is use 'ret == 0' as a signal to go ahead and run code that will crash if the module has been unloaded already. - Eric ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 17:35 ` Eric Biggers @ 2020-03-11 18:00 ` Luis Chamberlain 2020-03-11 18:21 ` Eric Biggers 0 siblings, 1 reply; 15+ messages in thread From: Luis Chamberlain @ 2020-03-11 18:00 UTC (permalink / raw) To: Eric Biggers Cc: NeilBrown, Josh Triplett, linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook On Wed, Mar 11, 2020 at 10:35:45AM -0700, Eric Biggers wrote: > On Wed, Mar 11, 2020 at 06:31:30AM +0000, Luis Chamberlain wrote: > > On Tue, Mar 10, 2020 at 10:26:20PM -0700, Eric Biggers wrote: > > > On Wed, Mar 11, 2020 at 04:32:21AM +0000, Luis Chamberlain wrote: > > > > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > > > > > From: Eric Biggers <ebiggers@google.com> > > > > > > > > > > It's long been possible to disable kernel module autoloading completely > > > > > by setting /proc/sys/kernel/modprobe to the empty string. This can be > > > > > preferable > > > > > > > > preferable but ... not documented. Or was this documented or recommended > > > > somewhere? > > > > > > > > > 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. > > > > > > Not that I know of, though I didn't look too hard. proc(5) mentions > > > /proc/sys/kernel/modprobe but doesn't mention the empty string case. > > > > > > In any case, it's been supported for a long time, and it's useful for the > > > reasons I mentioned. > > > > Sure. I think then its important to document it as such then, or perhaps > > make a kconfig option which sets this to empty and document it on the > > kconfig entry. > > I'll send a man-pages patch to document it in proc(5). > > Most users, including the one I have in mind, should just be able to run > 'echo > /proc/sys/kernel/modprobe' early in the boot process. So I don't think > the need for a kconfig option to control the default value has been clearly > demonstrated yet. You're certainly welcome to send a patch for it if you > believe it would be useful, though. When doing a rewrite of some of this code I did wonder who would use this and clear it out. A kconfig entry would remove any doubt over its use and would allow one to skip the userspace / early init requirement to empty it out, therefore actually being safer because you are not racing against modules being loaded. Is avoiding the race more suitable for your needs than echo'ing early on boot? Luis ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 18:00 ` Luis Chamberlain @ 2020-03-11 18:21 ` Eric Biggers 2020-03-11 18:40 ` Luis Chamberlain 0 siblings, 1 reply; 15+ messages in thread From: Eric Biggers @ 2020-03-11 18:21 UTC (permalink / raw) To: Luis Chamberlain Cc: NeilBrown, Josh Triplett, linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook On Wed, Mar 11, 2020 at 06:00:02PM +0000, Luis Chamberlain wrote: > On Wed, Mar 11, 2020 at 10:35:45AM -0700, Eric Biggers wrote: > > On Wed, Mar 11, 2020 at 06:31:30AM +0000, Luis Chamberlain wrote: > > > On Tue, Mar 10, 2020 at 10:26:20PM -0700, Eric Biggers wrote: > > > > On Wed, Mar 11, 2020 at 04:32:21AM +0000, Luis Chamberlain wrote: > > > > > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > > > > > > From: Eric Biggers <ebiggers@google.com> > > > > > > > > > > > > It's long been possible to disable kernel module autoloading completely > > > > > > by setting /proc/sys/kernel/modprobe to the empty string. This can be > > > > > > preferable > > > > > > > > > > preferable but ... not documented. Or was this documented or recommended > > > > > somewhere? > > > > > > > > > > > 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. > > > > > > > > Not that I know of, though I didn't look too hard. proc(5) mentions > > > > /proc/sys/kernel/modprobe but doesn't mention the empty string case. > > > > > > > > In any case, it's been supported for a long time, and it's useful for the > > > > reasons I mentioned. > > > > > > Sure. I think then its important to document it as such then, or perhaps > > > make a kconfig option which sets this to empty and document it on the > > > kconfig entry. > > > > I'll send a man-pages patch to document it in proc(5). > > > > Most users, including the one I have in mind, should just be able to run > > 'echo > /proc/sys/kernel/modprobe' early in the boot process. So I don't think > > the need for a kconfig option to control the default value has been clearly > > demonstrated yet. You're certainly welcome to send a patch for it if you > > believe it would be useful, though. > > When doing a rewrite of some of this code I did wonder who would use > this and clear it out. A kconfig entry would remove any doubt over its > use and would allow one to skip the userspace / early init requirement > to empty it out, therefore actually being safer because you are not > racing against modules being loaded. > > Is avoiding the race more suitable for your needs than echo'ing early on boot? > Maybe. It would avoid the chance of races, but I haven't seen any yet. Also, our userspace has to support old kernels, so we still need the 'echo > /proc/sys/kernel/modprobe' anyway. If that turns out to be good enough, then it makes things easier for everyone. If setting the default at build time turns out to be needed, then sure in that case I'll send a patch that adds a kconfig option to do that. But I'm first trying to use the existing kernel functionality. Also, a kconfig option isn't really a substitute for documenting this existing sysctl. We still need to document it properly in proc(5) and Documentation/admin-guide/sysctl/kernel.rst. - Eric ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 18:21 ` Eric Biggers @ 2020-03-11 18:40 ` Luis Chamberlain 0 siblings, 0 replies; 15+ messages in thread From: Luis Chamberlain @ 2020-03-11 18:40 UTC (permalink / raw) To: Eric Biggers Cc: NeilBrown, Josh Triplett, linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook On Wed, Mar 11, 2020 at 11:21:30AM -0700, Eric Biggers wrote: > On Wed, Mar 11, 2020 at 06:00:02PM +0000, Luis Chamberlain wrote: > > On Wed, Mar 11, 2020 at 10:35:45AM -0700, Eric Biggers wrote: > > > On Wed, Mar 11, 2020 at 06:31:30AM +0000, Luis Chamberlain wrote: > > > > On Tue, Mar 10, 2020 at 10:26:20PM -0700, Eric Biggers wrote: > > > > > On Wed, Mar 11, 2020 at 04:32:21AM +0000, Luis Chamberlain wrote: > > > > > > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > > > > > > > From: Eric Biggers <ebiggers@google.com> > > > > > > > > > > > > > > It's long been possible to disable kernel module autoloading completely > > > > > > > by setting /proc/sys/kernel/modprobe to the empty string. This can be > > > > > > > preferable > > > > > > > > > > > > preferable but ... not documented. Or was this documented or recommended > > > > > > somewhere? > > > > > > > > > > > > > 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. > > > > > > > > > > Not that I know of, though I didn't look too hard. proc(5) mentions > > > > > /proc/sys/kernel/modprobe but doesn't mention the empty string case. > > > > > > > > > > In any case, it's been supported for a long time, and it's useful for the > > > > > reasons I mentioned. > > > > > > > > Sure. I think then its important to document it as such then, or perhaps > > > > make a kconfig option which sets this to empty and document it on the > > > > kconfig entry. > > > > > > I'll send a man-pages patch to document it in proc(5). > > > > > > Most users, including the one I have in mind, should just be able to run > > > 'echo > /proc/sys/kernel/modprobe' early in the boot process. So I don't think > > > the need for a kconfig option to control the default value has been clearly > > > demonstrated yet. You're certainly welcome to send a patch for it if you > > > believe it would be useful, though. > > > > When doing a rewrite of some of this code I did wonder who would use > > this and clear it out. A kconfig entry would remove any doubt over its > > use and would allow one to skip the userspace / early init requirement > > to empty it out, therefore actually being safer because you are not > > racing against modules being loaded. > > > > Is avoiding the race more suitable for your needs than echo'ing early on boot? > > > > Maybe. It would avoid the chance of races, but I haven't seen any yet. > Also, our userspace has to support old kernels, so we still need the > 'echo > /proc/sys/kernel/modprobe' anyway. If that turns out to be good enough, > then it makes things easier for everyone. > > If setting the default at build time turns out to be needed, then sure in that > case I'll send a patch that adds a kconfig option to do that. But I'm first > trying to use the existing kernel functionality. > > Also, a kconfig option isn't really a substitute for documenting this existing > sysctl. We still need to document it properly in proc(5) and > Documentation/admin-guide/sysctl/kernel.rst. Yes, sure. Whatever mechanism you find *suitable* I think you may the shiny best new user of it, and so given all that is shared now, I'm sure you will document do what is needed. Whatever guidance you can provide based on your experience is of huge value to this little corner of the kernel. I just wanted to do away with unclear tribal knowledge and ensure we support / test whatever you do well moving forward. Luis ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 4:32 ` Luis Chamberlain 2020-03-11 5:26 ` Eric Biggers @ 2020-03-11 5:55 ` Josh Triplett 2020-03-11 6:32 ` Luis Chamberlain 1 sibling, 1 reply; 15+ messages in thread From: Josh Triplett @ 2020-03-11 5:55 UTC (permalink / raw) To: Luis Chamberlain, Eric Biggers, NeilBrown Cc: linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook, benh On March 11, 2020 5:32:21 AM GMT+01:00, Luis Chamberlain <mcgrof@kernel.org> wrote: >On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: >> However, request_module() should also >> 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. > >This is a user experience change though, and I wouldn't have on my >radar >who would use this, and expects the old behaviour. Josh, would you by >chance? I don't think this affects userspace. But I'd suggest Ben Hutchings (CCed). ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 5:55 ` Josh Triplett @ 2020-03-11 6:32 ` Luis Chamberlain 0 siblings, 0 replies; 15+ messages in thread From: Luis Chamberlain @ 2020-03-11 6:32 UTC (permalink / raw) To: Josh Triplett Cc: Eric Biggers, NeilBrown, linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Kees Cook, benh On Wed, Mar 11, 2020 at 06:55:30AM +0100, Josh Triplett wrote: > On March 11, 2020 5:32:21 AM GMT+01:00, Luis Chamberlain <mcgrof@kernel.org> wrote: > >On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > >> However, request_module() should also > >> 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. > > > >This is a user experience change though, and I wouldn't have on my > >radar > >who would use this, and expects the old behaviour. Josh, would you by > >chance? > > I don't think this affects userspace. But I'd suggest Ben Hutchings (CCed). It doesn't, so yes no verififcation needed. Thanks the quick response though! Luis ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-10 22:37 [PATCH] kmod: make request_module() return an error when autoloading is disabled Eric Biggers 2020-03-11 4:32 ` Luis Chamberlain @ 2020-03-11 17:28 ` Kees Cook 2020-03-11 17:41 ` Eric Biggers 1 sibling, 1 reply; 15+ messages in thread From: Kees Cook @ 2020-03-11 17:28 UTC (permalink / raw) To: Eric Biggers Cc: linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Luis Chamberlain On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > It's long been possible to disable kernel module autoloading completely > by setting /proc/sys/kernel/modprobe to the empty string. This can be Hunh. I've never seen that before. :) I've always used; echo 1 > /proc/sys/kernel/modules_disabled Regardless, Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > 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 > [...] > > Arguably this warning is broken and should be removed, since the module > could have been unloaded already. However, request_module() should also > 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. > > 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> > 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 bc6addd9152b..a2de58de6ab6 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.481.gfbce0eb801-goog > -- Kees Cook ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 17:28 ` Kees Cook @ 2020-03-11 17:41 ` Eric Biggers 2020-03-11 17:50 ` Kees Cook 2020-03-11 18:01 ` Luis Chamberlain 0 siblings, 2 replies; 15+ messages in thread From: Eric Biggers @ 2020-03-11 17:41 UTC (permalink / raw) To: Kees Cook Cc: linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Luis Chamberlain On Wed, Mar 11, 2020 at 10:28:07AM -0700, Kees Cook wrote: > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > It's long been possible to disable kernel module autoloading completely > > by setting /proc/sys/kernel/modprobe to the empty string. This can be > > Hunh. I've never seen that before. :) I've always used; > > echo 1 > /proc/sys/kernel/modules_disabled > > Regardless, > > Reviewed-by: Kees Cook <keescook@chromium.org> > modules_disabled is different because it disables *all* module loading, not just autoloading. - Eric ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 17:41 ` Eric Biggers @ 2020-03-11 17:50 ` Kees Cook 2020-03-11 18:01 ` Luis Chamberlain 1 sibling, 0 replies; 15+ messages in thread From: Kees Cook @ 2020-03-11 17:50 UTC (permalink / raw) To: Eric Biggers Cc: linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu, Luis Chamberlain On Wed, Mar 11, 2020 at 10:41:34AM -0700, Eric Biggers wrote: > On Wed, Mar 11, 2020 at 10:28:07AM -0700, Kees Cook wrote: > > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > It's long been possible to disable kernel module autoloading completely > > > by setting /proc/sys/kernel/modprobe to the empty string. This can be > > > > Hunh. I've never seen that before. :) I've always used; > > > > echo 1 > /proc/sys/kernel/modules_disabled > > > > Regardless, > > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > modules_disabled is different because it disables *all* module loading, not just > autoloading. Yes, quite true. Some day I'd love to revisit this series to improve autoloading sanity checking: https://github.com/KSPP/linux/issues/24 -- Kees Cook ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 17:41 ` Eric Biggers 2020-03-11 17:50 ` Kees Cook @ 2020-03-11 18:01 ` Luis Chamberlain 2020-03-11 18:08 ` Eric Biggers 1 sibling, 1 reply; 15+ messages in thread From: Luis Chamberlain @ 2020-03-11 18:01 UTC (permalink / raw) To: Eric Biggers Cc: Kees Cook, linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu On Wed, Mar 11, 2020 at 10:41:34AM -0700, Eric Biggers wrote: > On Wed, Mar 11, 2020 at 10:28:07AM -0700, Kees Cook wrote: > > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > It's long been possible to disable kernel module autoloading completely > > > by setting /proc/sys/kernel/modprobe to the empty string. This can be > > > > Hunh. I've never seen that before. :) I've always used; > > > > echo 1 > /proc/sys/kernel/modules_disabled > > > > Regardless, > > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > modules_disabled is different because it disables *all* module loading, not just > autoloading. Clarifying this on your patch would be useful, otherwise its lost tribal knowledge. LUis ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled 2020-03-11 18:01 ` Luis Chamberlain @ 2020-03-11 18:08 ` Eric Biggers 0 siblings, 0 replies; 15+ messages in thread From: Eric Biggers @ 2020-03-11 18:08 UTC (permalink / raw) To: Luis Chamberlain Cc: Kees Cook, linux-kernel, linux-fsdevel, stable, Alexei Starovoitov, Andrew Morton, Greg Kroah-Hartman, Jeff Vander Stoep, Jessica Yu On Wed, Mar 11, 2020 at 06:01:07PM +0000, Luis Chamberlain wrote: > On Wed, Mar 11, 2020 at 10:41:34AM -0700, Eric Biggers wrote: > > On Wed, Mar 11, 2020 at 10:28:07AM -0700, Kees Cook wrote: > > > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote: > > > > From: Eric Biggers <ebiggers@google.com> > > > > > > > > It's long been possible to disable kernel module autoloading completely > > > > by setting /proc/sys/kernel/modprobe to the empty string. This can be > > > > > > Hunh. I've never seen that before. :) I've always used; > > > > > > echo 1 > /proc/sys/kernel/modules_disabled > > > > > > Regardless, > > > > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > > > > modules_disabled is different because it disables *all* module loading, not just > > autoloading. > > Clarifying this on your patch would be useful, otherwise its lost > tribal knowledge. I think it would be more useful to improve the documentation in proc(5) and Documentation/admin-guide/sysctl/kernel.rst. People shouldn't have to read random kernel commit messages to find the documentation. I'll send out patches for those. - Eric ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-03-11 18:40 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-10 22:37 [PATCH] kmod: make request_module() return an error when autoloading is disabled Eric Biggers 2020-03-11 4:32 ` Luis Chamberlain 2020-03-11 5:26 ` Eric Biggers 2020-03-11 6:31 ` Luis Chamberlain 2020-03-11 17:35 ` Eric Biggers 2020-03-11 18:00 ` Luis Chamberlain 2020-03-11 18:21 ` Eric Biggers 2020-03-11 18:40 ` Luis Chamberlain 2020-03-11 5:55 ` Josh Triplett 2020-03-11 6:32 ` Luis Chamberlain 2020-03-11 17:28 ` Kees Cook 2020-03-11 17:41 ` Eric Biggers 2020-03-11 17:50 ` Kees Cook 2020-03-11 18:01 ` Luis Chamberlain 2020-03-11 18:08 ` Eric Biggers
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).