diff mbox

[RFC,2/4] security: mark nf ops in SELinux and Smack as __ro_after_init

Message ID alpine.LRH.2.20.1702131632340.8914@namei.org (mailing list archive)
State New, archived
Headers show

Commit Message

James Morris Feb. 13, 2017, 5:33 a.m. UTC
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(-)

Comments

Tetsuo Handa Feb. 13, 2017, 11:29 a.m. UTC | #1
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 .
--
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
Kees Cook Feb. 13, 2017, 5:29 p.m. UTC | #2
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
Stephen Smalley Feb. 13, 2017, 9:03 p.m. UTC | #3
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.
--
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
Casey Schaufler Feb. 13, 2017, 9:32 p.m. UTC | #4
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
>

--
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
Kees Cook Feb. 13, 2017, 9:49 p.m. UTC | #5
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
Casey Schaufler Feb. 13, 2017, 10:01 p.m. UTC | #6
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
>

--
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
Tetsuo Handa Feb. 13, 2017, 10:05 p.m. UTC | #7
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.
--
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
Kees Cook Feb. 13, 2017, 10:09 p.m. UTC | #8
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
Tetsuo Handa Feb. 13, 2017, 10:15 p.m. UTC | #9
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.
--
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
James Morris Feb. 13, 2017, 10:26 p.m. UTC | #10
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.
Casey Schaufler Feb. 14, 2017, 1:58 a.m. UTC | #11
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.


--
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
James Morris Feb. 14, 2017, 2:46 a.m. UTC | #12
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 mbox

Patch

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,