| Message ID | 157678334821.158235.2125894638773393579.stgit@chester (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] selinux: deprecate disabling SELinux and runtime | expand |
On 12/19/2019 11:22 AM, Paul Moore wrote: > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The > code was originally developed to make it easier for Linux > distributions to support architectures where adding parameters to the > kernel command line was difficult. Unfortunately, supporting runtime > disable meant we had to make some security trade-offs when it came to > the LSM hooks, as documented in the Kconfig help text: > > NOTE: selecting this option will disable the '__ro_after_init' > kernel hardening feature for security hooks. Please consider > using the selinux=0 boot parameter instead of enabling this > option. > > Fortunately it looks as if that the original motivation for the > runtime disable functionality is gone, and Fedora/RHEL appears to be > the only major distribution enabling this capability at build time > so we are now taking steps to remove it entirely from the kernel. > The first step is to mark the functionality as deprecated and print > an error when it is used (what this patch is doing). As Fedora/RHEL > makes progress in transitioning the distribution away from runtime > disable, we will introduce follow-up patches over several kernel > releases which will block for increasing periods of time when the > runtime disable is used. Finally we will remove the option entirely > once we believe all users have moved to the kernel cmdline approach. > > Signed-off-by: Paul Moore <paul@paul-moore.com> Acked-by: Casey Schaufler <casey@schaufler-ca.com> > --- > security/selinux/Kconfig | 3 +++ > security/selinux/selinuxfs.c | 6 ++++++ > 2 files changed, 9 insertions(+) > > diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig > index 996d35d950f7..580ac24c7aa1 100644 > --- a/security/selinux/Kconfig > +++ b/security/selinux/Kconfig > @@ -42,6 +42,9 @@ config SECURITY_SELINUX_DISABLE > using the selinux=0 boot parameter instead of enabling this > option. > > + WARNING: this option is deprecated and will be removed in a future > + kernel release. > + > If you are unsure how to answer this question, answer N. > > config SECURITY_SELINUX_DEVELOP > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > index 278417e67b4c..adbe2dd35202 100644 > --- a/security/selinux/selinuxfs.c > +++ b/security/selinux/selinuxfs.c > @@ -281,6 +281,12 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf, > int new_value; > int enforcing; > > + /* NOTE: we are now officially considering runtime disable as > + * deprecated, and using it will become increasingly painful > + * (e.g. sleeping/blocking) as we progress through future > + * kernel releases until eventually it is removed */ > + pr_err("SELinux: Runtime disable is deprecated, use selinux=0 on the kernel cmdline.\n"); > + > if (count >= PAGE_SIZE) > return -ENOMEM; > > >
On Thu, Dec 19, 2019 at 8:22 PM Paul Moore <paul@paul-moore.com> wrote: > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The > code was originally developed to make it easier for Linux > distributions to support architectures where adding parameters to the > kernel command line was difficult. Unfortunately, supporting runtime > disable meant we had to make some security trade-offs when it came to > the LSM hooks, as documented in the Kconfig help text: > > NOTE: selecting this option will disable the '__ro_after_init' > kernel hardening feature for security hooks. Please consider > using the selinux=0 boot parameter instead of enabling this > option. > > Fortunately it looks as if that the original motivation for the > runtime disable functionality is gone, and Fedora/RHEL appears to be > the only major distribution enabling this capability at build time > so we are now taking steps to remove it entirely from the kernel. > The first step is to mark the functionality as deprecated and print > an error when it is used (what this patch is doing). As Fedora/RHEL > makes progress in transitioning the distribution away from runtime > disable, we will introduce follow-up patches over several kernel > releases which will block for increasing periods of time when the > runtime disable is used. Finally we will remove the option entirely > once we believe all users have moved to the kernel cmdline approach. > > Signed-off-by: Paul Moore <paul@paul-moore.com> Looks reasonable, informal ACK from me. > --- > security/selinux/Kconfig | 3 +++ > security/selinux/selinuxfs.c | 6 ++++++ > 2 files changed, 9 insertions(+) > > diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig > index 996d35d950f7..580ac24c7aa1 100644 > --- a/security/selinux/Kconfig > +++ b/security/selinux/Kconfig > @@ -42,6 +42,9 @@ config SECURITY_SELINUX_DISABLE > using the selinux=0 boot parameter instead of enabling this > option. > > + WARNING: this option is deprecated and will be removed in a future > + kernel release. > + > If you are unsure how to answer this question, answer N. > > config SECURITY_SELINUX_DEVELOP > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > index 278417e67b4c..adbe2dd35202 100644 > --- a/security/selinux/selinuxfs.c > +++ b/security/selinux/selinuxfs.c > @@ -281,6 +281,12 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf, > int new_value; > int enforcing; > > + /* NOTE: we are now officially considering runtime disable as > + * deprecated, and using it will become increasingly painful > + * (e.g. sleeping/blocking) as we progress through future > + * kernel releases until eventually it is removed */ > + pr_err("SELinux: Runtime disable is deprecated, use selinux=0 on the kernel cmdline.\n"); > + > if (count >= PAGE_SIZE) > return -ENOMEM; > >
On Thu, Jan 2, 2020 at 4:24 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Thu, Dec 19, 2019 at 8:22 PM Paul Moore <paul@paul-moore.com> wrote: > > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The > > code was originally developed to make it easier for Linux > > distributions to support architectures where adding parameters to the > > kernel command line was difficult. Unfortunately, supporting runtime > > disable meant we had to make some security trade-offs when it came to > > the LSM hooks, as documented in the Kconfig help text: > > > > NOTE: selecting this option will disable the '__ro_after_init' > > kernel hardening feature for security hooks. Please consider > > using the selinux=0 boot parameter instead of enabling this > > option. > > > > Fortunately it looks as if that the original motivation for the > > runtime disable functionality is gone, and Fedora/RHEL appears to be > > the only major distribution enabling this capability at build time > > so we are now taking steps to remove it entirely from the kernel. > > The first step is to mark the functionality as deprecated and print > > an error when it is used (what this patch is doing). As Fedora/RHEL > > makes progress in transitioning the distribution away from runtime > > disable, we will introduce follow-up patches over several kernel > > releases which will block for increasing periods of time when the > > runtime disable is used. Finally we will remove the option entirely > > once we believe all users have moved to the kernel cmdline approach. > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > Looks reasonable, informal ACK from me. Thanks. You want to make that a formal ACK? ;)
On Thu, Jan 2, 2020 at 10:38 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Jan 2, 2020 at 4:24 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Dec 19, 2019 at 8:22 PM Paul Moore <paul@paul-moore.com> wrote: > > > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The > > > code was originally developed to make it easier for Linux > > > distributions to support architectures where adding parameters to the > > > kernel command line was difficult. Unfortunately, supporting runtime > > > disable meant we had to make some security trade-offs when it came to > > > the LSM hooks, as documented in the Kconfig help text: > > > > > > NOTE: selecting this option will disable the '__ro_after_init' > > > kernel hardening feature for security hooks. Please consider > > > using the selinux=0 boot parameter instead of enabling this > > > option. > > > > > > Fortunately it looks as if that the original motivation for the > > > runtime disable functionality is gone, and Fedora/RHEL appears to be > > > the only major distribution enabling this capability at build time > > > so we are now taking steps to remove it entirely from the kernel. > > > The first step is to mark the functionality as deprecated and print > > > an error when it is used (what this patch is doing). As Fedora/RHEL > > > makes progress in transitioning the distribution away from runtime > > > disable, we will introduce follow-up patches over several kernel > > > releases which will block for increasing periods of time when the > > > runtime disable is used. Finally we will remove the option entirely > > > once we believe all users have moved to the kernel cmdline approach. > > > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > Looks reasonable, informal ACK from me. > > Thanks. You want to make that a formal ACK? ;) Sure, if you find it useful :) Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
On Fri, Jan 3, 2020 at 4:32 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Thu, Jan 2, 2020 at 10:38 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Jan 2, 2020 at 4:24 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > On Thu, Dec 19, 2019 at 8:22 PM Paul Moore <paul@paul-moore.com> wrote: > > > > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality ... ... > > > Looks reasonable, informal ACK from me. > > > > Thanks. You want to make that a formal ACK? ;) > > Sure, if you find it useful :) > > Acked-by: Ondrej Mosnacek <omosnace@redhat.com> Yes, it is useful, thank you. For this patch your ACK is particularly significant because you are representing RH here (I'm assuming you are still the RH SELinux kernel person) and we are deprecating a feature used by Fedora. In my opinion it would be a mistake to merge a deprecation patch without the ACKs of those who rely on the feature targeted for removal (although in some cases it may need to be done regardless). I also really dislike merging my own patches without at least one other Acked-by/Reviewed-by tag for the simple reason that I believe every patch should have at least two people (author and at least one reviewer) who agree that the patch is reasonable. Of course there are exceptions for trivial and critical fixes, e.g. 15b590a81fcd ("selinux: ensure the policy has been loaded before reading the sidtab stats"), but I like to keep those as the exception rather than the rule. Just because someone is listed in the MAINTAINERS file shouldn't mean they are exempt from the normal review process. Generally speaking, one of the more useful things one can do from an upstream perspective is to review and test patches that are submitted to the list. We are a community driven project after all, and the community aspect shouldn't be limited to just the development of patches ;)
On 12/19/19 2:22 PM, Paul Moore wrote: > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The > code was originally developed to make it easier for Linux > distributions to support architectures where adding parameters to the > kernel command line was difficult. Unfortunately, supporting runtime > disable meant we had to make some security trade-offs when it came to > the LSM hooks, as documented in the Kconfig help text: > > NOTE: selecting this option will disable the '__ro_after_init' > kernel hardening feature for security hooks. Please consider > using the selinux=0 boot parameter instead of enabling this > option. > > Fortunately it looks as if that the original motivation for the > runtime disable functionality is gone, and Fedora/RHEL appears to be > the only major distribution enabling this capability at build time > so we are now taking steps to remove it entirely from the kernel. > The first step is to mark the functionality as deprecated and print > an error when it is used (what this patch is doing). As Fedora/RHEL > makes progress in transitioning the distribution away from runtime > disable, we will introduce follow-up patches over several kernel > releases which will block for increasing periods of time when the > runtime disable is used. Finally we will remove the option entirely > once we believe all users have moved to the kernel cmdline approach. > > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/Kconfig | 3 +++ > security/selinux/selinuxfs.c | 6 ++++++ > 2 files changed, 9 insertions(+) > > diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig > index 996d35d950f7..580ac24c7aa1 100644 > --- a/security/selinux/Kconfig > +++ b/security/selinux/Kconfig > @@ -42,6 +42,9 @@ config SECURITY_SELINUX_DISABLE > using the selinux=0 boot parameter instead of enabling this > option. > > + WARNING: this option is deprecated and will be removed in a future > + kernel release. > + > If you are unsure how to answer this question, answer N. > > config SECURITY_SELINUX_DEVELOP > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > index 278417e67b4c..adbe2dd35202 100644 > --- a/security/selinux/selinuxfs.c > +++ b/security/selinux/selinuxfs.c > @@ -281,6 +281,12 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf, > int new_value; > int enforcing; > > + /* NOTE: we are now officially considering runtime disable as > + * deprecated, and using it will become increasingly painful > + * (e.g. sleeping/blocking) as we progress through future > + * kernel releases until eventually it is removed */ > + pr_err("SELinux: Runtime disable is deprecated, use selinux=0 on the kernel cmdline.\n"); Looking for examples of similar deprecations in the kernel, I notice that they often use pr_warn_once() or WARN_ONCE() to avoid spamming the kernel logs excessively. They also often include the current process name to identify the offending process. In this case, it probably matters little since this is only done (legitimately) by the init process and only once, so up to you whether you bother amending it. Also for some interfaces, they appear to document the intent to remove it in a file under Documentation/ABI/obsolete/ and then later move that to removed/ when fully removed. Regardless, Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > + > if (count >= PAGE_SIZE) > return -ENOMEM; > >
On Mon, Jan 6, 2020 at 4:25 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 12/19/19 2:22 PM, Paul Moore wrote: > > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The > > code was originally developed to make it easier for Linux > > distributions to support architectures where adding parameters to the > > kernel command line was difficult. Unfortunately, supporting runtime > > disable meant we had to make some security trade-offs when it came to > > the LSM hooks, as documented in the Kconfig help text: > > > > NOTE: selecting this option will disable the '__ro_after_init' > > kernel hardening feature for security hooks. Please consider > > using the selinux=0 boot parameter instead of enabling this > > option. > > > > Fortunately it looks as if that the original motivation for the > > runtime disable functionality is gone, and Fedora/RHEL appears to be > > the only major distribution enabling this capability at build time > > so we are now taking steps to remove it entirely from the kernel. > > The first step is to mark the functionality as deprecated and print > > an error when it is used (what this patch is doing). As Fedora/RHEL > > makes progress in transitioning the distribution away from runtime > > disable, we will introduce follow-up patches over several kernel > > releases which will block for increasing periods of time when the > > runtime disable is used. Finally we will remove the option entirely > > once we believe all users have moved to the kernel cmdline approach. > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > security/selinux/Kconfig | 3 +++ > > security/selinux/selinuxfs.c | 6 ++++++ > > 2 files changed, 9 insertions(+) > > > > diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig > > index 996d35d950f7..580ac24c7aa1 100644 > > --- a/security/selinux/Kconfig > > +++ b/security/selinux/Kconfig > > @@ -42,6 +42,9 @@ config SECURITY_SELINUX_DISABLE > > using the selinux=0 boot parameter instead of enabling this > > option. > > > > + WARNING: this option is deprecated and will be removed in a future > > + kernel release. > > + > > If you are unsure how to answer this question, answer N. > > > > config SECURITY_SELINUX_DEVELOP > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > > index 278417e67b4c..adbe2dd35202 100644 > > --- a/security/selinux/selinuxfs.c > > +++ b/security/selinux/selinuxfs.c > > @@ -281,6 +281,12 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf, > > int new_value; > > int enforcing; > > > > + /* NOTE: we are now officially considering runtime disable as > > + * deprecated, and using it will become increasingly painful > > + * (e.g. sleeping/blocking) as we progress through future > > + * kernel releases until eventually it is removed */ > > + pr_err("SELinux: Runtime disable is deprecated, use selinux=0 on the kernel cmdline.\n"); > > Looking for examples of similar deprecations in the kernel, I notice > that they often use pr_warn_once() or WARN_ONCE() to avoid spamming the > kernel logs excessively. They also often include the current process > name to identify the offending process. In this case, it probably > matters little since this is only done (legitimately) by the init > process and only once, so up to you whether you bother amending it. Yes, I saw that too and decided we were better off printing something each time since it really should only ever happen once on a well behaved system. > Also for some interfaces, they appear to document the intent to remove > it in a file under Documentation/ABI/obsolete/ and then later move that > to removed/ when fully removed. Thanks, I wasn't aware of that, and couldn't find anything relevant while grep'ing under Documentation/process. There used to be a Documentation/feature-removal.txt (or a file with a similar name) which tracked these things, but I guess it migrated over to Documentation/ABI during the last Documentation shuffle a couple of years ago. I'll send out an updated patch in just a moment.
diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig index 996d35d950f7..580ac24c7aa1 100644 --- a/security/selinux/Kconfig +++ b/security/selinux/Kconfig @@ -42,6 +42,9 @@ config SECURITY_SELINUX_DISABLE using the selinux=0 boot parameter instead of enabling this option. + WARNING: this option is deprecated and will be removed in a future + kernel release. + If you are unsure how to answer this question, answer N. config SECURITY_SELINUX_DEVELOP diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 278417e67b4c..adbe2dd35202 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -281,6 +281,12 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf, int new_value; int enforcing; + /* NOTE: we are now officially considering runtime disable as + * deprecated, and using it will become increasingly painful + * (e.g. sleeping/blocking) as we progress through future + * kernel releases until eventually it is removed */ + pr_err("SELinux: Runtime disable is deprecated, use selinux=0 on the kernel cmdline.\n"); + if (count >= PAGE_SIZE) return -ENOMEM;
Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The code was originally developed to make it easier for Linux distributions to support architectures where adding parameters to the kernel command line was difficult. Unfortunately, supporting runtime disable meant we had to make some security trade-offs when it came to the LSM hooks, as documented in the Kconfig help text: NOTE: selecting this option will disable the '__ro_after_init' kernel hardening feature for security hooks. Please consider using the selinux=0 boot parameter instead of enabling this option. Fortunately it looks as if that the original motivation for the runtime disable functionality is gone, and Fedora/RHEL appears to be the only major distribution enabling this capability at build time so we are now taking steps to remove it entirely from the kernel. The first step is to mark the functionality as deprecated and print an error when it is used (what this patch is doing). As Fedora/RHEL makes progress in transitioning the distribution away from runtime disable, we will introduce follow-up patches over several kernel releases which will block for increasing periods of time when the runtime disable is used. Finally we will remove the option entirely once we believe all users have moved to the kernel cmdline approach. Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/selinux/Kconfig | 3 +++ security/selinux/selinuxfs.c | 6 ++++++ 2 files changed, 9 insertions(+)