Re: [PATCH] LSM: add static to security_ops variable

From: wzt wzt
Date: Fri Feb 19 2010 - 06:41:14 EST

Next message: Andy Shevchenko: "Re: [PATCH 00/12] Clean up code to use native kernel functions when convert or check hex digits"
Previous message: Christian Ehrhardt: "Re: Performance regression in scsi sequential throughput (iozone)due to "e084b - page-allocator: preserve PFN ordering when __GFP_COLD isset""
In reply to: Stephen Smalley: "Re: [PATCH] LSM: add static to security_ops variable"
Next in thread: Stephen Smalley: "Re: [PATCH] LSM: add static to security_ops variable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> ---
>> Â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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Andy Shevchenko: "Re: [PATCH 00/12] Clean up code to use native kernel functions when convert or check hex digits"
Previous message: Christian Ehrhardt: "Re: Performance regression in scsi sequential throughput (iozone)due to "e084b - page-allocator: preserve PFN ordering when __GFP_COLD isset""
In reply to: Stephen Smalley: "Re: [PATCH] LSM: add static to security_ops variable"
Next in thread: Stephen Smalley: "Re: [PATCH] LSM: add static to security_ops variable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]