[PATCH -mm 0/4] LSM interfaced Audit (SELinux audit separation)

[PATCH -mm 0/4] LSM interfaced Audit (SELinux audit separation)

[Ahmed S. Darwish]

Hi everybody,

This is a beginning of work (started and suggested by Casey Schaufler)
to let Audit be LSM neutral. This is done for proper audit<->SMACK 
integration which will also be useful for any future LSM.

What follows is four patches to remove the following exported 
SElinux interfaces:
selinux_get_inode_sid(inode, sid)
selinux_get_ipc_sid(ipcp, sid) 
selinux_get_task_sid(tsk, sid)
selinux_sid_to_string(sid, ctx, len)

and substitue them respectively with:
new LSM hook, inode_getsecid(inode, secid)
new LSM hook, ipc_getsecid*(ipcp, secid)
LSM hook, task_getsecid(tsk, secid)
LSM hook, sid_to_secctx(sid, ctx, len)

The work isn't complete yet, and those four patches are sent for
an early review. A new LSM interfaces/hooks will be created to
substitute the SELinux exported audit interfaces, thus completing
the separation.

It's worthy to note that those changes can be merged in
their current state. The tree is fully grepped to make sure
that no subsystem ,except the patched ones, will be affected
by this SELinux API breakage.

Diffstat:

 include/linux/security.h   |   23 +++++++++++++++-
 include/linux/selinux.h    |   62 ---------------------------------------------
 kernel/audit.c             |   14 +++++-----
 kernel/auditfilter.c       |    5 ++-
 kernel/auditsc.c           |   37 +++++++++++++-------------
 net/netlink/af_netlink.c   |    3 --
 security/dummy.c           |   16 ++++++++++-
 security/security.c        |   12 ++++++++
 security/selinux/exports.c |   42 ------------------------------
 security/selinux/hooks.c   |   19 ++++++++++++-
 10 files changed, 95 insertions(+), 138 deletions(-)

Thanks in advance for your reviews and comments.

-- 
Ahmed S. Darwish
Blog: http://darwish-07.blogspot.com
Homepage: http://darwish.07.googlepages.com

[PATCH -mm 1/4] LSM: Introduce inode_getsecid and ipc_getsecid hooks

[Ahmed S. Darwish]

Introduce inode_getsecid(inode, secid) and ipc_getsecid(ipcp, secid)
LSM hooks.

This hooks will be used instead of similar exported SELinux 
interfaces.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

 include/linux/security.h |   18 ++++++++++++++++++
 security/dummy.c         |   12 ++++++++++++
 security/security.c      |   12 ++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/include/linux/security.h b/include/linux/security.h
index a33fd03..a7fb136 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -449,6 +449,10 @@ struct request_sock;
  *	@dentry is the dentry being changed.
  *	Return 0 on success.  If error is returned, then the operation
  *	causing setuid bit removal is failed.
+ * @inode_getsecid:
+ *      Get the secid associated with the node
+ *      @inode contains a pointer to the inode
+ *      @secid contains a pointer to the location to store the result
  *
  * Security hooks for file operations
  *
@@ -989,6 +993,10 @@ struct request_sock;
  *	@ipcp contains the kernel IPC permission structure
  *	@flag contains the desired (requested) permission set
  *	Return 0 if permission is granted.
+ * @ipc_getsecid:
+ *      Get the secid associated with the ipc object
+ *      @ipcp contains the kernel IPC permission structure
+ *      @secid contains a pointer to the location where result will be saved
  *
  * Security hooks for individual messages held in System V IPC message queues
  * @msg_msg_alloc_security:
@@ -1310,6 +1318,7 @@ struct security_operations {
 	int (*inode_getsecurity)(const struct inode *inode, const char *name, void **buffer, bool alloc);
   	int (*inode_setsecurity)(struct inode *inode, const char *name, const void *value, size_t size, int flags);
   	int (*inode_listsecurity)(struct inode *inode, char *buffer, size_t buffer_size);
+	void (*inode_getsecid)(const struct inode *inode, u32 *secid);
 
 	int (*file_permission) (struct file * file, int mask);
 	int (*file_alloc_security) (struct file * file);
@@ -1362,6 +1371,7 @@ struct security_operations {
 	void (*task_to_inode)(struct task_struct *p, struct inode *inode);
 
 	int (*ipc_permission) (struct kern_ipc_perm * ipcp, short flag);
+	void (*ipc_getsecid) (struct kern_ipc_perm *ipcp, u32 *secid);
 
 	int (*msg_msg_alloc_security) (struct msg_msg * msg);
 	void (*msg_msg_free_security) (struct msg_msg * msg);
@@ -1571,6 +1581,7 @@ int security_inode_killpriv(struct dentry *dentry);
 int security_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc);
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
+void security_inode_getsecid(const struct inode *inode, u32 *secid);
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
@@ -1615,6 +1626,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 void security_task_reparent_to_init(struct task_struct *p);
 void security_task_to_inode(struct task_struct *p, struct inode *inode);
 int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag);
+void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid);
 int security_msg_msg_alloc(struct msg_msg *msg);
 void security_msg_msg_free(struct msg_msg *msg);
 int security_msg_queue_alloc(struct msg_queue *msq);
@@ -1985,6 +1997,9 @@ static inline int security_inode_listsecurity(struct inode *inode, char *buffer,
 	return 0;
 }
 
+static inline void security_inode_getsecid(const struct inode *inode, u32 *secid)
+{ }
+
 static inline int security_file_permission (struct file *file, int mask)
 {
 	return 0;
@@ -2179,6 +2194,9 @@ static inline int security_ipc_permission (struct kern_ipc_perm *ipcp,
 	return 0;
 }
 
+static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid)
+{ }
+
 static inline int security_msg_msg_alloc (struct msg_msg * msg)
 {
 	return 0;
diff --git a/security/dummy.c b/security/dummy.c
index 6a0056b..c2f4c52 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -422,6 +422,11 @@ static int dummy_inode_listsecurity(struct inode *inode, char *buffer, size_t bu
 	return 0;
 }
 
+static void dummy_inode_getsecid(const struct inode *inode, u32 *secid)
+{
+	return;
+}
+
 static int dummy_file_permission (struct file *file, int mask)
 {
 	return 0;
@@ -614,6 +619,11 @@ static int dummy_ipc_permission (struct kern_ipc_perm *ipcp, short flag)
 	return 0;
 }
 
+static void dummy_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid)
+{
+	return;
+}
+
 static int dummy_msg_msg_alloc_security (struct msg_msg *msg)
 {
 	return 0;
@@ -1062,6 +1072,7 @@ void security_fixup_ops (struct security_operations *ops)
 	set_to_dummy_if_null(ops, inode_getsecurity);
 	set_to_dummy_if_null(ops, inode_setsecurity);
 	set_to_dummy_if_null(ops, inode_listsecurity);
+	set_to_dummy_if_null(ops, inode_getsecid);
 	set_to_dummy_if_null(ops, file_permission);
 	set_to_dummy_if_null(ops, file_alloc_security);
 	set_to_dummy_if_null(ops, file_free_security);
@@ -1098,6 +1109,7 @@ void security_fixup_ops (struct security_operations *ops)
 	set_to_dummy_if_null(ops, task_reparent_to_init);
  	set_to_dummy_if_null(ops, task_to_inode);
 	set_to_dummy_if_null(ops, ipc_permission);
+	set_to_dummy_if_null(ops, ipc_getsecid);
 	set_to_dummy_if_null(ops, msg_msg_alloc_security);
 	set_to_dummy_if_null(ops, msg_msg_free_security);
 	set_to_dummy_if_null(ops, msg_queue_alloc_security);
diff --git a/security/security.c b/security/security.c
index 3e75b90..fcf2be3 100644
--- a/security/security.c
+++ b/security/security.c
@@ -516,6 +516,12 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer
 	return security_ops->inode_listsecurity(inode, buffer, buffer_size);
 }
 
+void security_inode_getsecid(const struct inode *inode, u32 *secid)
+{
+	security_ops->inode_getsecid(inode, secid);
+}
+EXPORT_SYMBOL(security_inode_getsecid);
+
 int security_file_permission(struct file *file, int mask)
 {
 	return security_ops->file_permission(file, mask);
@@ -705,6 +711,12 @@ int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
 	return security_ops->ipc_permission(ipcp, flag);
 }
 
+void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid)
+{
+	security_ops->ipc_getsecid(ipcp, secid);
+}
+EXPORT_SYMBOL(security_ipc_getsecid);
+
 int security_msg_msg_alloc(struct msg_msg *msg)
 {
 	return security_ops->msg_msg_alloc_security(msg);

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

[PATCH -mm 2/4] SELinux: Remove various exported symbols

[Ahmed S. Darwish]

Remove the following exported SELinux interfaces:
selinux_get_inode_sid(inode, sid)
selinux_get_ipc_sid(ipcp, sid) 
selinux_get_task_sid(tsk, sid)
selinux_sid_to_string(sid, ctx, len)

and substitue them with following equivalents respectively:
new LSM hook, inode_getsecid(inode, secid)
new LSM hook, ipc_getsecid*(ipcp, secid)
LSM hook, task_getsecid(tsk, secid)
LSM hook, sid_to_secctx(sid, ctx, len)

This is done to remove SELinux dependency from some
of the kernel subsystems (audit).

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

 include/linux/selinux.h    |   62 ---------------------------------------------
 security/selinux/exports.c |   42 ------------------------------
 security/selinux/hooks.c   |   19 ++++++++++++-
 3 files changed, 17 insertions(+), 106 deletions(-)

diff --git a/include/linux/selinux.h b/include/linux/selinux.h
index 8c2cc4c..24b0af1 100644
--- a/include/linux/selinux.h
+++ b/include/linux/selinux.h
@@ -16,7 +16,6 @@
 
 struct selinux_audit_rule;
 struct audit_context;
-struct inode;
 struct kern_ipc_perm;
 
 #ifdef CONFIG_SECURITY_SELINUX
@@ -70,45 +69,6 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op,
 void selinux_audit_set_callback(int (*callback)(void));
 
 /**
- *     selinux_sid_to_string - map a security context ID to a string
- *     @sid: security context ID to be converted.
- *     @ctx: address of context string to be returned
- *     @ctxlen: length of returned context string.
- *
- *     Returns 0 if successful, -errno if not.  On success, the context
- *     string will be allocated internally, and the caller must call
- *     kfree() on it after use.
- */
-int selinux_sid_to_string(u32 sid, char **ctx, u32 *ctxlen);
-
-/**
- *     selinux_get_inode_sid - get the inode's security context ID
- *     @inode: inode structure to get the sid from.
- *     @sid: pointer to security context ID to be filled in.
- *
- *     Returns nothing
- */
-void selinux_get_inode_sid(const struct inode *inode, u32 *sid);
-
-/**
- *     selinux_get_ipc_sid - get the ipc security context ID
- *     @ipcp: ipc structure to get the sid from.
- *     @sid: pointer to security context ID to be filled in.
- *
- *     Returns nothing
- */
-void selinux_get_ipc_sid(const struct kern_ipc_perm *ipcp, u32 *sid);
-
-/**
- *     selinux_get_task_sid - return the SID of task
- *     @tsk: the task whose SID will be returned
- *     @sid: pointer to security context ID to be filled in.
- *
- *     Returns nothing
- */
-void selinux_get_task_sid(struct task_struct *tsk, u32 *sid);
-
-/**
  *     selinux_string_to_sid - map a security context string to a security ID
  *     @str: the security context string to be mapped
  *     @sid: ID value returned via this.
@@ -175,28 +135,6 @@ static inline void selinux_audit_set_callback(int (*callback)(void))
 	return;
 }
 
-static inline int selinux_sid_to_string(u32 sid, char **ctx, u32 *ctxlen)
-{
-       *ctx = NULL;
-       *ctxlen = 0;
-       return 0;
-}
-
-static inline void selinux_get_inode_sid(const struct inode *inode, u32 *sid)
-{
-	*sid = 0;
-}
-
-static inline void selinux_get_ipc_sid(const struct kern_ipc_perm *ipcp, u32 *sid)
-{
-	*sid = 0;
-}
-
-static inline void selinux_get_task_sid(struct task_struct *tsk, u32 *sid)
-{
-	*sid = 0;
-}
-
 static inline int selinux_string_to_sid(const char *str, u32 *sid)
 {
        *sid = 0;
diff --git a/security/selinux/exports.c b/security/selinux/exports.c
index 87d2bb3..64af2d3 100644
--- a/security/selinux/exports.c
+++ b/security/selinux/exports.c
@@ -25,48 +25,6 @@
 /* SECMARK reference count */
 extern atomic_t selinux_secmark_refcount;
 
-int selinux_sid_to_string(u32 sid, char **ctx, u32 *ctxlen)
-{
-	if (selinux_enabled)
-		return security_sid_to_context(sid, ctx, ctxlen);
-	else {
-		*ctx = NULL;
-		*ctxlen = 0;
-	}
-
-	return 0;
-}
-
-void selinux_get_inode_sid(const struct inode *inode, u32 *sid)
-{
-	if (selinux_enabled) {
-		struct inode_security_struct *isec = inode->i_security;
-		*sid = isec->sid;
-		return;
-	}
-	*sid = 0;
-}
-
-void selinux_get_ipc_sid(const struct kern_ipc_perm *ipcp, u32 *sid)
-{
-	if (selinux_enabled) {
-		struct ipc_security_struct *isec = ipcp->security;
-		*sid = isec->sid;
-		return;
-	}
-	*sid = 0;
-}
-
-void selinux_get_task_sid(struct task_struct *tsk, u32 *sid)
-{
-	if (selinux_enabled) {
-		struct task_security_struct *tsec = tsk->security;
-		*sid = tsec->sid;
-		return;
-	}
-	*sid = 0;
-}
-
 int selinux_string_to_sid(char *str, u32 *sid)
 {
 	if (selinux_enabled)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f42ebfc..62a0f26 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2724,6 +2724,12 @@ static int selinux_inode_killpriv(struct dentry *dentry)
 	return secondary_ops->inode_killpriv(dentry);
 }
 
+static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
+{
+	struct inode_security_struct *isec = inode->i_security;
+	*secid = isec->sid;
+}
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)
@@ -3120,7 +3126,8 @@ static int selinux_task_getsid(struct task_struct *p)
 
 static void selinux_task_getsecid(struct task_struct *p, u32 *secid)
 {
-	selinux_get_task_sid(p, secid);
+	struct task_security_struct *tsec = p->security;
+	*secid = tsec->sid;
 }
 
 static int selinux_task_setgroups(struct group_info *group_info)
@@ -4090,7 +4097,7 @@ static int selinux_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *
 		goto out;
 
 	if (sock && family == PF_UNIX)
-		selinux_get_inode_sid(SOCK_INODE(sock), &peer_secid);
+		selinux_inode_getsecid(SOCK_INODE(sock), &peer_secid);
 	else if (skb)
 		selinux_skb_peerlbl_sid(skb, family, &peer_secid);
 
@@ -4970,6 +4977,12 @@ static int selinux_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
 	return ipc_has_perm(ipcp, av);
 }
 
+static void selinux_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid)
+{
+	struct ipc_security_struct *isec = ipcp->security;
+	*secid = isec->sid;
+}
+
 /* module stacking operations */
 static int selinux_register_security (const char *name, struct security_operations *ops)
 {
@@ -5292,6 +5305,7 @@ static struct security_operations selinux_ops = {
 	.inode_listsecurity =           selinux_inode_listsecurity,
 	.inode_need_killpriv =		selinux_inode_need_killpriv,
 	.inode_killpriv =		selinux_inode_killpriv,
+	.inode_getsecid =               selinux_inode_getsecid,
 
 	.file_permission =		selinux_file_permission,
 	.file_alloc_security =		selinux_file_alloc_security,
@@ -5332,6 +5346,7 @@ static struct security_operations selinux_ops = {
 	.task_to_inode =                selinux_task_to_inode,
 
 	.ipc_permission =		selinux_ipc_permission,
+	.ipc_getsecid =                 selinux_ipc_getsecid,
 
 	.msg_msg_alloc_security =	selinux_msg_msg_alloc_security,
 	.msg_msg_free_security =	selinux_msg_msg_free_security,

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

[PATCH -mm 3/4] Audit: start not to use SELinux exported symbols

[Ahmed S. Darwish]

Remove the following exported SELinux interfaces:
selinux_get_inode_sid(inode, sid)
selinux_get_ipc_sid(ipcp, sid) 
selinux_get_task_sid(tsk, sid)
selinux_sid_to_string(sid, ctx, len)

and substitue them with following generic LSM equivalents 
respectively:
security_inode_getsecid(inode, secid)
security_ipc_getsecid*(ipcp, secid)
security_task_getsecid(tsk, secid)
security_sid_to_secctx(sid, ctx, len)

Let the security_task_getsecid(tsk, secid) LSM hook set
the secid to 0 by default if CONFIG_SECURITY is not defined 
or if the hook is set to NULL (dummy). This is done to
notify the caller that no valid secid exists.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

 include/linux/security.h |    5 ++++-
 kernel/audit.c           |   14 +++++++-------
 kernel/auditfilter.c     |    5 +++--
 kernel/auditsc.c         |   37 ++++++++++++++++++-------------------
 security/dummy.c         |    4 +++-
 6 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index a7fb136..35c98f0 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -621,6 +621,7 @@ struct request_sock;
  * @task_getsecid:
  *	Retrieve the security identifier of the process @p.
  *	@p contains the task_struct for the process and place is into @secid.
+ *      In case of failure, @secid will be set to zero
  * @task_setgroups:
  *	Check permission before setting the supplementary group set of the
  *	current process.
@@ -2115,7 +2116,9 @@ static inline int security_task_getsid (struct task_struct *p)
 }
 
 static inline void security_task_getsecid (struct task_struct *p, u32 *secid)
-{ }
+{ 
+	*secid = 0;
+}
 
 static inline int security_task_setgroups (struct group_info *group_info)
 {
diff --git a/security/dummy.c b/security/dummy.c
index c2f4c52..56b007e 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -545,7 +545,9 @@ static int dummy_task_getsid (struct task_struct *p)
 }
 
 static void dummy_task_getsecid (struct task_struct *p, u32 *secid)
-{ }
+{
+	*secid = 0;
+}
 
 static int dummy_task_setgroups (struct group_info *group_info)
 {
diff --git a/kernel/audit.c b/kernel/audit.c
index 8316a88..2bd4124 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -259,13 +259,13 @@ static int audit_log_config_change(char *function_name, int new, int old,
 		char *ctx = NULL;
 		u32 len;
 
-		rc = selinux_sid_to_string(sid, &ctx, &len);
+		rc = security_secid_to_secctx(sid, &ctx, &len);
 		if (rc) {
 			audit_log_format(ab, " sid=%u", sid);
 			allow_changes = 0; /* Something weird, deny request */
 		} else {
 			audit_log_format(ab, " subj=%s", ctx);
-			kfree(ctx);
+			security_release_secctx(ctx, len);
 		}
 	}
 	audit_log_format(ab, " res=%d", allow_changes);
@@ -543,12 +543,12 @@ static int audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type,
 	audit_log_format(*ab, "user pid=%d uid=%u auid=%u",
 			 pid, uid, auid);
 	if (sid) {
-		rc = selinux_sid_to_string(sid, &ctx, &len);
+		rc = security_secid_to_secctx(sid, &ctx, &len);
 		if (rc)
 			audit_log_format(*ab, " ssid=%u", sid);
 		else
 			audit_log_format(*ab, " subj=%s", ctx);
-		kfree(ctx);
+		security_release_secctx(ctx, len);
 	}
 
 	return rc;
@@ -750,18 +750,18 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		break;
 	}
 	case AUDIT_SIGNAL_INFO:
-		err = selinux_sid_to_string(audit_sig_sid, &ctx, &len);
+		err = security_secid_to_secctx(audit_sig_sid, &ctx, &len);
 		if (err)
 			return err;
 		sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
 		if (!sig_data) {
-			kfree(ctx);
+			security_release_secctx(ctx, len);
 			return -ENOMEM;
 		}
 		sig_data->uid = audit_sig_uid;
 		sig_data->pid = audit_sig_pid;
 		memcpy(sig_data->ctx, ctx, len);
-		kfree(ctx);
+		security_release_secctx(ctx, len);
 		audit_send_reply(NETLINK_CB(skb).pid, seq, AUDIT_SIGNAL_INFO,
 				0, 0, sig_data, sizeof(*sig_data) + len);
 		kfree(sig_data);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 2f2914b..894e3bd 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -28,6 +28,7 @@
 #include <linux/netlink.h>
 #include <linux/sched.h>
 #include <linux/inotify.h>
+#include <linux/security.h>
 #include <linux/selinux.h>
 #include "audit.h"
 
@@ -1515,11 +1516,11 @@ static void audit_log_rule_change(uid_t loginuid, u32 sid, char *action,
 	if (sid) {
 		char *ctx = NULL;
 		u32 len;
-		if (selinux_sid_to_string(sid, &ctx, &len))
+		if (security_secid_to_secctx(sid, &ctx, &len))
 			audit_log_format(ab, " ssid=%u", sid);
 		else
 			audit_log_format(ab, " subj=%s", ctx);
-		kfree(ctx);
+		security_release_secctx(ctx, len);
 	}
 	audit_log_format(ab, " op=%s rule key=", action);
 	if (rule->filterkey)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d07fc4a..631c044 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -530,7 +530,7 @@ static int audit_filter_rules(struct task_struct *tsk,
 			   logged upon error */
 			if (f->se_rule) {
 				if (need_sid) {
-					selinux_get_task_sid(tsk, &sid);
+					security_task_getsecid(tsk, &sid);
 					need_sid = 0;
 				}
 				result = selinux_audit_rule_match(sid, f->type,
@@ -885,11 +885,11 @@ void audit_log_task_context(struct audit_buffer *ab)
 	int error;
 	u32 sid;
 
-	selinux_get_task_sid(current, &sid);
+	security_task_getsecid(current, &sid);
 	if (!sid)
 		return;
 
-	error = selinux_sid_to_string(sid, &ctx, &len);
+	error = security_secid_to_secctx(sid, &ctx, &len);
 	if (error) {
 		if (error != -EINVAL)
 			goto error_path;
@@ -897,7 +897,7 @@ void audit_log_task_context(struct audit_buffer *ab)
 	}
 
 	audit_log_format(ab, " subj=%s", ctx);
-	kfree(ctx);
+	security_release_secctx(ctx, len);
 	return;
 
 error_path:
@@ -951,7 +951,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
 
 	audit_log_format(ab, "opid=%d oauid=%d ouid=%d oses=%d", pid, auid,
 			 uid, sessionid);
-	if (selinux_sid_to_string(sid, &s, &len)) {
+	if (security_secid_to_secctx(sid, &s, &len)) {
 		audit_log_format(ab, " obj=(none)");
 		rc = 1;
 	} else
@@ -1268,14 +1268,14 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
 			if (axi->osid != 0) {
 				char *ctx = NULL;
 				u32 len;
-				if (selinux_sid_to_string(
+				if (security_secid_to_secctx(
 						axi->osid, &ctx, &len)) {
 					audit_log_format(ab, " osid=%u",
 							axi->osid);
 					call_panic = 1;
 				} else
 					audit_log_format(ab, " obj=%s", ctx);
-				kfree(ctx);
+				security_release_secctx(ctx, len);
 			}
 			break; }
 
@@ -1389,13 +1389,13 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
 		if (n->osid != 0) {
 			char *ctx = NULL;
 			u32 len;
-			if (selinux_sid_to_string(
+			if (security_secid_to_secctx(
 				n->osid, &ctx, &len)) {
 				audit_log_format(ab, " osid=%u", n->osid);
 				call_panic = 2;
 			} else
 				audit_log_format(ab, " obj=%s", ctx);
-			kfree(ctx);
+			security_release_secctx(ctx, len);
 		}
 
 		audit_log_end(ab);
@@ -1772,7 +1772,7 @@ static void audit_copy_inode(struct audit_names *name, const struct inode *inode
 	name->uid   = inode->i_uid;
 	name->gid   = inode->i_gid;
 	name->rdev  = inode->i_rdev;
-	selinux_get_inode_sid(inode, &name->osid);
+	security_inode_getsecid(inode, &name->osid);
 }
 
 /**
@@ -2187,8 +2187,7 @@ int __audit_ipc_obj(struct kern_ipc_perm *ipcp)
 	ax->uid = ipcp->uid;
 	ax->gid = ipcp->gid;
 	ax->mode = ipcp->mode;
-	selinux_get_ipc_sid(ipcp, &ax->osid);
-
+	security_ipc_getsecid(ipcp, &ax->osid);
 	ax->d.type = AUDIT_IPC;
 	ax->d.next = context->aux;
 	context->aux = (void *)ax;
@@ -2340,7 +2339,7 @@ void __audit_ptrace(struct task_struct *t)
 	context->target_auid = audit_get_loginuid(t);
 	context->target_uid = t->uid;
 	context->target_sessionid = audit_get_sessionid(t);
-	selinux_get_task_sid(t, &context->target_sid);
+	security_task_getsecid(t, &context->target_sid);
 	memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
 }
 
@@ -2368,7 +2367,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
 				audit_sig_uid = tsk->loginuid;
 			else
 				audit_sig_uid = tsk->uid;
-			selinux_get_task_sid(tsk, &audit_sig_sid);
+			security_task_getsecid(tsk, &audit_sig_sid);
 		}
 		if (!audit_signals || audit_dummy_context())
 			return 0;
@@ -2381,7 +2380,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
 		ctx->target_auid = audit_get_loginuid(t);
 		ctx->target_uid = t->uid;
 		ctx->target_sessionid = audit_get_sessionid(t);
-		selinux_get_task_sid(t, &ctx->target_sid);
+		security_task_getsecid(t, &ctx->target_sid);
 		memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
 		return 0;
 	}
@@ -2402,7 +2401,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
 	axp->target_auid[axp->pid_count] = audit_get_loginuid(t);
 	axp->target_uid[axp->pid_count] = t->uid;
 	axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
-	selinux_get_task_sid(t, &axp->target_sid[axp->pid_count]);
+	security_task_getsecid(t, &axp->target_sid[axp->pid_count]);
 	memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
 	axp->pid_count++;
 
@@ -2432,16 +2431,16 @@ void audit_core_dumps(long signr)
 	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
 	audit_log_format(ab, "auid=%u uid=%u gid=%u ses=%u",
 			auid, current->uid, current->gid, sessionid);
-	selinux_get_task_sid(current, &sid);
+	security_task_getsecid(current, &sid);
 	if (sid) {
 		char *ctx = NULL;
 		u32 len;
 
-		if (selinux_sid_to_string(sid, &ctx, &len))
+		if (security_secid_to_secctx(sid, &ctx, &len))
 			audit_log_format(ab, " ssid=%u", sid);
 		else
 			audit_log_format(ab, " subj=%s", ctx);
-		kfree(ctx);
+		security_release_secctx(ctx, len);
 	}
 	audit_log_format(ab, " pid=%d comm=", current->pid);
 	audit_log_untrustedstring(ab, current->comm);


-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

[PATCH -mm 4/4] Netlink: Use LSM interface instead of SELinux one

[Ahmed S. Darwish]

Don't use SELinux exported selinux_get_task_sid symbol. 
Use the generic LSM equivalent instead.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 1ab0da2..61fd277 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -54,7 +54,6 @@
 #include <linux/mm.h>
 #include <linux/types.h>
 #include <linux/audit.h>
-#include <linux/selinux.h>
 #include <linux/mutex.h>
 
 #include <net/net_namespace.h>
@@ -1239,7 +1238,7 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
 	NETLINK_CB(skb).pid	= nlk->pid;
 	NETLINK_CB(skb).dst_group = dst_group;
 	NETLINK_CB(skb).loginuid = audit_get_loginuid(current);
-	selinux_get_task_sid(current, &(NETLINK_CB(skb).sid));
+	security_task_getsecid(current, &(NETLINK_CB(skb).sid));
 	memcpy(NETLINK_CREDS(skb), &siocb->scm->creds, sizeof(struct ucred));
 
 	/* What can I do? Netlink is asynchronous, so that

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH -mm 2/4] SELinux: Remove various exported symbols

[Paul Moore]

On Tuesday 26 February 2008 6:25:41 pm Ahmed S. Darwish wrote:
> Remove the following exported SELinux interfaces:
> selinux_get_inode_sid(inode, sid)
> selinux_get_ipc_sid(ipcp, sid)
> selinux_get_task_sid(tsk, sid)
> selinux_sid_to_string(sid, ctx, len)
>
> and substitue them with following equivalents respectively:
> new LSM hook, inode_getsecid(inode, secid)
> new LSM hook, ipc_getsecid*(ipcp, secid)
> LSM hook, task_getsecid(tsk, secid)
> LSM hook, sid_to_secctx(sid, ctx, len)
>
> This is done to remove SELinux dependency from some
> of the kernel subsystems (audit).
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
> ---
>
>  include/linux/selinux.h    |   62
> ---------------------------------------------
> security/selinux/exports.c |   42 ------------------------------
> security/selinux/hooks.c   |   19 ++++++++++++-
>  3 files changed, 17 insertions(+), 106 deletions(-)

I haven't had a chance to look at the rest of the changes in detail yet, 
but this should be the last patch in the series.  The reason is that 
after applying this patch (and not the next two) the kernel will no 
longer compile meaning bisects will break which will cause people to 
get grumpy.

If you have to split things into multiple patches, it's a good idea to 
do it in this order:

 1. Add the new function
 2. Convert all the callers
 3. Remove the old function you replaced

-- 
paul moore
linux security @ hp

Re: [PATCH -mm 3/4] Audit: start not to use SELinux exported symbols

[Paul Moore]

On Tuesday 26 February 2008 6:28:08 pm Ahmed S. Darwish wrote:
> Remove the following exported SELinux interfaces:
> selinux_get_inode_sid(inode, sid)
> selinux_get_ipc_sid(ipcp, sid)
> selinux_get_task_sid(tsk, sid)
> selinux_sid_to_string(sid, ctx, len)
>
> and substitue them with following generic LSM equivalents
> respectively:
> security_inode_getsecid(inode, secid)
> security_ipc_getsecid*(ipcp, secid)
> security_task_getsecid(tsk, secid)
> security_sid_to_secctx(sid, ctx, len)
>
> Let the security_task_getsecid(tsk, secid) LSM hook set
> the secid to 0 by default if CONFIG_SECURITY is not defined
> or if the hook is set to NULL (dummy). This is done to
> notify the caller that no valid secid exists.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>

In the future, such patches should probably also be CC'd to the audit 
list.

> ---
>
>  include/linux/security.h |    5 ++++-
>  kernel/audit.c           |   14 +++++++-------
>  kernel/auditfilter.c     |    5 +++--
>  kernel/auditsc.c         |   37
> ++++++++++++++++++------------------- security/dummy.c         |    4
> +++-
>  6 files changed, 36 insertions(+), 32 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index a7fb136..35c98f0 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -621,6 +621,7 @@ struct request_sock;
>   * @task_getsecid:
>   *	Retrieve the security identifier of the process @p.
>   *	@p contains the task_struct for the process and place is into
> @secid.
> + *      In case of failure, @secid will be set to zero

This is a nit, but you used spaces between the "*" and "In case ..." 
where the rest of the files uses tabs.  When in doubt, try and stick 
with whatever formatting the rest of the source file uses.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 8316a88..2bd4124 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -259,13 +259,13 @@ static int audit_log_config_change(char
> *function_name, int new, int old, char *ctx = NULL;
>  		u32 len;
>
> -		rc = selinux_sid_to_string(sid, &ctx, &len);
> +		rc = security_secid_to_secctx(sid, &ctx, &len);
>  		if (rc) {
>  			audit_log_format(ab, " sid=%u", sid);
>  			allow_changes = 0; /* Something weird, deny request */
>  		} else {
>  			audit_log_format(ab, " subj=%s", ctx);
> -			kfree(ctx);
> +			security_release_secctx(ctx, len);
>  		}
>  	}
>  	audit_log_format(ab, " res=%d", allow_changes);
> @@ -543,12 +543,12 @@ static int audit_log_common_recv_msg(struct
> audit_buffer **ab, u16 msg_type, audit_log_format(*ab, "user pid=%d
> uid=%u auid=%u",
>  			 pid, uid, auid);
>  	if (sid) {
> -		rc = selinux_sid_to_string(sid, &ctx, &len);
> +		rc = security_secid_to_secctx(sid, &ctx, &len);
>  		if (rc)
>  			audit_log_format(*ab, " ssid=%u", sid);
>  		else
>  			audit_log_format(*ab, " subj=%s", ctx);
> -		kfree(ctx);
> +		security_release_secctx(ctx, len);
>  	}
>
>  	return rc;

This next part isn't your fault, but you're touching the code so I'm 
going to point it out and suggest you fix it while you are at it :)

If you look at how kfree()/security_release_secctx() is called in the 
above two functions you notice how it is inconsistent: it is only 
called on success in the first case, but called regardless in the 
second.  Make this consistent, preferably using the approach taken in 
the first case (only call it on success).

> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 2f2914b..894e3bd 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -28,6 +28,7 @@
>  #include <linux/netlink.h>
>  #include <linux/sched.h>
>  #include <linux/inotify.h>
> +#include <linux/security.h>
>  #include <linux/selinux.h>
>  #include "audit.h"
>
> @@ -1515,11 +1516,11 @@ static void audit_log_rule_change(uid_t
> loginuid, u32 sid, char *action, if (sid) {
>  		char *ctx = NULL;
>  		u32 len;
> -		if (selinux_sid_to_string(sid, &ctx, &len))
> +		if (security_secid_to_secctx(sid, &ctx, &len))
>  			audit_log_format(ab, " ssid=%u", sid);
>  		else
>  			audit_log_format(ab, " subj=%s", ctx);
> -		kfree(ctx);
> +		security_release_secctx(ctx, len);
>  	}
>  	audit_log_format(ab, " op=%s rule key=", action);
>  	if (rule->filterkey)

Same comment about security_release_secctx() applies here.

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d07fc4a..631c044 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -885,11 +885,11 @@ void audit_log_task_context(struct audit_buffer
> *ab) int error;
>  	u32 sid;
>
> -	selinux_get_task_sid(current, &sid);
> +	security_task_getsecid(current, &sid);
>  	if (!sid)
>  		return;
>
> -	error = selinux_sid_to_string(sid, &ctx, &len);
> +	error = security_secid_to_secctx(sid, &ctx, &len);
>  	if (error) {
>  		if (error != -EINVAL)
>  			goto error_path;
> @@ -897,7 +897,7 @@ void audit_log_task_context(struct audit_buffer
> *ab) }
>
>  	audit_log_format(ab, " subj=%s", ctx);
> -	kfree(ctx);
> +	security_release_secctx(ctx, len);
>  	return;

Same thing.

>  error_path:
> @@ -951,7 +951,7 @@ static int audit_log_pid_context(struct
> audit_context *context, pid_t pid,
>
>  	audit_log_format(ab, "opid=%d oauid=%d ouid=%d oses=%d", pid, auid,
>  			 uid, sessionid);
> -	if (selinux_sid_to_string(sid, &s, &len)) {
> +	if (security_secid_to_secctx(sid, &s, &len)) {
>  		audit_log_format(ab, " obj=(none)");
>  		rc = 1;
>  	} else

You forgot to convert the matching kfree() call to a 
security_release_secctx() call.  Also it might be nice to change the 
context variable name from "s" to "ctx", but some ornery folks might 
whine about that ...

> @@ -1268,14 +1268,14 @@ static void audit_log_exit(struct
> audit_context *context, struct task_struct *ts if (axi->osid != 0) {
>  				char *ctx = NULL;
>  				u32 len;
> -				if (selinux_sid_to_string(
> +				if (security_secid_to_secctx(
>  						axi->osid, &ctx, &len)) {
>  					audit_log_format(ab, " osid=%u",
>  							axi->osid);
>  					call_panic = 1;
>  				} else
>  					audit_log_format(ab, " obj=%s", ctx);
> -				kfree(ctx);
> +				security_release_secctx(ctx, len);
>  			}
>  			break; }

Only call the release routine on success.

> @@ -1389,13 +1389,13 @@ static void audit_log_exit(struct
> audit_context *context, struct task_struct *ts if (n->osid != 0) {
>  			char *ctx = NULL;
>  			u32 len;
> -			if (selinux_sid_to_string(
> +			if (security_secid_to_secctx(
>  				n->osid, &ctx, &len)) {
>  				audit_log_format(ab, " osid=%u", n->osid);
>  				call_panic = 2;
>  			} else
>  				audit_log_format(ab, " obj=%s", ctx);
> -			kfree(ctx);
> +			security_release_secctx(ctx, len);
>  		}

Here too.

> @@ -2432,16 +2431,16 @@ void audit_core_dumps(long signr)
>  	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
>  	audit_log_format(ab, "auid=%u uid=%u gid=%u ses=%u",
>  			auid, current->uid, current->gid, sessionid);
> -	selinux_get_task_sid(current, &sid);
> +	security_task_getsecid(current, &sid);
>  	if (sid) {
>  		char *ctx = NULL;
>  		u32 len;
>
> -		if (selinux_sid_to_string(sid, &ctx, &len))
> +		if (security_secid_to_secctx(sid, &ctx, &len))
>  			audit_log_format(ab, " ssid=%u", sid);
>  		else
>  			audit_log_format(ab, " subj=%s", ctx);
> -		kfree(ctx);
> +		security_release_secctx(ctx, len);
>  	}
>  	audit_log_format(ab, " pid=%d comm=", current->pid);
>  	audit_log_untrustedstring(ab, current->comm);

One last time.

-- 
paul moore
linux security @ hp

Re: [PATCH -mm 1/4] LSM: Introduce inode_getsecid and ipc_getsecid hooks

[Paul Moore]

On Tuesday 26 February 2008 6:24:11 pm Ahmed S. Darwish wrote:
> Introduce inode_getsecid(inode, secid) and ipc_getsecid(ipcp, secid)
> LSM hooks.
>
> This hooks will be used instead of similar exported SELinux
> interfaces.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
> ---
>
>  include/linux/security.h |   18 ++++++++++++++++++
>  security/dummy.c         |   12 ++++++++++++
>  security/security.c      |   12 ++++++++++++
>  3 files changed, 42 insertions(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index a33fd03..a7fb136 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -449,6 +449,10 @@ struct request_sock;
>   *	@dentry is the dentry being changed.
>   *	Return 0 on success.  If error is returned, then the operation
>   *	causing setuid bit removal is failed.
> + * @inode_getsecid:
> + *      Get the secid associated with the node
> + *      @inode contains a pointer to the inode
> + *      @secid contains a pointer to the location to store the
> result *
>   * Security hooks for file operations
>   *
> @@ -989,6 +993,10 @@ struct request_sock;
>   *	@ipcp contains the kernel IPC permission structure
>   *	@flag contains the desired (requested) permission set
>   *	Return 0 if permission is granted.
> + * @ipc_getsecid:
> + *      Get the secid associated with the ipc object
> + *      @ipcp contains the kernel IPC permission structure
> + *      @secid contains a pointer to the location where result will
> be saved

See my other comments about tabs not spaces.

> +static inline void security_inode_getsecid(const struct inode
> *inode, u32 *secid)
> +{ } 

...

> +static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp,
> u32 *secid)
> +{ } 

Should these both do a  "*secid = 0;"?

> diff --git a/security/dummy.c b/security/dummy.c
> index 6a0056b..c2f4c52 100644
> --- a/security/dummy.c
> +++ b/security/dummy.c
> @@ -422,6 +422,11 @@ static int dummy_inode_listsecurity(struct inode
> *inode, char *buffer, size_t bu return 0;
>  }
>
> +static void dummy_inode_getsecid(const struct inode *inode, u32
> *secid)
> +{ 
> +	return;
> +}

...

> +static void dummy_ipc_getsecid(struct kern_ipc_perm *ipcp, u32
> *secid)
> +{ 
> +	return;
> +}

Same question.

-- 
paul moore
linux security @ hp

Re: [PATCH -mm 1/4] LSM: Introduce inode_getsecid and ipc_getsecid hooks

[Ahmed S. Darwish]

Hi Paul,

On Wed, Feb 27, 2008 at 11:04:57AM -0500, Paul Moore wrote:
> On Tuesday 26 February 2008 6:24:11 pm Ahmed S. Darwish wrote:
> > Introduce inode_getsecid(inode, secid) and ipc_getsecid(ipcp, secid)
> > LSM hooks.
> >
> > This hooks will be used instead of similar exported SELinux
> > interfaces.
> >
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
> > ---
> >
> >  include/linux/security.h |   18 ++++++++++++++++++
> >  security/dummy.c         |   12 ++++++++++++
> >  security/security.c      |   12 ++++++++++++
> >  3 files changed, 42 insertions(+)
> >
...
> ...
> 
> > +static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp,
> > u32 *secid)
> > +{ } 
> 
> Should these both do a  "*secid = 0;"?
> 

Yes, this will also lead to consistency in the interface espcially 
after changing task_getsecid to do the same (in patch #3).

> > diff --git a/security/dummy.c b/security/dummy.c
> > index 6a0056b..c2f4c52 100644
> > --- a/security/dummy.c
> > +++ b/security/dummy.c
> > @@ -422,6 +422,11 @@ static int dummy_inode_listsecurity(struct inode
> > *inode, char *buffer, size_t bu return 0;
> >  }
> >
> > +static void dummy_inode_getsecid(const struct inode *inode, u32
> > *secid)
> > +{ 
> > +	return;
> > +}
> 
> ...
> 
> > +static void dummy_ipc_getsecid(struct kern_ipc_perm *ipcp, u32
> > *secid)
> > +{ 
> > +	return;
> > +}
> 
> Same question.
> 

Both will be changed to *secid = 0 in the comming send. Thanks!

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH -mm 3/4] Audit: start not to use SELinux exported symbols

[Ahmed S. Darwish]

Hi!,

On Wed, Feb 27, 2008 at 11:00:57AM -0500, Paul Moore wrote:
> On Tuesday 26 February 2008 6:28:08 pm Ahmed S. Darwish wrote:
...
> >
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
> 
> In the future, such patches should probably also be CC'd to the audit 
> list.
> 

No problem.

> > ---
> >
> >  include/linux/security.h |    5 ++++-
> >  kernel/audit.c           |   14 +++++++-------
> >  kernel/auditfilter.c     |    5 +++--
> >  kernel/auditsc.c         |   37
> > ++++++++++++++++++------------------- security/dummy.c         |    4
> > +++-
> >  6 files changed, 36 insertions(+), 32 deletions(-)
> >
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index a7fb136..35c98f0 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -621,6 +621,7 @@ struct request_sock;
> >   * @task_getsecid:
> >   *	Retrieve the security identifier of the process @p.
> >   *	@p contains the task_struct for the process and place is into
> > @secid.
> > + *      In case of failure, @secid will be set to zero
> 
> This is a nit, but you used spaces between the "*" and "In case ..." 
> where the rest of the files uses tabs.  When in doubt, try and stick 
> with whatever formatting the rest of the source file uses.
> 

I'll find a way to teach emacs how to print a tab ;). Will do.

> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 8316a88..2bd4124 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -259,13 +259,13 @@ static int audit_log_config_change(char
> > *function_name, int new, int old, char *ctx = NULL;
> >  		u32 len;
> >
> > -		rc = selinux_sid_to_string(sid, &ctx, &len);
> > +		rc = security_secid_to_secctx(sid, &ctx, &len);
> >  		if (rc) {
> >  			audit_log_format(ab, " sid=%u", sid);
> >  			allow_changes = 0; /* Something weird, deny request */
> >  		} else {
> >  			audit_log_format(ab, " subj=%s", ctx);
> > -			kfree(ctx);
> > +			security_release_secctx(ctx, len);
> >  		}
> >  	}
> >  	audit_log_format(ab, " res=%d", allow_changes);
> > @@ -543,12 +543,12 @@ static int audit_log_common_recv_msg(struct
> > audit_buffer **ab, u16 msg_type, audit_log_format(*ab, "user pid=%d
> > uid=%u auid=%u",
> >  			 pid, uid, auid);
> >  	if (sid) {
> > -		rc = selinux_sid_to_string(sid, &ctx, &len);
> > +		rc = security_secid_to_secctx(sid, &ctx, &len);
> >  		if (rc)
> >  			audit_log_format(*ab, " ssid=%u", sid);
> >  		else
> >  			audit_log_format(*ab, " subj=%s", ctx);
> > -		kfree(ctx);
> > +		security_release_secctx(ctx, len);
> >  	}
> >
> >  	return rc;
> 
> This next part isn't your fault, but you're touching the code so I'm 
> going to point it out and suggest you fix it while you are at it :)
> 
> If you look at how kfree()/security_release_secctx() is called in the 
> above two functions you notice how it is inconsistent: it is only 
> called on success in the first case, but called regardless in the 
> second.  Make this consistent, preferably using the approach taken in 
> the first case (only call it on success).
> 

My pleasure. You'll find them consistent in the next send.

[Same release_ctx() consistency point]
...

> 
> > @@ -951,7 +951,7 @@ static int audit_log_pid_context(struct
> > audit_context *context, pid_t pid,
> >
> >  	audit_log_format(ab, "opid=%d oauid=%d ouid=%d oses=%d", pid, auid,
> >  			 uid, sessionid);
> > -	if (selinux_sid_to_string(sid, &s, &len)) {
> > +	if (security_secid_to_secctx(sid, &s, &len)) {
> >  		audit_log_format(ab, " obj=(none)");
> >  		rc = 1;
> >  	} else
> 
> You forgot to convert the matching kfree() call to a 
> security_release_secctx() call.  Also it might be nice to change the 
> context variable name from "s" to "ctx", but some ornery folks might 
> whine about that ...
> 

Nice spot, will do. 's' will be changed too, context is already
used as 'ctx' in the whole file anyway.

[...]
> 
> Only call the release routine on success.
> 
[...]
> 
> Here too.
> 
[...]
> 
> One last time.
> 

Thanks a lot for such a deep review :). I'll modify spotted problems
as suggested, reorder the patch in a non compilation-breaking order, 
and keep you informed.

Modified patches will be sent with the new ones that will complete 
the separation not to add too much noise in LKML.

Warm regards

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH -mm 3/4] Audit: start not to use SELinux exported symbols

[James Morris]

On Wed, 27 Feb 2008, Ahmed S. Darwish wrote:

> Thanks a lot for such a deep review :). I'll modify spotted problems
> as suggested, reorder the patch in a non compilation-breaking order, 
> and keep you informed.
> 
> Modified patches will be sent with the new ones that will complete 
> the separation not to add too much noise in LKML.

I'm not quite sure what this means, but you should resend the whole 
patchset.


- James
-- 
James Morris 
<jmorris@namei.org>