| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 19 Feb 2010 19:40:55 +0800
From: wzt wzt <wzt.wzt@...il.com>
To: Stephen Smalley <sds@...ho.nsa.gov>
Cc: linux-kernel@...r.kernel.org, jmorris@...ei.org,
eparis@...isplace.org, lsm <linux-security-module@...r.kernel.org>,
Greg Kroah-Hartman <greg@...ah.com>
Subject: Re: [PATCH] LSM: add static to security_ops variable
I rewrite the patch, thx.
---
include/linux/security.h | 2 ++
security/security.c | 7 ++++++-
security/selinux/hooks.c | 14 ++------------
3 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 2c627d3..3a15b57 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -95,6 +95,8 @@ struct seq_file;
extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb);
extern int cap_netlink_recv(struct sk_buff *skb, int cap);
+extern void reset_security_ops(void);
+
#ifdef CONFIG_MMU
extern unsigned long mmap_min_addr;
extern unsigned long dac_mmap_min_addr;
diff --git a/security/security.c b/security/security.c
index 122b748..3e4c4bc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -26,7 +26,7 @@ static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
extern struct security_operations default_security_ops;
extern void security_fixup_ops(struct security_operations *ops);
-struct security_operations *security_ops; /* Initialized to NULL */
+static struct security_operations *security_ops; /* Initialized
to NULL */
static inline int verify(struct security_operations *ops)
{
@@ -63,6 +63,11 @@ int __init security_init(void)
return 0;
}
+void reset_security_ops(void)
+{
+ security_ops = &default_security_ops;
+}
+
/* Save user chosen LSM */
static int __init choose_lsm(char *str)
{
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9a2ee84..e9599fd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -93,6 +93,7 @@
extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
extern struct security_operations *security_ops;
+extern struct security_operations default_security_ops;
/* SECMARK reference count */
atomic_t selinux_secmark_refcount = ATOMIC_INIT(0);
@@ -125,13 +126,6 @@ __setup("selinux=", selinux_enabled_setup);
int selinux_enabled = 1;
#endif
-
-/*
- * Minimal support for a secondary security module,
- * just to allow the use of the capability module.
- */
-static struct security_operations *secondary_ops;
-
/* Lists of inode and superblock security structures initialized
before the policy was loaded. */
static LIST_HEAD(superblock_security_head);
@@ -5672,9 +5666,6 @@ static __init int selinux_init(void)
0, SLAB_PANIC, NULL);
avc_init();
- secondary_ops = security_ops;
- if (!secondary_ops)
- panic("SELinux: No initial security operations\n");
if (register_security(&selinux_ops))
panic("SELinux: Unable to register with kernel.\n");
@@ -5835,8 +5826,7 @@ int selinux_disable(void)
selinux_disabled = 1;
selinux_enabled = 0;
- /* Reset security_ops to the secondary module, dummy or capability. */
- security_ops = secondary_ops;
+ reset_security_ops();
/* Try to destroy the avc node cache */
avc_disable();
--
1.6.5.3
On Tue, Feb 16, 2010 at 10:57 PM, Stephen Smalley <sds@...ho.nsa.gov> wrote:
> On Sun, 2010-02-07 at 19:24 +0800, wzt wzt wrote:
>> security_ops was declared as a global variable, so other drivers or
>> kernel code can easily change its value, like:
>>
>> extern struct security_operations *security_ops;
>> security_ops = NULL;
>>
>> then insmod this driver immediately, it will get an oops. Other evil
>> drivers can aslo fake this variable as extern.
>
> I'd support a patch along these lines (but with the changes below) for a
> different reason: at present, SELinux directly manipulates security_ops
> for the purpose of runtime disable support, whereas that ought to be
> handled by the security framework.
>
>>
>> Signed-off-by: wzt <zhitong.wangzt@...baba-inc.com>
>> ---
>> security/security.c | 25 ++++++++++++++++++++++++-
>> security/selinux/hooks.c | 18 ++++++------------
>> 2 files changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/security/security.c b/security/security.c
>> index 24e060b..781117d 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -26,7 +26,12 @@ static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
>> extern struct security_operations default_security_ops;
>> extern void security_fixup_ops(struct security_operations *ops);
>>
>> -struct security_operations *security_ops; /* Initialized to NULL */
>> +static struct security_operations *security_ops; /* Initialized
>> to NULL */
>> +/*
>> + * Minimal support for a secondary security module,
>> + * just to allow the use of the capability module.
>> + */
>
> The comment is no longer accurate - secondary_ops was originally used by
> SELinux to call the "secondary" security module (capability or dummy),
> but that was replaced by direct calls to capability and the only
> remaining use is to save and restore the original security ops pointer
> value if SELinux is disabled by early userspace based
> on /etc/selinux/config. Further, if we support this directly in the
> security framework, then we can just use &default_security_ops for this
> purpose since that is now available. So I don't believe we need
> secondary_ops at all.
>
>> +static struct security_operations *secondary_ops;
>
> We don't need the above variable at all.
>
>> static inline int verify(struct security_operations *ops)
>> {
>> @@ -63,6 +68,24 @@ int __init security_init(void)
>> return 0;
>> }
>>
>> +void reset_secondary_ops(void)
>> +{
>> + secondary_ops = security_ops;
>> + if (!secondary_ops)
>> + panic("SELinux: No initial security operations\n");
>> +}
>
> We don't need the above function at all.
>
>> +
>> +void reset_security_ops(void)
>> +{
>> + /* Reset security_ops to the secondary module, dummy or capability. */
>
> The dummy module was removed so this can only be capability.
>
>> + security_ops = secondary_ops;
>
> This can just be:
> security_ops = &default_security_ops;
>
>> +}
>>
>> /* Save user chosen LSM */
>> static int __init choose_lsm(char *str)
>> {
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 9a2ee84..9e8607e 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -92,7 +92,9 @@
>> #define NUM_SEL_MNT_OPTS 5
>>
>> extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
>> -extern struct security_operations *security_ops;
>> +extern void reset_secondary_ops(void);
>> +extern void reset_security_ops(void);
>
> The extern declaration for reset_security_ops() would properly go in
> include/linux/security.h for general use by security modules.
>
>> /* SECMARK reference count */
>> atomic_t selinux_secmark_refcount = ATOMIC_INIT(0);
>> @@ -126,12 +128,6 @@ int selinux_enabled = 1;
>> #endif
>>
>>
>> -/*
>> - * Minimal support for a secondary security module,
>> - * just to allow the use of the capability module.
>> - */
>> -static struct security_operations *secondary_ops;
>> -
>> /* Lists of inode and superblock security structures initialized
>> before the policy was loaded. */
>> static LIST_HEAD(superblock_security_head);
>> @@ -5672,9 +5668,8 @@ static __init int selinux_init(void)
>> 0, SLAB_PANIC, NULL);
>> avc_init();
>>
>> - secondary_ops = security_ops;
>> - if (!secondary_ops)
>> - panic("SELinux: No initial security operations\n");
>> + reset_secondary_ops();
>> +
>
> We don't need to save this value as it is available via
> &default_security_ops and there is now only one possible value (dummy
> module killed).
>
>> if (register_security(&selinux_ops))
>> panic("SELinux: Unable to register with kernel.\n");
>>
>> @@ -5835,8 +5830,7 @@ int selinux_disable(void)
>> selinux_disabled = 1;
>> selinux_enabled = 0;
>>
>> - /* Reset security_ops to the secondary module, dummy or capability. */
>> - security_ops = secondary_ops;
>> + reset_security_ops();
>
> So this is the only change needed here.
>
>>
>> /* Try to destroy the avc node cache */
>> avc_disable();
> --
> Stephen Smalley
> National Security Agency
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists