From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932159AbdK1TOL (ORCPT ); Tue, 28 Nov 2017 14:14:11 -0500 Received: from mx2.suse.de ([195.135.220.15]:54703 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752341AbdK1TOJ (ORCPT ); Tue, 28 Nov 2017 14:14:09 -0500 Date: Tue, 28 Nov 2017 20:14:05 +0100 From: "Luis R. Rodriguez" To: Djalal Harouni Cc: Kees Cook , Andy Lutomirski , Andrew Morton , "Luis R. Rodriguez" , James Morris , Ben Hutchings , Solar Designer , Serge Hallyn , Jessica Yu , Rusty Russell , linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, kernel-hardening@lists.openwall.com, Jonathan Corbet , Ingo Molnar , "David S. Miller" , netdev@vger.kernel.org, Peter Zijlstra , Linus Torvalds Subject: Re: [PATCH v5 next 1/5] modules:capabilities: add request_module_cap() Message-ID: <20171128191405.GO729@wotan.suse.de> References: <1511803118-2552-1-git-send-email-tixxdz@gmail.com> <1511803118-2552-2-git-send-email-tixxdz@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1511803118-2552-2-git-send-email-tixxdz@gmail.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 27, 2017 at 06:18:34PM +0100, Djalal Harouni wrote: ... > After a discussion with Rusty Russell [1], the suggestion was to pass > the capability from request_module() to security_kernel_module_request() > for 'netdev-%s' modules that need CAP_NET_ADMIN, and after review from > Kees Cook [2] and experimenting with the code, the patch now does the > following: > > * Adds the request_module_cap() function. > * Updates the __request_module() to take the "required_cap" argument > with the "prefix". ... > Signed-off-by: Djalal Harouni > --- > diff --git a/kernel/kmod.c b/kernel/kmod.c > index bc6addd..679d401 100644 > --- a/kernel/kmod.c > +++ b/kernel/kmod.c > @@ -139,13 +147,22 @@ int __request_module(bool wait, const char *fmt, ...) > if (!modprobe_path[0]) > return 0; > > + /* > + * Lets attach the prefix to the module name > + */ > + if (prefix != NULL && *prefix != '\0') { > + len += snprintf(module_name, MODULE_NAME_LEN, "%s-", prefix); > + if (len >= MODULE_NAME_LEN) > + return -ENAMETOOLONG; > + } > + > va_start(args, fmt); > - ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args); > + ret = vsnprintf(module_name + len, MODULE_NAME_LEN - len, fmt, args); > va_end(args); > - if (ret >= MODULE_NAME_LEN) > + if (ret >= MODULE_NAME_LEN - len) > return -ENAMETOOLONG; > > - ret = security_kernel_module_request(module_name); > + ret = security_kernel_module_request(module_name, required_cap, prefix); > if (ret) > return ret; > kmod is just a helper to poke userpsace to load a module, that's it. The old init_module() and newer finit_module() do the real handy work or module loading, and both currently only use may_init_module(): static int may_init_module(void) { if (!capable(CAP_SYS_MODULE) || modules_disabled) return -EPERM; return 0; } This begs the question: o If userspace just tries to just use raw finit_module() do we want similar checks? Otherwise, correct me if I'm wrong this all seems pointless. If we want something similar I think we might need to be processing aliases and check for the aliases for their desired restrictions on finit_module(), otherwise userspace can skip through the checks if the module name does not match the alias prefix. To be clear, aliases are completely ignored today on load_module(), so loading 'xfs' with finit_module() will just have the kernel know about 'xfs' not 'fs-xfs'. So we currently do not process aliases in kernel. I have debugging patches to enable us to process them, but they are just for debugging and I've been meaning to send them in for review. I designed them only for debugging given last time someone suggested for aliases processing to be added, the only use case we found was a pre-optimizations we decided to avoid pursuing. Debugging is a good reason to have alias processing in-kernel though. The pre-optimization we decided to stay away from was to check if the requested module via request_module() was already loaded *and* also check if the name passed matches any of the existing module aliases for currently loaded modules. Today request_module() does not even check if a requested module is already loaded, its a stupid loader, it just goes to userspace, and lets userspace figure it out. Userspace in turn could check for aliases, but it could lie, or not be up to date to do that. The pre-optmization is a theoretical gain only then, and if userspace had proper alias checking it is arguable that it may perform just as equal. To help valuate these sorts of things we now have: tools/testing/selftests/kmod/kmod.sh So further patches can use and test impact with it. Anyway -- so aliasing is currently only a debugging consideration, but without processing aliases, all this work seems pointless to me as the real loader is in finit_module(). kmod is just a stupid loader and uses the kernel usermode helper to do work. Luis From mboxrd@z Thu Jan 1 00:00:00 1970 From: mcgrof@kernel.org (Luis R. Rodriguez) Date: Tue, 28 Nov 2017 20:14:05 +0100 Subject: [PATCH v5 next 1/5] modules:capabilities: add request_module_cap() In-Reply-To: <1511803118-2552-2-git-send-email-tixxdz@gmail.com> References: <1511803118-2552-1-git-send-email-tixxdz@gmail.com> <1511803118-2552-2-git-send-email-tixxdz@gmail.com> Message-ID: <20171128191405.GO729@wotan.suse.de> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Mon, Nov 27, 2017 at 06:18:34PM +0100, Djalal Harouni wrote: ... > After a discussion with Rusty Russell [1], the suggestion was to pass > the capability from request_module() to security_kernel_module_request() > for 'netdev-%s' modules that need CAP_NET_ADMIN, and after review from > Kees Cook [2] and experimenting with the code, the patch now does the > following: > > * Adds the request_module_cap() function. > * Updates the __request_module() to take the "required_cap" argument > with the "prefix". ... > Signed-off-by: Djalal Harouni > --- > diff --git a/kernel/kmod.c b/kernel/kmod.c > index bc6addd..679d401 100644 > --- a/kernel/kmod.c > +++ b/kernel/kmod.c > @@ -139,13 +147,22 @@ int __request_module(bool wait, const char *fmt, ...) > if (!modprobe_path[0]) > return 0; > > + /* > + * Lets attach the prefix to the module name > + */ > + if (prefix != NULL && *prefix != '\0') { > + len += snprintf(module_name, MODULE_NAME_LEN, "%s-", prefix); > + if (len >= MODULE_NAME_LEN) > + return -ENAMETOOLONG; > + } > + > va_start(args, fmt); > - ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args); > + ret = vsnprintf(module_name + len, MODULE_NAME_LEN - len, fmt, args); > va_end(args); > - if (ret >= MODULE_NAME_LEN) > + if (ret >= MODULE_NAME_LEN - len) > return -ENAMETOOLONG; > > - ret = security_kernel_module_request(module_name); > + ret = security_kernel_module_request(module_name, required_cap, prefix); > if (ret) > return ret; > kmod is just a helper to poke userpsace to load a module, that's it. The old init_module() and newer finit_module() do the real handy work or module loading, and both currently only use may_init_module(): static int may_init_module(void) { if (!capable(CAP_SYS_MODULE) || modules_disabled) return -EPERM; return 0; } This begs the question: o If userspace just tries to just use raw finit_module() do we want similar checks? Otherwise, correct me if I'm wrong this all seems pointless. If we want something similar I think we might need to be processing aliases and check for the aliases for their desired restrictions on finit_module(), otherwise userspace can skip through the checks if the module name does not match the alias prefix. To be clear, aliases are completely ignored today on load_module(), so loading 'xfs' with finit_module() will just have the kernel know about 'xfs' not 'fs-xfs'. So we currently do not process aliases in kernel. I have debugging patches to enable us to process them, but they are just for debugging and I've been meaning to send them in for review. I designed them only for debugging given last time someone suggested for aliases processing to be added, the only use case we found was a pre-optimizations we decided to avoid pursuing. Debugging is a good reason to have alias processing in-kernel though. The pre-optimization we decided to stay away from was to check if the requested module via request_module() was already loaded *and* also check if the name passed matches any of the existing module aliases for currently loaded modules. Today request_module() does not even check if a requested module is already loaded, its a stupid loader, it just goes to userspace, and lets userspace figure it out. Userspace in turn could check for aliases, but it could lie, or not be up to date to do that. The pre-optmization is a theoretical gain only then, and if userspace had proper alias checking it is arguable that it may perform just as equal. To help valuate these sorts of things we now have: tools/testing/selftests/kmod/kmod.sh So further patches can use and test impact with it. Anyway -- so aliasing is currently only a debugging consideration, but without processing aliases, all this work seems pointless to me as the real loader is in finit_module(). kmod is just a stupid loader and uses the kernel usermode helper to do work. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Tue, 28 Nov 2017 20:14:05 +0100 From: "Luis R. Rodriguez" Message-ID: <20171128191405.GO729@wotan.suse.de> References: <1511803118-2552-1-git-send-email-tixxdz@gmail.com> <1511803118-2552-2-git-send-email-tixxdz@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1511803118-2552-2-git-send-email-tixxdz@gmail.com> Subject: [kernel-hardening] Re: [PATCH v5 next 1/5] modules:capabilities: add request_module_cap() To: Djalal Harouni Cc: Kees Cook , Andy Lutomirski , Andrew Morton , "Luis R. Rodriguez" , James Morris , Ben Hutchings , Solar Designer , Serge Hallyn , Jessica Yu , Rusty Russell , linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, kernel-hardening@lists.openwall.com, Jonathan Corbet , Ingo Molnar , "David S. Miller" , netdev@vger.kernel.org, Peter Zijlstra , Linus Torvalds List-ID: On Mon, Nov 27, 2017 at 06:18:34PM +0100, Djalal Harouni wrote: ... > After a discussion with Rusty Russell [1], the suggestion was to pass > the capability from request_module() to security_kernel_module_request() > for 'netdev-%s' modules that need CAP_NET_ADMIN, and after review from > Kees Cook [2] and experimenting with the code, the patch now does the > following: > > * Adds the request_module_cap() function. > * Updates the __request_module() to take the "required_cap" argument > with the "prefix". ... > Signed-off-by: Djalal Harouni > --- > diff --git a/kernel/kmod.c b/kernel/kmod.c > index bc6addd..679d401 100644 > --- a/kernel/kmod.c > +++ b/kernel/kmod.c > @@ -139,13 +147,22 @@ int __request_module(bool wait, const char *fmt, ...) > if (!modprobe_path[0]) > return 0; > > + /* > + * Lets attach the prefix to the module name > + */ > + if (prefix != NULL && *prefix != '\0') { > + len += snprintf(module_name, MODULE_NAME_LEN, "%s-", prefix); > + if (len >= MODULE_NAME_LEN) > + return -ENAMETOOLONG; > + } > + > va_start(args, fmt); > - ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args); > + ret = vsnprintf(module_name + len, MODULE_NAME_LEN - len, fmt, args); > va_end(args); > - if (ret >= MODULE_NAME_LEN) > + if (ret >= MODULE_NAME_LEN - len) > return -ENAMETOOLONG; > > - ret = security_kernel_module_request(module_name); > + ret = security_kernel_module_request(module_name, required_cap, prefix); > if (ret) > return ret; > kmod is just a helper to poke userpsace to load a module, that's it. The old init_module() and newer finit_module() do the real handy work or module loading, and both currently only use may_init_module(): static int may_init_module(void) { if (!capable(CAP_SYS_MODULE) || modules_disabled) return -EPERM; return 0; } This begs the question: o If userspace just tries to just use raw finit_module() do we want similar checks? Otherwise, correct me if I'm wrong this all seems pointless. If we want something similar I think we might need to be processing aliases and check for the aliases for their desired restrictions on finit_module(), otherwise userspace can skip through the checks if the module name does not match the alias prefix. To be clear, aliases are completely ignored today on load_module(), so loading 'xfs' with finit_module() will just have the kernel know about 'xfs' not 'fs-xfs'. So we currently do not process aliases in kernel. I have debugging patches to enable us to process them, but they are just for debugging and I've been meaning to send them in for review. I designed them only for debugging given last time someone suggested for aliases processing to be added, the only use case we found was a pre-optimizations we decided to avoid pursuing. Debugging is a good reason to have alias processing in-kernel though. The pre-optimization we decided to stay away from was to check if the requested module via request_module() was already loaded *and* also check if the name passed matches any of the existing module aliases for currently loaded modules. Today request_module() does not even check if a requested module is already loaded, its a stupid loader, it just goes to userspace, and lets userspace figure it out. Userspace in turn could check for aliases, but it could lie, or not be up to date to do that. The pre-optmization is a theoretical gain only then, and if userspace had proper alias checking it is arguable that it may perform just as equal. To help valuate these sorts of things we now have: tools/testing/selftests/kmod/kmod.sh So further patches can use and test impact with it. Anyway -- so aliasing is currently only a debugging consideration, but without processing aliases, all this work seems pointless to me as the real loader is in finit_module(). kmod is just a stupid loader and uses the kernel usermode helper to do work. Luis