| Message ID | alpine.LRH.2.20.1702131632340.8914@namei.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
James Morris wrote: > Both SELinux and Smack register Netfilter operations during init, > which then don't change. Mark these ops as __ro_after_init. > > Signed-off-by: James Morris <james.l.morris@oracle.com> This patch breaks CONFIG_SECURITY_SELINUX_DISABLE=y + SELINUX=disabled in /etc/selinux/config case, doesn't it? Although I heard that SELinux is planning to remove CONFIG_SECURITY_SELINUX_DISABLE, CONFIG_SECURITY_SELINUX_DISABLE is valid as of current linux-security.git#next .
On Mon, Feb 13, 2017 at 3:29 AM, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > James Morris wrote: >> Both SELinux and Smack register Netfilter operations during init, >> which then don't change. Mark these ops as __ro_after_init. >> >> Signed-off-by: James Morris <james.l.morris@oracle.com> > > This patch breaks CONFIG_SECURITY_SELINUX_DISABLE=y + SELINUX=disabled in /etc/selinux/config case, > doesn't it? Although I heard that SELinux is planning to remove CONFIG_SECURITY_SELINUX_DISABLE, > CONFIG_SECURITY_SELINUX_DISABLE is valid as of current linux-security.git#next . We could fold that removal into this series? -Kees
On Mon, 2017-02-13 at 09:29 -0800, Kees Cook wrote: > On Mon, Feb 13, 2017 at 3:29 AM, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > > > James Morris wrote: > > > > > > Both SELinux and Smack register Netfilter operations during init, > > > which then don't change. Mark these ops as __ro_after_init. > > > > > > Signed-off-by: James Morris <james.l.morris@oracle.com> > > > > This patch breaks CONFIG_SECURITY_SELINUX_DISABLE=y + > > SELINUX=disabled in /etc/selinux/config case, > > doesn't it? Although I heard that SELinux is planning to remove > > CONFIG_SECURITY_SELINUX_DISABLE, > > CONFIG_SECURITY_SELINUX_DISABLE is valid as of current linux- > > security.git#next . > > We could fold that removal into this series? I'm personally in favor of removing it, but that support was originally requested by the Fedora distro folks on the grounds that it is too painful to manage kernel boot parameters on some platforms, and therefore they needed an alternative to booting with selinux=0 on the kernel command line. The documented way to disable SELinux on such distros is through the use of /etc/selinux/config SELINUX=disabled, which relies on this mechanism. So you'd have to work through removal with the distro folks. Maybe in the interim we could just wrap the ro-after-init markings under a conditional dependent on !CONFIG_SECURITY_SELINUX_DISABLE so that systems (e.g. Android) that do not rely on this feature could benefit.
On 2/13/2017 1:03 PM, Stephen Smalley wrote: > On Mon, 2017-02-13 at 09:29 -0800, Kees Cook wrote: >> On Mon, Feb 13, 2017 at 3:29 AM, Tetsuo Handa >> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>> James Morris wrote: >>>> Both SELinux and Smack register Netfilter operations during init, >>>> which then don't change. Mark these ops as __ro_after_init. >>>> >>>> Signed-off-by: James Morris <james.l.morris@oracle.com> >>> This patch breaks CONFIG_SECURITY_SELINUX_DISABLE=y + >>> SELINUX=disabled in /etc/selinux/config case, >>> doesn't it? Although I heard that SELinux is planning to remove >>> CONFIG_SECURITY_SELINUX_DISABLE, >>> CONFIG_SECURITY_SELINUX_DISABLE is valid as of current linux- >>> security.git#next . >> We could fold that removal into this series? > I'm personally in favor of removing it, but that support was originally > requested by the Fedora distro folks on the grounds that it is too > painful to manage kernel boot parameters on some platforms, and > therefore they needed an alternative to booting with selinux=0 on the > kernel command line. The documented way to disable SELinux on such > distros is through the use of /etc/selinux/config SELINUX=disabled, > which relies on this mechanism. So you'd have to work through removal > with the distro folks. > > Maybe in the interim we could just wrap the ro-after-init markings > under a conditional dependent on !CONFIG_SECURITY_SELINUX_DISABLE so > that systems (e.g. Android) that do not rely on this feature could > benefit. If we changed CONFIG_SECURITY_SELINUX_DISABLE to CONFIG_SECURITY_DYNAMIC_MODULES and put the __ro_after_init under !CONFIG_SECURITY_DYNAMIC_MODULES we solve both the current and potential future issues. > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon, Feb 13, 2017 at 1:32 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 2/13/2017 1:03 PM, Stephen Smalley wrote: >> On Mon, 2017-02-13 at 09:29 -0800, Kees Cook wrote: >>> On Mon, Feb 13, 2017 at 3:29 AM, Tetsuo Handa >>> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>>> James Morris wrote: >>>>> Both SELinux and Smack register Netfilter operations during init, >>>>> which then don't change. Mark these ops as __ro_after_init. >>>>> >>>>> Signed-off-by: James Morris <james.l.morris@oracle.com> >>>> This patch breaks CONFIG_SECURITY_SELINUX_DISABLE=y + >>>> SELINUX=disabled in /etc/selinux/config case, >>>> doesn't it? Although I heard that SELinux is planning to remove >>>> CONFIG_SECURITY_SELINUX_DISABLE, >>>> CONFIG_SECURITY_SELINUX_DISABLE is valid as of current linux- >>>> security.git#next . >>> We could fold that removal into this series? >> I'm personally in favor of removing it, but that support was originally >> requested by the Fedora distro folks on the grounds that it is too >> painful to manage kernel boot parameters on some platforms, and >> therefore they needed an alternative to booting with selinux=0 on the >> kernel command line. The documented way to disable SELinux on such >> distros is through the use of /etc/selinux/config SELINUX=disabled, >> which relies on this mechanism. So you'd have to work through removal >> with the distro folks. >> >> Maybe in the interim we could just wrap the ro-after-init markings >> under a conditional dependent on !CONFIG_SECURITY_SELINUX_DISABLE so >> that systems (e.g. Android) that do not rely on this feature could >> benefit. > > If we changed CONFIG_SECURITY_SELINUX_DISABLE to > CONFIG_SECURITY_DYNAMIC_MODULES and put the __ro_after_init > under !CONFIG_SECURITY_DYNAMIC_MODULES we solve both the > current and potential future issues. Something like... #ifdef CONFIG_SECURITY_DYNAMIC_LSM # define lsm_ro_after_init __ro_after_init # define lsm_const const #else # define lsm_ro_after_init # define lsm_const #endif ? -Kees
On 2/13/2017 1:49 PM, Kees Cook wrote: > On Mon, Feb 13, 2017 at 1:32 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 2/13/2017 1:03 PM, Stephen Smalley wrote: >>> On Mon, 2017-02-13 at 09:29 -0800, Kees Cook wrote: >>>> On Mon, Feb 13, 2017 at 3:29 AM, Tetsuo Handa >>>> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>>>> James Morris wrote: >>>>>> Both SELinux and Smack register Netfilter operations during init, >>>>>> which then don't change. Mark these ops as __ro_after_init. >>>>>> >>>>>> Signed-off-by: James Morris <james.l.morris@oracle.com> >>>>> This patch breaks CONFIG_SECURITY_SELINUX_DISABLE=y + >>>>> SELINUX=disabled in /etc/selinux/config case, >>>>> doesn't it? Although I heard that SELinux is planning to remove >>>>> CONFIG_SECURITY_SELINUX_DISABLE, >>>>> CONFIG_SECURITY_SELINUX_DISABLE is valid as of current linux- >>>>> security.git#next . >>>> We could fold that removal into this series? >>> I'm personally in favor of removing it, but that support was originally >>> requested by the Fedora distro folks on the grounds that it is too >>> painful to manage kernel boot parameters on some platforms, and >>> therefore they needed an alternative to booting with selinux=0 on the >>> kernel command line. The documented way to disable SELinux on such >>> distros is through the use of /etc/selinux/config SELINUX=disabled, >>> which relies on this mechanism. So you'd have to work through removal >>> with the distro folks. >>> >>> Maybe in the interim we could just wrap the ro-after-init markings >>> under a conditional dependent on !CONFIG_SECURITY_SELINUX_DISABLE so >>> that systems (e.g. Android) that do not rely on this feature could >>> benefit. >> If we changed CONFIG_SECURITY_SELINUX_DISABLE to >> CONFIG_SECURITY_DYNAMIC_MODULES and put the __ro_after_init >> under !CONFIG_SECURITY_DYNAMIC_MODULES we solve both the >> current and potential future issues. > Something like... > > #ifdef CONFIG_SECURITY_DYNAMIC_LSM > # define lsm_ro_after_init __ro_after_init > # define lsm_const const > #else > # define lsm_ro_after_init > # define lsm_const > #endif > > ? That's more elaborate than I was thinking, but sure. I was going to put ifdefs in the respective declarations, but this may be more in line with modern tastes. > > -Kees >
Kees Cook wrote: > On Mon, Feb 13, 2017 at 1:32 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > > If we changed CONFIG_SECURITY_SELINUX_DISABLE to > > CONFIG_SECURITY_DYNAMIC_MODULES and put the __ro_after_init > > under !CONFIG_SECURITY_DYNAMIC_MODULES we solve both the > > current and potential future issues. > > Something like... > > #ifdef CONFIG_SECURITY_DYNAMIC_LSM > # define lsm_ro_after_init __ro_after_init > # define lsm_const const > #else > # define lsm_ro_after_init > # define lsm_const > #endif > > ? Fedora/RHEL won't use CONFIG_SECURITY_DYNAMIC_LSM=y whereas LKM based LSMs are targeted for such distributions. I don't worry much about Android, for manufactures who ship their products with TOMOYO enabled can rebuild their kernels. But asking for rebuild of Fedora/RHEL kernels to end users is too painful.
On Mon, Feb 13, 2017 at 2:05 PM, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > Kees Cook wrote: >> On Mon, Feb 13, 2017 at 1:32 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> > If we changed CONFIG_SECURITY_SELINUX_DISABLE to >> > CONFIG_SECURITY_DYNAMIC_MODULES and put the __ro_after_init >> > under !CONFIG_SECURITY_DYNAMIC_MODULES we solve both the >> > current and potential future issues. >> >> Something like... >> >> #ifdef CONFIG_SECURITY_DYNAMIC_LSM >> # define lsm_ro_after_init __ro_after_init >> # define lsm_const const >> #else >> # define lsm_ro_after_init >> # define lsm_const >> #endif >> >> ? > > Fedora/RHEL won't use CONFIG_SECURITY_DYNAMIC_LSM=y whereas > LKM based LSMs are targeted for such distributions. > > I don't worry much about Android, for manufactures who ship their > products with TOMOYO enabled can rebuild their kernels. But asking > for rebuild of Fedora/RHEL kernels to end users is too painful. I thought the argument was that Fedora WOULD ship that way, since it needs to have the run-time selinux disabling feature? -Kees
Kees Cook wrote: > On Mon, Feb 13, 2017 at 2:05 PM, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > Kees Cook wrote: > >> On Mon, Feb 13, 2017 at 1:32 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > >> > If we changed CONFIG_SECURITY_SELINUX_DISABLE to > >> > CONFIG_SECURITY_DYNAMIC_MODULES and put the __ro_after_init > >> > under !CONFIG_SECURITY_DYNAMIC_MODULES we solve both the > >> > current and potential future issues. > >> > >> Something like... > >> > >> #ifdef CONFIG_SECURITY_DYNAMIC_LSM > >> # define lsm_ro_after_init __ro_after_init > >> # define lsm_const const > >> #else > >> # define lsm_ro_after_init > >> # define lsm_const > >> #endif > >> > >> ? > > > > Fedora/RHEL won't use CONFIG_SECURITY_DYNAMIC_LSM=y whereas > > LKM based LSMs are targeted for such distributions. > > > > I don't worry much about Android, for manufactures who ship their > > products with TOMOYO enabled can rebuild their kernels. But asking > > for rebuild of Fedora/RHEL kernels to end users is too painful. > > I thought the argument was that Fedora WOULD ship that way, since it > needs to have the run-time selinux disabling feature? True only if Fedora/RHEL doesn't separate kernel packages. They can build separate kernel packages for with-/etc/selinux/config environments and without-/etc/selinux/config environments because modules needed for those environments would differ.
On Mon, 13 Feb 2017, Casey Schaufler wrote: > If we changed CONFIG_SECURITY_SELINUX_DISABLE to > CONFIG_SECURITY_DYNAMIC_MODULES and put the __ro_after_init > under !CONFIG_SECURITY_DYNAMIC_MODULES we solve both the > current and potential future issues. We don't need to solve issues which don't exist and ideally will not exist.
On 2/13/2017 2:26 PM, James Morris wrote: > On Mon, 13 Feb 2017, Casey Schaufler wrote: > >> If we changed CONFIG_SECURITY_SELINUX_DISABLE to >> CONFIG_SECURITY_DYNAMIC_MODULES and put the __ro_after_init >> under !CONFIG_SECURITY_DYNAMIC_MODULES we solve both the >> current and potential future issues. > We don't need to solve issues which don't exist and ideally will not > exist. > There is a problem with CONFIG_SECURITY_SELINUX_DISABLE and __ro_after_init that does exist. Whether the possible future issue should or shouldn't exist has no bearing on the existing issue. It's true that we don't need to change CONFIG_SECURITY_SELINUX_DISABLE to CONFIG_SECURITY_DYNAMIC_MODULES to solve the current problem. I suggest that we leave that change to the separate debate on loadable security modules.
On Mon, 13 Feb 2017, Casey Schaufler wrote: > On 2/13/2017 2:26 PM, James Morris wrote: > > On Mon, 13 Feb 2017, Casey Schaufler wrote: > > > >> If we changed CONFIG_SECURITY_SELINUX_DISABLE to > >> CONFIG_SECURITY_DYNAMIC_MODULES and put the __ro_after_init > >> under !CONFIG_SECURITY_DYNAMIC_MODULES we solve both the > >> current and potential future issues. > > We don't need to solve issues which don't exist and ideally will not > > exist. > > > There is a problem with CONFIG_SECURITY_SELINUX_DISABLE > and __ro_after_init that does exist. Whether the possible > future issue should or shouldn't exist has no bearing on > the existing issue. It's true that we don't need to change > CONFIG_SECURITY_SELINUX_DISABLE to CONFIG_SECURITY_DYNAMIC_MODULES > to solve the current problem. I suggest that we leave that > change to the separate debate on loadable security modules. Agreed.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 9599e97..37a7866 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6379,7 +6379,7 @@ void selinux_complete_init(void) #if defined(CONFIG_NETFILTER) -static struct nf_hook_ops selinux_nf_ops[] = { +static struct nf_hook_ops selinux_nf_ops[] __ro_after_init = { { .hook = selinux_ipv4_postroute, .pf = NFPROTO_IPV4, diff --git a/security/smack/smack_netfilter.c b/security/smack/smack_netfilter.c index 205b785..b945f2c 100644 --- a/security/smack/smack_netfilter.c +++ b/security/smack/smack_netfilter.c @@ -57,7 +57,7 @@ static unsigned int smack_ipv4_output(void *priv, return NF_ACCEPT; } -static struct nf_hook_ops smack_nf_ops[] = { +static struct nf_hook_ops smack_nf_ops[] __ro_after_init = { { .hook = smack_ipv4_output, .pf = NFPROTO_IPV4,
Both SELinux and Smack register Netfilter operations during init, which then don't change. Mark these ops as __ro_after_init. Signed-off-by: James Morris <james.l.morris@oracle.com> --- security/selinux/hooks.c | 2 +- security/smack/smack_netfilter.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)