lkml.org 
[lkml]   [2010]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] LSM: add static to security_ops variable
From
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@tycho.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@alibaba-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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2010-02-19 12:43    [W:0.087 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site