[PATCH 1/6] secmark: do not return early if there was no error

[PATCH 1/6] secmark: do not return early if there was no error

[Eric Paris]

Commit 4a5a5c73 attempted to pass decent error messages back to userspace for
netfilter errors.  In xt_SECMARK.c however the patch screwed up and returned
on 0 (aka no error) early and didn't finish setting up secmark.  This results
in a kernel BUG if you use SECMARK.

------------[ cut here ]------------
kernel BUG at net/netfilter/xt_SECMARK.c:38!
invalid opcode: 0000 [#1] SMP
last sysfs file: /sys/devices/system/cpu/cpu2/cache/index2/shared_cpu_map
CPU 0
Modules linked in: xt_SECMARK iptable_mangle nfs lockd fscache nfs_acl
auth_rpcgss sunrpc ip6t_REJECT nf_conntrack_ipv6 ip6table_filter ip6_tables
uinput virtio_net virtio_balloon i2c_piix4 i2c_core joydev microcode ipv6
virtio_blk virtio_pci virtio_ring virtio [last unloaded: speedstep_lib]

Pid: 0, comm: swapper Not tainted 2.6.36-0.8.rc2.git0.fc15.x86_64 #1 /KVM
RIP: 0010:[<ffffffffa022117d>]  [<ffffffffa022117d>] secmark_tg+0x17/0x2e [xt_SECMARK]
RSP: 0018:ffff880003e03a40  EFLAGS: 00010202
RAX: ffff88001f3074b0 RBX: ffff88001f3073f0 RCX: ffff88001f307490
RDX: ffff88001f307401 RSI: ffff880003e03b30 RDI: ffff88001f18e500
RBP: ffff880003e03a40 R08: 0000000000000002 R09: ffff880003e03a10
R10: ffff880003fd2ad8 R11: ffffffff00000001 R12: ffff88001a85d498
R13: ffffe8ffff808240 R14: ffff88001ac133ae R15: ffff88001f18e500
FS:  0000000000000000(0000) GS:ffff880003e00000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 000000000073b130 CR3: 000000000fdc0000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 0, threadinfo ffffffff81a00000, task
ffffffff81a4b020)
Stack:
ffff880003e03b90 ffffffff814599ff 0000000000003a18 0000000000000000
ffff880003e03b70 ffffffffffffffb8 0000000000000000 ffffffff82a39d60
ffff880003e03a90 ffffffff8140db60 ffff880003e03ae0 ffffffff8140f2c0
Call Trace:
 <IRQ>
[<ffffffff814599ff>] ipt_do_table+0x58a/0x6e2
[<ffffffff8140db60>] ? rcu_read_unlock+0x21/0x23
[<ffffffff8140f2c0>] ? nf_conntrack_find_get+0xb4/0xc7
[<ffffffffa021b182>] iptable_mangle_hook+0x10a/0x120 [iptable_mangle]
[<ffffffff8140c226>] nf_iterate+0x46/0x89
[<ffffffff8141d2e8>] ? ip_rcv_finish+0x0/0x3c6
[<ffffffff8140c2e1>] nf_hook_slow+0x78/0xe3
[<ffffffff8141d2e8>] ? ip_rcv_finish+0x0/0x3c6
[<ffffffff81472f06>] ? run_filter+0x0/0xc0
[<ffffffff813e6802>] ? dev_seq_stop+0x8/0x10
[<ffffffff8141d2e8>] ? ip_rcv_finish+0x0/0x3c6
[<ffffffff8141d9a9>] NF_HOOK.clone.6+0x46/0x58
[<ffffffff8141dd93>] ip_rcv+0x21f/0x24c
[<ffffffff813e7d43>] __netif_receive_skb+0x3e0/0x40a
[<ffffffff813e8834>] netif_receive_skb+0x6c/0x73
[<ffffffffa00c954e>] virtnet_poll+0x55b/0x6cb [virtio_net]
[<ffffffff8107fb92>] ? lock_release+0x19a/0x1a6
[<ffffffff813e9bc4>] net_rx_action+0xb1/0x1e3
[<ffffffff8107d64b>] ? print_lock_contention_bug+0x1b/0xd5
[<ffffffff8100ac1c>] ? call_softirq+0x1c/0x30
[<ffffffff8105752a>] __do_softirq+0xfa/0x1cf
[<ffffffff8107fb92>] ? lock_release+0x19a/0x1a6
[<ffffffff8100ac1c>] call_softirq+0x1c/0x30
[<ffffffff8100c3d9>] do_softirq+0x4b/0xa2
[<ffffffff810576d0>] irq_exit+0x4a/0x8c
[<ffffffff814a198d>] do_IRQ+0x9d/0xb4
[<ffffffff8149b813>] ret_from_intr+0x0/0x16
 <EOI>
[<ffffffff81010faf>] ? default_idle+0x3c/0x61
[<ffffffff8102c7b1>] ? native_safe_halt+0xb/0xd
[<ffffffff810800c0>] ? trace_hardirqs_on+0xd/0xf
[<ffffffff81010fb4>] default_idle+0x41/0x61
[<ffffffff8100830b>] cpu_idle+0xb3/0x10f
[<ffffffff814824c3>] rest_init+0xb7/0xbe
[<ffffffff8148240c>] ? rest_init+0x0/0xbe
[<ffffffff81d76c50>] start_kernel+0x412/0x41d
[<ffffffff81d762c6>] x86_64_start_reservations+0xb1/0xb5
[<ffffffff81d763c2>] x86_64_start_kernel+0xf8/0x107
Code: 41 8a 04 24 88 05 1c 05 00 00 5a 89 d8 5b 41 5c 41 5d c9 c3 55 48 89 e5
0f 1f 44 00 00 48 8b 46 08 8a 10 3a 15 fd 04 00 00 74 02 <0f> 0b fe ca 75 0e
8b 40 04 89 87 b4 00 00 00 83 c8 ff c9 c3 0f
RIP  [<ffffffffa022117d>] secmark_tg+0x17/0x2e [xt_SECMARK]
RSP <ffff880003e03a40>
---[ end trace 9aa5d06a71143e74 ]---

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 net/netfilter/xt_SECMARK.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
index 23b2d6c..364ad16 100644
--- a/net/netfilter/xt_SECMARK.c
+++ b/net/netfilter/xt_SECMARK.c
@@ -101,7 +101,7 @@ static int secmark_tg_check(const struct xt_tgchk_param *par)
 	switch (info->mode) {
 	case SECMARK_MODE_SEL:
 		err = checkentry_selinux(info);
-		if (err <= 0)
+		if (err)
 			return err;
 		break;
 

[PATCH 2/6] secmark: make secmark object handling generic

[Eric Paris]

Right now secmark has lots of direct selinux calls.  Use all LSM calls and
remove all SELinux specific knowledge.  The only SELinux specific knowledge
we leave is the mode.  The only point is to make sure that other LSMs at
least test this generic code before they assume it works.  (They may also
have to make changes if they do not represent labels as strings)

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 include/linux/netfilter/xt_SECMARK.h |   14 ++------
 include/linux/security.h             |   25 +++++++++++++
 include/linux/selinux.h              |   63 ----------------------------------
 net/netfilter/xt_CT.c                |    1 -
 net/netfilter/xt_SECMARK.c           |   57 +++++++++++++++----------------
 security/capability.c                |   17 +++++++++
 security/security.c                  |   18 ++++++++++
 security/selinux/exports.c           |   49 --------------------------
 security/selinux/hooks.c             |   24 +++++++++++++
 security/selinux/include/security.h  |    1 +
 10 files changed, 116 insertions(+), 153 deletions(-)

diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
index 6fcd344..b8d55c4 100644
--- a/include/linux/netfilter/xt_SECMARK.h
+++ b/include/linux/netfilter/xt_SECMARK.h
@@ -10,19 +10,13 @@
  * 'mode' refers to the specific security subsystem which the
  * packets are being marked for.
  */
-#define SECMARK_MODE_SEL	0x01		/* SELinux */
-#define SECMARK_SELCTX_MAX	256
-
-struct xt_secmark_target_selinux_info {
-	__u32 selsid;
-	char selctx[SECMARK_SELCTX_MAX];
-};
+#define SECMARK_MODE_SELINUX	0x01		/* SELinux */
+#define SECMARK_SECCTX_MAX	256
 
 struct xt_secmark_target_info {
 	__u8 mode;
-	union {
-		struct xt_secmark_target_selinux_info sel;
-	} u;
+	__u32 secid;
+	char secctx[SECMARK_SECCTX_MAX];
 };
 
 #endif /*_XT_SECMARK_H_target */
diff --git a/include/linux/security.h b/include/linux/security.h
index c2f0cdb..2f2e181 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -960,6 +960,12 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	Sets the new child socket's sid to the openreq sid.
  * @inet_conn_established:
  *	Sets the connection's peersid to the secmark on skb.
+ * @secmark_relabel_packet:
+ *	check if the process should be allowed to relabel packets to the given secid
+ * @security_secmark_refcount_inc
+ *	tells the LSM to increment the number of secmark labeling rules loaded
+ * @security_secmark_refcount_dec
+ *	tells the LSM to decrement the number of secmark labeling rules loaded
  * @req_classify_flow:
  *	Sets the flow's sid to the openreq sid.
  * @tun_dev_create:
@@ -1597,6 +1603,9 @@ struct security_operations {
 				  struct request_sock *req);
 	void (*inet_csk_clone) (struct sock *newsk, const struct request_sock *req);
 	void (*inet_conn_established) (struct sock *sk, struct sk_buff *skb);
+	int (*secmark_relabel_packet) (u32 secid);
+	void (*secmark_refcount_inc) (void);
+	void (*secmark_refcount_dec) (void);
 	void (*req_classify_flow) (const struct request_sock *req, struct flowi *fl);
 	int (*tun_dev_create)(void);
 	void (*tun_dev_post_create)(struct sock *sk);
@@ -2560,6 +2569,9 @@ void security_inet_csk_clone(struct sock *newsk,
 			const struct request_sock *req);
 void security_inet_conn_established(struct sock *sk,
 			struct sk_buff *skb);
+int security_secmark_relabel_packet(u32 secid);
+void security_secmark_refcount_inc(void);
+void security_secmark_refcount_dec(void);
 int security_tun_dev_create(void);
 void security_tun_dev_post_create(struct sock *sk);
 int security_tun_dev_attach(struct sock *sk);
@@ -2714,6 +2726,19 @@ static inline void security_inet_conn_established(struct sock *sk,
 {
 }
 
+static inline int security_secmark_relabel_packet(u32 secid)
+{
+	return 0;
+}
+
+static inline void security_secmark_refcount_inc(void)
+{
+}
+
+static inline void security_secmark_refcount_dec(void)
+{
+}
+
 static inline int security_tun_dev_create(void)
 {
 	return 0;
diff --git a/include/linux/selinux.h b/include/linux/selinux.h
index 82e0f26..44f4596 100644
--- a/include/linux/selinux.h
+++ b/include/linux/selinux.h
@@ -21,74 +21,11 @@ struct kern_ipc_perm;
 #ifdef CONFIG_SECURITY_SELINUX
 
 /**
- *     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.
- *
- *     Returns 0 if successful, with the SID stored in sid.  A value
- *     of zero for sid indicates no SID could be determined (but no error
- *     occurred).
- */
-int selinux_string_to_sid(char *str, u32 *sid);
-
-/**
- *     selinux_secmark_relabel_packet_permission - secmark permission check
- *     @sid: SECMARK ID value to be applied to network packet
- *
- *     Returns 0 if the current task is allowed to set the SECMARK label of
- *     packets with the supplied security ID.  Note that it is implicit that
- *     the packet is always being relabeled from the default unlabeled value,
- *     and that the access control decision is made in the AVC.
- */
-int selinux_secmark_relabel_packet_permission(u32 sid);
-
-/**
- *     selinux_secmark_refcount_inc - increments the secmark use counter
- *
- *     SELinux keeps track of the current SECMARK targets in use so it knows
- *     when to apply SECMARK label access checks to network packets.  This
- *     function incements this reference count to indicate that a new SECMARK
- *     target has been configured.
- */
-void selinux_secmark_refcount_inc(void);
-
-/**
- *     selinux_secmark_refcount_dec - decrements the secmark use counter
- *
- *     SELinux keeps track of the current SECMARK targets in use so it knows
- *     when to apply SECMARK label access checks to network packets.  This
- *     function decements this reference count to indicate that one of the
- *     existing SECMARK targets has been removed/flushed.
- */
-void selinux_secmark_refcount_dec(void);
-
-/**
  * selinux_is_enabled - is SELinux enabled?
  */
 bool selinux_is_enabled(void);
 #else
 
-static inline int selinux_string_to_sid(const char *str, u32 *sid)
-{
-       *sid = 0;
-       return 0;
-}
-
-static inline int selinux_secmark_relabel_packet_permission(u32 sid)
-{
-	return 0;
-}
-
-static inline void selinux_secmark_refcount_inc(void)
-{
-	return;
-}
-
-static inline void selinux_secmark_refcount_dec(void)
-{
-	return;
-}
-
 static inline bool selinux_is_enabled(void)
 {
 	return false;
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 0cb6053..782e519 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -9,7 +9,6 @@
 #include <linux/module.h>
 #include <linux/gfp.h>
 #include <linux/skbuff.h>
-#include <linux/selinux.h>
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv6/ip6_tables.h>
 #include <linux/netfilter/x_tables.h>
diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
index 364ad16..fa775b7 100644
--- a/net/netfilter/xt_SECMARK.c
+++ b/net/netfilter/xt_SECMARK.c
@@ -14,8 +14,8 @@
  */
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 #include <linux/module.h>
+#include <linux/security.h>
 #include <linux/skbuff.h>
-#include <linux/selinux.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_SECMARK.h>
 
@@ -38,10 +38,9 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
 	BUG_ON(info->mode != mode);
 
 	switch (mode) {
-	case SECMARK_MODE_SEL:
-		secmark = info->u.sel.selsid;
+	case SECMARK_MODE_SELINUX:
+		secmark = info->secid;
 		break;
-
 	default:
 		BUG();
 	}
@@ -50,33 +49,33 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
 	return XT_CONTINUE;
 }
 
-static int checkentry_selinux(struct xt_secmark_target_info *info)
+static int checkentry_lsm(struct xt_secmark_target_info *info)
 {
 	int err;
-	struct xt_secmark_target_selinux_info *sel = &info->u.sel;
 
-	sel->selctx[SECMARK_SELCTX_MAX - 1] = '\0';
+	info->secctx[SECMARK_SECCTX_MAX - 1] = '\0';
+	info->secid = 0;
 
-	err = selinux_string_to_sid(sel->selctx, &sel->selsid);
+	err = security_secctx_to_secid(info->secctx, strlen(info->secctx),
+				       &info->secid);
 	if (err) {
 		if (err == -EINVAL)
-			pr_info("invalid SELinux context \'%s\'\n",
-				sel->selctx);
+			pr_info("invalid security context \'%s\'\n", info->secctx);
 		return err;
 	}
 
-	if (!sel->selsid) {
-		pr_info("unable to map SELinux context \'%s\'\n", sel->selctx);
+	if (!info->secid) {
+		pr_info("unable to map security context \'%s\'\n", info->secctx);
 		return -ENOENT;
 	}
 
-	err = selinux_secmark_relabel_packet_permission(sel->selsid);
+	err = security_secmark_relabel_packet(info->secid);
 	if (err) {
 		pr_info("unable to obtain relabeling permission\n");
 		return err;
 	}
 
-	selinux_secmark_refcount_inc();
+	security_secmark_refcount_inc();
 	return 0;
 }
 
@@ -99,17 +98,17 @@ static int secmark_tg_check(const struct xt_tgchk_param *par)
 	}
 
 	switch (info->mode) {
-	case SECMARK_MODE_SEL:
-		err = checkentry_selinux(info);
-		if (err)
-			return err;
+	case SECMARK_MODE_SELINUX:
 		break;
-
 	default:
 		pr_info("invalid mode: %hu\n", info->mode);
 		return -EINVAL;
 	}
 
+	err = checkentry_lsm(info);
+	if (err)
+		return err;
+
 	if (!mode)
 		mode = info->mode;
 	return 0;
@@ -118,20 +117,20 @@ static int secmark_tg_check(const struct xt_tgchk_param *par)
 static void secmark_tg_destroy(const struct xt_tgdtor_param *par)
 {
 	switch (mode) {
-	case SECMARK_MODE_SEL:
-		selinux_secmark_refcount_dec();
+	case SECMARK_MODE_SELINUX:
+		security_secmark_refcount_dec();
 	}
 }
 
 static struct xt_target secmark_tg_reg __read_mostly = {
-	.name       = "SECMARK",
-	.revision   = 0,
-	.family     = NFPROTO_UNSPEC,
-	.checkentry = secmark_tg_check,
-	.destroy    = secmark_tg_destroy,
-	.target     = secmark_tg,
-	.targetsize = sizeof(struct xt_secmark_target_info),
-	.me         = THIS_MODULE,
+	.name		= "SECMARK",
+	.revision	= 0,
+	.family		= NFPROTO_UNSPEC,
+	.checkentry	= secmark_tg_check,
+	.destroy	= secmark_tg_destroy,
+	.target		= secmark_tg,
+	.targetsize	= sizeof(struct xt_secmark_target_info),
+	.me		= THIS_MODULE,
 };
 
 static int __init secmark_tg_init(void)
diff --git a/security/capability.c b/security/capability.c
index 1e26299..806061b 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -681,7 +681,18 @@ static void cap_inet_conn_established(struct sock *sk, struct sk_buff *skb)
 {
 }
 
+static int cap_secmark_relabel_packet(u32 secid)
+{
+	return 0;
+}
 
+static void cap_secmark_refcount_inc(void)
+{
+}
+
+static void cap_secmark_refcount_dec(void)
+{
+}
 
 static void cap_req_classify_flow(const struct request_sock *req,
 				  struct flowi *fl)
@@ -781,7 +792,8 @@ static int cap_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
 
 static int cap_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
 {
-	return -EOPNOTSUPP;
+	*secid = 0;
+	return 0;
 }
 
 static void cap_release_secctx(char *secdata, u32 seclen)
@@ -1023,6 +1035,9 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, inet_conn_request);
 	set_to_cap_if_null(ops, inet_csk_clone);
 	set_to_cap_if_null(ops, inet_conn_established);
+	set_to_cap_if_null(ops, secmark_relabel_packet);
+	set_to_cap_if_null(ops, secmark_refcount_inc);
+	set_to_cap_if_null(ops, secmark_refcount_dec);
 	set_to_cap_if_null(ops, req_classify_flow);
 	set_to_cap_if_null(ops, tun_dev_create);
 	set_to_cap_if_null(ops, tun_dev_post_create);
diff --git a/security/security.c b/security/security.c
index 1d77d8b..1975b2e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1144,6 +1144,24 @@ void security_inet_conn_established(struct sock *sk,
 	security_ops->inet_conn_established(sk, skb);
 }
 
+int security_secmark_relabel_packet(u32 secid)
+{
+	return security_ops->secmark_relabel_packet(secid);
+}
+EXPORT_SYMBOL(security_secmark_relabel_packet);
+
+void security_secmark_refcount_inc(void)
+{
+	security_ops->secmark_refcount_inc();
+}
+EXPORT_SYMBOL(security_secmark_refcount_inc);
+
+void security_secmark_refcount_dec(void)
+{
+	security_ops->secmark_refcount_dec();
+}
+EXPORT_SYMBOL(security_secmark_refcount_dec);
+
 int security_tun_dev_create(void)
 {
 	return security_ops->tun_dev_create();
diff --git a/security/selinux/exports.c b/security/selinux/exports.c
index c0a454a..9066438 100644
--- a/security/selinux/exports.c
+++ b/security/selinux/exports.c
@@ -11,58 +11,9 @@
  * it under the terms of the GNU General Public License version 2,
  * as published by the Free Software Foundation.
  */
-#include <linux/types.h>
-#include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/selinux.h>
-#include <linux/fs.h>
-#include <linux/ipc.h>
-#include <asm/atomic.h>
 
 #include "security.h"
-#include "objsec.h"
-
-/* SECMARK reference count */
-extern atomic_t selinux_secmark_refcount;
-
-int selinux_string_to_sid(char *str, u32 *sid)
-{
-	if (selinux_enabled)
-		return security_context_to_sid(str, strlen(str), sid);
-	else {
-		*sid = 0;
-		return 0;
-	}
-}
-EXPORT_SYMBOL_GPL(selinux_string_to_sid);
-
-int selinux_secmark_relabel_packet_permission(u32 sid)
-{
-	if (selinux_enabled) {
-		const struct task_security_struct *__tsec;
-		u32 tsid;
-
-		__tsec = current_security();
-		tsid = __tsec->sid;
-
-		return avc_has_perm(tsid, sid, SECCLASS_PACKET,
-				    PACKET__RELABELTO, NULL);
-	}
-	return 0;
-}
-EXPORT_SYMBOL_GPL(selinux_secmark_relabel_packet_permission);
-
-void selinux_secmark_refcount_inc(void)
-{
-	atomic_inc(&selinux_secmark_refcount);
-}
-EXPORT_SYMBOL_GPL(selinux_secmark_refcount_inc);
-
-void selinux_secmark_refcount_dec(void)
-{
-	atomic_dec(&selinux_secmark_refcount);
-}
-EXPORT_SYMBOL_GPL(selinux_secmark_refcount_dec);
 
 bool selinux_is_enabled(void)
 {
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index aaa2dd8..7cfcd19 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4290,6 +4290,27 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb)
 	selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid);
 }
 
+static int selinux_secmark_relabel_packet(u32 sid)
+{
+	const struct task_security_struct *__tsec;
+	u32 tsid;
+
+	__tsec = current_security();
+	tsid = __tsec->sid;
+
+	return avc_has_perm(tsid, sid, SECCLASS_PACKET, PACKET__RELABELTO, NULL);
+}
+
+static void selinux_secmark_refcount_inc(void)
+{
+	atomic_inc(&selinux_secmark_refcount);
+}
+
+static void selinux_secmark_refcount_dec(void)
+{
+	atomic_dec(&selinux_secmark_refcount);
+}
+
 static void selinux_req_classify_flow(const struct request_sock *req,
 				      struct flowi *fl)
 {
@@ -5545,6 +5566,9 @@ static struct security_operations selinux_ops = {
 	.inet_conn_request =		selinux_inet_conn_request,
 	.inet_csk_clone =		selinux_inet_csk_clone,
 	.inet_conn_established =	selinux_inet_conn_established,
+	.secmark_relabel_packet =	selinux_secmark_relabel_packet,
+	.secmark_refcount_inc =		selinux_secmark_refcount_inc,
+	.secmark_refcount_dec =		selinux_secmark_refcount_dec,
 	.req_classify_flow =		selinux_req_classify_flow,
 	.tun_dev_create =		selinux_tun_dev_create,
 	.tun_dev_post_create = 		selinux_tun_dev_post_create,
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 9df4477..a71e224 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -9,6 +9,7 @@
 #define _SELINUX_SECURITY_H_
 
 #include <linux/magic.h>
+#include <linux/types.h>
 #include "flask.h"
 
 #define SECSID_NULL			0x00000000 /* unspecified SID */

[PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Eric Paris]

Currently the nfconntrack export code sends the kernel internal secid to
userspace in a couple of proc files and over netlink as an integer.  This
is wrong.  This number is a kernel internal.  This patch changes the export
code to output either 0 or 1 for this value.  A future patch will implement
sending the name rather than the number in a new field.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 .../netfilter/nf_conntrack_l3proto_ipv4_compat.c   |    2 +-
 net/netfilter/nf_conntrack_netlink.c               |    2 +-
 net/netfilter/nf_conntrack_standalone.c            |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
index 244f7cb..053d7d3 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
@@ -149,7 +149,7 @@ static int ct_seq_show(struct seq_file *s, void *v)
 #endif
 
 #ifdef CONFIG_NF_CONNTRACK_SECMARK
-	if (seq_printf(s, "secmark=%u ", ct->secmark))
+	if (seq_printf(s, "secmark=%u ", ct->secmark ? 1 : 0))
 		goto release;
 #endif
 
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 5bae1cd..3a50699 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -247,7 +247,7 @@ nla_put_failure:
 static inline int
 ctnetlink_dump_secmark(struct sk_buff *skb, const struct nf_conn *ct)
 {
-	NLA_PUT_BE32(skb, CTA_SECMARK, htonl(ct->secmark));
+	NLA_PUT_BE32(skb, CTA_SECMARK, htonl(ct->secmark ? 1 : 0));
 	return 0;
 
 nla_put_failure:
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index eb973fc..a5761d3 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -169,7 +169,7 @@ static int ct_seq_show(struct seq_file *s, void *v)
 #endif
 
 #ifdef CONFIG_NF_CONNTRACK_SECMARK
-	if (seq_printf(s, "secmark=%u ", ct->secmark))
+	if (seq_printf(s, "secmark=%u ", ct->secmark ? 1 : 0))
 		goto release;
 #endif
 

[PATCH 4/6] security: secid_to_secctx returns len when data is NULL

[Eric Paris]

With the (long ago) interface change to have the secid_to_secctx functions
do the string allocation instead of having the caller do the allocation we
lost the ability to query the security server for the length of the
upcoming string.  The SECMARK code would like to allocate a netlink skb
with enough length to hold the string but it is just too unclean to do the
string allocation twice or to do the allocation the first time and hold
onto the string and slen.  This patch adds the ability to call
security_secid_to_secctx() with a NULL data pointer and it will just set
the slen pointer.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 security/selinux/ss/services.c |   11 +++++++++--
 security/smack/smack_lsm.c     |    3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 73508af..1d4955a 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -998,7 +998,8 @@ static int context_struct_to_string(struct context *context, char **scontext, u3
 {
 	char *scontextp;
 
-	*scontext = NULL;
+	if (scontext)
+		*scontext = NULL;
 	*scontext_len = 0;
 
 	if (context->len) {
@@ -1015,6 +1016,9 @@ static int context_struct_to_string(struct context *context, char **scontext, u3
 	*scontext_len += strlen(sym_name(&policydb, SYM_TYPES, context->type - 1)) + 1;
 	*scontext_len += mls_compute_context_len(context);
 
+	if (!scontext)
+		return 0;
+
 	/* Allocate space for the context; caller must free this space. */
 	scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
 	if (!scontextp)
@@ -1054,7 +1058,8 @@ static int security_sid_to_context_core(u32 sid, char **scontext,
 	struct context *context;
 	int rc = 0;
 
-	*scontext = NULL;
+	if (scontext)
+		*scontext = NULL;
 	*scontext_len  = 0;
 
 	if (!ss_initialized) {
@@ -1062,6 +1067,8 @@ static int security_sid_to_context_core(u32 sid, char **scontext,
 			char *scontextp;
 
 			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
+			if (!scontext)
+				goto out;
 			scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
 			if (!scontextp) {
 				rc = -ENOMEM;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index c448d57..b95d7b1 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3005,7 +3005,8 @@ static int smack_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
 {
 	char *sp = smack_from_secid(secid);
 
-	*secdata = sp;
+	if (secdata)
+		*secdata = sp;
 	*seclen = strlen(sp);
 	return 0;
 }

[PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink

[Eric Paris]

The conntrack code can export the internal secid to userspace.  These are
dynamic, can change on lsm changes, and have no meaning in userspace.  We
should instead be sending lsm contexts to userspace instead.  This patch sends
the secctx (rather than secid) to userspace over the netlink socket.  We use a
new field CTA_SECCTX and stop using the the old CTA_SECMARK field since it did
not send particularly useful information.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 include/linux/netfilter/nfnetlink_conntrack.h |    8 ++++
 net/netfilter/nf_conntrack_netlink.c          |   46 ++++++++++++++++++++-----
 2 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/include/linux/netfilter/nfnetlink_conntrack.h b/include/linux/netfilter/nfnetlink_conntrack.h
index 9ed534c..ba03544 100644
--- a/include/linux/netfilter/nfnetlink_conntrack.h
+++ b/include/linux/netfilter/nfnetlink_conntrack.h
@@ -41,6 +41,7 @@ enum ctattr_type {
 	CTA_NAT_SEQ_ADJ_REPLY,
 	CTA_SECMARK,
 	CTA_ZONE,
+	CTA_SECCTX,
 	__CTA_MAX
 };
 #define CTA_MAX (__CTA_MAX - 1)
@@ -172,4 +173,11 @@ enum ctattr_help {
 };
 #define CTA_HELP_MAX (__CTA_HELP_MAX - 1)
 
+enum ctattr_secctx {
+	CTA_SECCTX_UNSPEC,
+	CTA_SECCTX_NAME,
+	__CTA_SECCTX_MAX
+};
+#define CTA_SECCTX_MAX (__CTA_SECCTX_MAX - 1)
+
 #endif /* _IPCONNTRACK_NETLINK_H */
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 3a50699..b3c6285 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -22,6 +22,7 @@
 #include <linux/rculist_nulls.h>
 #include <linux/types.h>
 #include <linux/timer.h>
+#include <linux/security.h>
 #include <linux/skbuff.h>
 #include <linux/errno.h>
 #include <linux/netlink.h>
@@ -245,16 +246,31 @@ nla_put_failure:
 
 #ifdef CONFIG_NF_CONNTRACK_SECMARK
 static inline int
-ctnetlink_dump_secmark(struct sk_buff *skb, const struct nf_conn *ct)
+ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
 {
-	NLA_PUT_BE32(skb, CTA_SECMARK, htonl(ct->secmark ? 1 : 0));
-	return 0;
+	struct nlattr *nest_secctx;
+	int len, ret;
+	char *secctx;
+
+	ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
+	if (ret)
+		return ret;
+
+	ret = -1;
+	nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
+	if (!nest_secctx)
+		goto nla_put_failure;
 
+	NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
+	nla_nest_end(skb, nest_secctx);
+
+	ret = 0;
 nla_put_failure:
-	return -1;
+	security_release_secctx(secctx, len);
+	return ret;
 }
 #else
-#define ctnetlink_dump_secmark(a, b) (0)
+#define ctnetlink_dump_secctx(a, b) (0)
 #endif
 
 #define master_tuple(ct) &(ct->master->tuplehash[IP_CT_DIR_ORIGINAL].tuple)
@@ -391,7 +407,7 @@ ctnetlink_fill_info(struct sk_buff *skb, u32 pid, u32 seq,
 	    ctnetlink_dump_protoinfo(skb, ct) < 0 ||
 	    ctnetlink_dump_helpinfo(skb, ct) < 0 ||
 	    ctnetlink_dump_mark(skb, ct) < 0 ||
-	    ctnetlink_dump_secmark(skb, ct) < 0 ||
+	    ctnetlink_dump_secctx(skb, ct) < 0 ||
 	    ctnetlink_dump_id(skb, ct) < 0 ||
 	    ctnetlink_dump_use(skb, ct) < 0 ||
 	    ctnetlink_dump_master(skb, ct) < 0 ||
@@ -437,6 +453,17 @@ ctnetlink_counters_size(const struct nf_conn *ct)
 	       ;
 }
 
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+static int ctnetlink_nlmsg_secctx_size(const struct nf_conn *ct)
+{
+	int len;
+
+	security_secid_to_secctx(ct->secmark, NULL, &len);
+
+	return sizeof(char) * len;
+}
+#endif
+
 static inline size_t
 ctnetlink_nlmsg_size(const struct nf_conn *ct)
 {
@@ -453,7 +480,8 @@ ctnetlink_nlmsg_size(const struct nf_conn *ct)
 	       + nla_total_size(0) /* CTA_HELP */
 	       + nla_total_size(NF_CT_HELPER_NAME_LEN) /* CTA_HELP_NAME */
 #ifdef CONFIG_NF_CONNTRACK_SECMARK
-	       + nla_total_size(sizeof(u_int32_t)) /* CTA_SECMARK */
+	       + nla_total_size(0) /* CTA_SECCTX */
+	       + nla_total_size(ctnetlink_nlmsg_secctx_size(ct)) /* CTA_SECCTX_NAME */
 #endif
 #ifdef CONFIG_NF_NAT_NEEDED
 	       + 2 * nla_total_size(0) /* CTA_NAT_SEQ_ADJ_ORIG|REPL */
@@ -554,11 +582,9 @@ ctnetlink_conntrack_event(unsigned int events, struct nf_ct_event *item)
 		    && ctnetlink_dump_helpinfo(skb, ct) < 0)
 			goto nla_put_failure;
 
-#ifdef CONFIG_NF_CONNTRACK_SECMARK
 		if ((events & (1 << IPCT_SECMARK) || ct->secmark)
-		    && ctnetlink_dump_secmark(skb, ct) < 0)
+		    && ctnetlink_dump_secctx(skb, ct) < 0)
 			goto nla_put_failure;
-#endif
 
 		if (events & (1 << IPCT_RELATED) &&
 		    ctnetlink_dump_master(skb, ct) < 0)

[PATCH 6/6] secmark: export secctx, drop secmark in procfs

[Eric Paris]

The current secmark code exports a secmark= field which just indicates if
there is special labeling on a packet or not.  We drop this field as it
isn't particularly useful and instead export a new field secctx= which is
the actual human readable text label.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 .../netfilter/nf_conntrack_l3proto_ipv4_compat.c   |   27 ++++++++++++++++++--
 net/netfilter/nf_conntrack_standalone.c            |   27 ++++++++++++++++++--
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
index 053d7d3..2ca510e 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
@@ -11,6 +11,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/percpu.h>
+#include <linux/security.h>
 #include <net/net_namespace.h>
 
 #include <linux/netfilter.h>
@@ -87,6 +88,28 @@ static void ct_seq_stop(struct seq_file *s, void *v)
 	rcu_read_unlock();
 }
 
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+static int ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
+{
+	int len, ret;
+	char *secctx;
+
+	ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
+	if (ret)
+		return ret;
+
+	ret = seq_printf(s, "secctx=%s ", secctx);
+
+	security_release_secctx(secctx, len);
+	return ret;
+}
+#else
+static inline int ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
+{
+	return 0;
+}
+#endif
+
 static int ct_seq_show(struct seq_file *s, void *v)
 {
 	struct nf_conntrack_tuple_hash *hash = v;
@@ -148,10 +171,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
 		goto release;
 #endif
 
-#ifdef CONFIG_NF_CONNTRACK_SECMARK
-	if (seq_printf(s, "secmark=%u ", ct->secmark ? 1 : 0))
+	if (ct_show_secctx(s, ct))
 		goto release;
-#endif
 
 	if (seq_printf(s, "use=%u\n", atomic_read(&ct->ct_general.use)))
 		goto release;
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index a5761d3..a6985da 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -15,6 +15,7 @@
 #include <linux/seq_file.h>
 #include <linux/percpu.h>
 #include <linux/netdevice.h>
+#include <linux/security.h>
 #include <net/net_namespace.h>
 #ifdef CONFIG_SYSCTL
 #include <linux/sysctl.h>
@@ -108,6 +109,28 @@ static void ct_seq_stop(struct seq_file *s, void *v)
 	rcu_read_unlock();
 }
 
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+static int ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
+{
+	int len, ret;
+	char *secctx;
+
+	ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
+	if (ret)
+		return ret;
+
+	ret = seq_printf(s, "secctx=%s ", secctx);
+
+	security_release_secctx(secctx, len);
+	return ret;
+}
+#else
+static inline int ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
+{
+	return 0;
+}
+#endif
+
 /* return 0 on success, 1 in case of error */
 static int ct_seq_show(struct seq_file *s, void *v)
 {
@@ -168,10 +191,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
 		goto release;
 #endif
 
-#ifdef CONFIG_NF_CONNTRACK_SECMARK
-	if (seq_printf(s, "secmark=%u ", ct->secmark ? 1 : 0))
+	if (ct_show_secctx(s, ct))
 		goto release;
-#endif
 
 #ifdef CONFIG_NF_CONNTRACK_ZONES
 	if (seq_printf(s, "zone=%u ", nf_ct_zone(ct)))

Re: [PATCH 1/6] secmark: do not return early if there was no error

[Jan Engelhardt]

On Friday 2010-09-24 22:45, Eric Paris wrote:

>Commit 4a5a5c73 attempted to pass decent error messages back to userspace for
>netfilter errors.  In xt_SECMARK.c however the patch screwed up and returned
>on 0 (aka no error) early and didn't finish setting up secmark.  This results
>in a kernel BUG if you use SECMARK.

>+++ b/net/netfilter/xt_SECMARK.c
>@@ -101,7 +101,7 @@ static int secmark_tg_check(const struct xt_tgchk_param *par)
> 	switch (info->mode) {
> 	case SECMARK_MODE_SEL:
> 		err = checkentry_selinux(info);
>-		if (err <= 0)
>+		if (err)
> 			return err;

Indeed the = is unwanted and err < 0 was intended here.
Sorry for the slip.

Re: [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink

[Jan Engelhardt]

On Friday 2010-09-24 22:45, Eric Paris wrote:
>+#ifdef CONFIG_NF_CONNTRACK_SECMARK
>+static int ctnetlink_nlmsg_secctx_size(const struct nf_conn *ct)
>+{
>+	int len;
>+
>+	security_secid_to_secctx(ct->secmark, NULL, &len);
>+
>+	return sizeof(char) * len;
>+}
>+#endif

sizeof(char) is defined to be 1, so it's a little redundant.

Re: [PATCH 2/6] secmark: make secmark object handling generic

[Pablo Neira Ayuso]

On 24/09/10 22:45, Eric Paris wrote:
> Right now secmark has lots of direct selinux calls.  Use all LSM calls and
> remove all SELinux specific knowledge.  The only SELinux specific knowledge
> we leave is the mode.  The only point is to make sure that other LSMs at
> least test this generic code before they assume it works.  (They may also
> have to make changes if they do not represent labels as strings)
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
> 
>  include/linux/netfilter/xt_SECMARK.h |   14 ++------
>  include/linux/security.h             |   25 +++++++++++++
>  include/linux/selinux.h              |   63 ----------------------------------
>  net/netfilter/xt_CT.c                |    1 -
>  net/netfilter/xt_SECMARK.c           |   57 +++++++++++++++----------------
>  security/capability.c                |   17 +++++++++
>  security/security.c                  |   18 ++++++++++
>  security/selinux/exports.c           |   49 --------------------------
>  security/selinux/hooks.c             |   24 +++++++++++++
>  security/selinux/include/security.h  |    1 +
>  10 files changed, 116 insertions(+), 153 deletions(-)
> 
> diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
> index 6fcd344..b8d55c4 100644
> --- a/include/linux/netfilter/xt_SECMARK.h
> +++ b/include/linux/netfilter/xt_SECMARK.h
> @@ -10,19 +10,13 @@
>   * 'mode' refers to the specific security subsystem which the
>   * packets are being marked for.
>   */
> -#define SECMARK_MODE_SEL	0x01		/* SELinux */
> -#define SECMARK_SELCTX_MAX	256
> -
> -struct xt_secmark_target_selinux_info {
> -	__u32 selsid;
> -	char selctx[SECMARK_SELCTX_MAX];
> -};
> +#define SECMARK_MODE_SELINUX	0x01		/* SELinux */
> +#define SECMARK_SECCTX_MAX	256

The SECMARK_MODE_SEL is exposed to user-space, even if we have a copy of
it in the internal iptables tree, I'm not sure if it's a good policy to
change it.

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Pablo Neira Ayuso]

On 24/09/10 22:45, Eric Paris wrote:
> Currently the nfconntrack export code sends the kernel internal secid to
> userspace in a couple of proc files and over netlink as an integer.  This
> is wrong.  This number is a kernel internal.  This patch changes the export
> code to output either 0 or 1 for this value.  A future patch will implement
> sending the name rather than the number in a new field.

I'm not sure why you need this transitional patch if you later on
replace it.

Better to change the /proc output to make it consistent with patch 5/6?

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[James Morris]

On Fri, 24 Sep 2010, Eric Paris wrote:

> Currently the nfconntrack export code sends the kernel internal secid to
> userspace in a couple of proc files and over netlink as an integer.  This
> is wrong.  This number is a kernel internal.

I wouldn't say wrong, exactly.

The secid is a 32-bit integer which each LSM maps to its own domain.  
It's unlikely to change from this format (we were v. lucky to get this 
field, btw).

There's really no difference between this and the various kernel addresses 
exported via procfs by the networking code -- it's an opaque token which 
you can use to identify the security marking of a packet without 
necessarily having any knowledge of the security model itself.

It's potentially useful for debugging and writing simple userland tools, 
and may be in use already by apps.

>  This patch changes the export
> code to output either 0 or 1 for this value.

This potentially breaks existing apps and does not seem to add any value 
itself.  If you want to determine if a packet is marked, then simply check 
for zero or not.

>  A future patch will implement sending the name rather than the number 
> in a new field.

For the reasons above, I think the secctx string needs to be exported in 
addition to this rather than instead of.


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

Re: [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink

[Pablo Neira Ayuso]

On 24/09/10 22:45, Eric Paris wrote:
> @@ -172,4 +173,11 @@ enum ctattr_help {
>   };
>   #define CTA_HELP_MAX (__CTA_HELP_MAX - 1)
>
> +enum ctattr_secctx {
> +	CTA_SECCTX_UNSPEC,
> +	CTA_SECCTX_NAME,
> +	__CTA_SECCTX_MAX
> +};
> +#define CTA_SECCTX_MAX (__CTA_SECCTX_MAX - 1)

I guess that you have included this nest for consistency with CTA_HELP. 
My question: do you think that we'll include more attributes in that 
nest in the future? Otherwise, I would remove that nest and put 
CTA_SECCTX_NAME in the first level, since the nest would increase the 
message size.

Re: [PATCH 4/6] security: secid_to_secctx returns len when data is NULL

[Casey Schaufler]

 On 9/24/2010 1:45 PM, Eric Paris wrote:
> With the (long ago) interface change to have the secid_to_secctx functions
> do the string allocation instead of having the caller do the allocation we
> lost the ability to query the security server for the length of the
> upcoming string.  The SECMARK code would like to allocate a netlink skb
> with enough length to hold the string but it is just too unclean to do the
> string allocation twice or to do the allocation the first time and hold
> onto the string and slen.  This patch adds the ability to call
> security_secid_to_secctx() with a NULL data pointer and it will just set
> the slen pointer.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>

Acked-by: Casey Schaufler <casey@schaufler-ca.com>

For the Smack bit at least.


> ---
>
>  security/selinux/ss/services.c |   11 +++++++++--
>  security/smack/smack_lsm.c     |    3 ++-
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 73508af..1d4955a 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -998,7 +998,8 @@ static int context_struct_to_string(struct context *context, char **scontext, u3
>  {
>  	char *scontextp;
>  
> -	*scontext = NULL;
> +	if (scontext)
> +		*scontext = NULL;
>  	*scontext_len = 0;
>  
>  	if (context->len) {
> @@ -1015,6 +1016,9 @@ static int context_struct_to_string(struct context *context, char **scontext, u3
>  	*scontext_len += strlen(sym_name(&policydb, SYM_TYPES, context->type - 1)) + 1;
>  	*scontext_len += mls_compute_context_len(context);
>  
> +	if (!scontext)
> +		return 0;
> +
>  	/* Allocate space for the context; caller must free this space. */
>  	scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
>  	if (!scontextp)
> @@ -1054,7 +1058,8 @@ static int security_sid_to_context_core(u32 sid, char **scontext,
>  	struct context *context;
>  	int rc = 0;
>  
> -	*scontext = NULL;
> +	if (scontext)
> +		*scontext = NULL;
>  	*scontext_len  = 0;
>  
>  	if (!ss_initialized) {
> @@ -1062,6 +1067,8 @@ static int security_sid_to_context_core(u32 sid, char **scontext,
>  			char *scontextp;
>  
>  			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
> +			if (!scontext)
> +				goto out;
>  			scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
>  			if (!scontextp) {
>  				rc = -ENOMEM;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index c448d57..b95d7b1 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3005,7 +3005,8 @@ static int smack_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
>  {
>  	char *sp = smack_from_secid(secid);
>  
> -	*secdata = sp;
> +	if (secdata)
> +		*secdata = sp;
>  	*seclen = strlen(sp);
>  	return 0;
>  }
>
>

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Eric Paris]

On Sat, 2010-09-25 at 10:41 +0200, Pablo Neira Ayuso wrote:
> On 24/09/10 22:45, Eric Paris wrote:
> > Currently the nfconntrack export code sends the kernel internal secid to
> > userspace in a couple of proc files and over netlink as an integer.  This
> > is wrong.  This number is a kernel internal.  This patch changes the export
> > code to output either 0 or 1 for this value.  A future patch will implement
> > sending the name rather than the number in a new field.
> 
> I'm not sure why you need this transitional patch if you later on
> replace it.
> 
> Better to change the /proc output to make it consistent with patch 5/6?

Jan has stated his opinion I should not export the new secctx field via
the procfs interface.  This patch was included in the series in case
that was the consensus of the lists and thus we just wouldn't apply
patch 6/6 (which drops secmark and adds secctx to procfs)

-Eric

Re: [PATCH 2/6] secmark: make secmark object handling generic

[Eric Paris]

On Sat, 2010-09-25 at 10:39 +0200, Pablo Neira Ayuso wrote:
> On 24/09/10 22:45, Eric Paris wrote:
> > Right now secmark has lots of direct selinux calls.  Use all LSM calls and
> > remove all SELinux specific knowledge.  The only SELinux specific knowledge
> > we leave is the mode.  The only point is to make sure that other LSMs at
> > least test this generic code before they assume it works.  (They may also
> > have to make changes if they do not represent labels as strings)
> > 
> > Signed-off-by: Eric Paris <eparis@redhat.com>
> > ---
> > 
> >  include/linux/netfilter/xt_SECMARK.h |   14 ++------
> >  include/linux/security.h             |   25 +++++++++++++
> >  include/linux/selinux.h              |   63 ----------------------------------
> >  net/netfilter/xt_CT.c                |    1 -
> >  net/netfilter/xt_SECMARK.c           |   57 +++++++++++++++----------------
> >  security/capability.c                |   17 +++++++++
> >  security/security.c                  |   18 ++++++++++
> >  security/selinux/exports.c           |   49 --------------------------
> >  security/selinux/hooks.c             |   24 +++++++++++++
> >  security/selinux/include/security.h  |    1 +
> >  10 files changed, 116 insertions(+), 153 deletions(-)
> > 
> > diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
> > index 6fcd344..b8d55c4 100644
> > --- a/include/linux/netfilter/xt_SECMARK.h
> > +++ b/include/linux/netfilter/xt_SECMARK.h
> > @@ -10,19 +10,13 @@
> >   * 'mode' refers to the specific security subsystem which the
> >   * packets are being marked for.
> >   */
> > -#define SECMARK_MODE_SEL	0x01		/* SELinux */
> > -#define SECMARK_SELCTX_MAX	256
> > -
> > -struct xt_secmark_target_selinux_info {
> > -	__u32 selsid;
> > -	char selctx[SECMARK_SELCTX_MAX];
> > -};
> > +#define SECMARK_MODE_SELINUX	0x01		/* SELinux */
> > +#define SECMARK_SECCTX_MAX	256
> 
> The SECMARK_MODE_SEL is exposed to user-space, even if we have a copy of
> it in the internal iptables tree, I'm not sure if it's a good policy to
> change it.

Ok.  I just hate 'SEL' as an abbreviation of SELinux.  I always think
it's select or something like that.  I could do something hideous like

#define SECMARK_MODE_SEL	0x01
#define SECMARK_MODE_SELINUX	SECMARK_MODE_SEL

But it seems like too much work and doesn't add anything really.  I'll
revert that part of this change.

-Eric

Re: [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink

[Eric Paris]

On Mon, 2010-09-27 at 13:01 +0200, Pablo Neira Ayuso wrote:
> On 24/09/10 22:45, Eric Paris wrote:
> > @@ -172,4 +173,11 @@ enum ctattr_help {
> >   };
> >   #define CTA_HELP_MAX (__CTA_HELP_MAX - 1)
> >
> > +enum ctattr_secctx {
> > +	CTA_SECCTX_UNSPEC,
> > +	CTA_SECCTX_NAME,
> > +	__CTA_SECCTX_MAX
> > +};
> > +#define CTA_SECCTX_MAX (__CTA_SECCTX_MAX - 1)
> 
> I guess that you have included this nest for consistency with CTA_HELP. 
> My question: do you think that we'll include more attributes in that 
> nest in the future? Otherwise, I would remove that nest and put 
> CTA_SECCTX_NAME in the first level, since the nest would increase the 
> message size.

My initial thought was that you were right (and I did just copy the
CTA_HELP implementation), but now I've decided that I like that nest.  I
have no idea what future representation an LSM might use.  The only 2
LSMs that make sense to use this interface SELinux and SMACK both use a
string, but I don't know why we have to limit it to that....

-Eric

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Eric Paris]

On Mon, 2010-09-27 at 10:50 +1000, James Morris wrote:
> On Fri, 24 Sep 2010, Eric Paris wrote:

> For the reasons above, I think the secctx string needs to be exported in 
> addition to this rather than instead of.

I won't argue, I don't agree with your reasoning, but I'm not opposed to
this result.  We have 3 competing suggestions:

Jan suggested we:
completely eliminate secmark from procfs+netlink and only export secctx
in netlink.

Eric suggested we:
completely eliminate secmark from procfs+netlink and then export secctx
in procfs+netlink

sounds like James suggested we:
continue to export meaningless and confusing secmark from procfs+netlink
and then export secctx in procfs+netlink as well.

I'm going to implement James' idea and resend the patch series.  Any
strong objections?

-Eric

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Paul Moore]

On Mon, 2010-09-27 at 13:01 -0400, Eric Paris wrote:
> On Mon, 2010-09-27 at 10:50 +1000, James Morris wrote:
> > On Fri, 24 Sep 2010, Eric Paris wrote:
> 
> > For the reasons above, I think the secctx string needs to be exported in 
> > addition to this rather than instead of.
> 
> I won't argue, I don't agree with your reasoning, but I'm not opposed to
> this result.  We have 3 competing suggestions:
> 
> Jan suggested we:
> completely eliminate secmark from procfs+netlink and only export secctx
> in netlink.
> 
> Eric suggested we:
> completely eliminate secmark from procfs+netlink and then export secctx
> in procfs+netlink
> 
> sounds like James suggested we:
> continue to export meaningless and confusing secmark from procfs+netlink
> and then export secctx in procfs+netlink as well.
> 
> I'm going to implement James' idea and resend the patch series.  Any
> strong objections?

I apologize for not getting a chance to look at these patches sooner.
In general they look fine to me and my only real concern was addressed
by Pablo already (breaking userspace due to #define changes).

As far as exporting the 32bit secid/secmark to userspace, I think that
is a mistake.  James correctly points out that it does map to a LSM
specific value, e.g. SELinux and Smack security labels, but I don't
think he makes it clear that in the two LSMs that currently use secids
the mapping between the secid and the secctx is not constant; the secids
are transient values that will change with each boot in a manner that
userspace can not predict.  For this reason, I think exporting the
secids is only going to cause users/admins grief, whereas exporting the
associated secctx should be a much more stable value and is likely what
the user/admin is expecting anyway.

-- 
paul moore
linux @ hp

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Eric Paris]

On Mon, 2010-09-27 at 14:29 -0400, Paul Moore wrote:
> On Mon, 2010-09-27 at 13:01 -0400, Eric Paris wrote:
> > On Mon, 2010-09-27 at 10:50 +1000, James Morris wrote:
> > > On Fri, 24 Sep 2010, Eric Paris wrote:
> > 
> > > For the reasons above, I think the secctx string needs to be exported in 
> > > addition to this rather than instead of.
> > 
> > I won't argue, I don't agree with your reasoning, but I'm not opposed to
> > this result.  We have 3 competing suggestions:
> > 
> > Jan suggested we:
> > completely eliminate secmark from procfs+netlink and only export secctx
> > in netlink.
> > 
> > Eric suggested we:
> > completely eliminate secmark from procfs+netlink and then export secctx
> > in procfs+netlink
> > 
> > sounds like James suggested we:
> > continue to export meaningless and confusing secmark from procfs+netlink
> > and then export secctx in procfs+netlink as well.
> > 
> > I'm going to implement James' idea and resend the patch series.  Any
> > strong objections?
> 
> I apologize for not getting a chance to look at these patches sooner.
> In general they look fine to me and my only real concern was addressed
> by Pablo already (breaking userspace due to #define changes).
> 
> As far as exporting the 32bit secid/secmark to userspace, I think that
> is a mistake.  James correctly points out that it does map to a LSM
> specific value, e.g. SELinux and Smack security labels, but I don't
> think he makes it clear that in the two LSMs that currently use secids
> the mapping between the secid and the secctx is not constant; the secids
> are transient values that will change with each boot in a manner that
> userspace can not predict.  For this reason, I think exporting the
> secids is only going to cause users/admins grief, whereas exporting the
> associated secctx should be a much more stable value and is likely what
> the user/admin is expecting anyway.

So it sounds to me like Paul agrees with me that exporting the SELinux
sid as 'secmark=' was a bad idea.  It's the whole reason this thread
started, someone wanted to be able to translate and use that field (and
instantly realized it was useless.)

I see it as having 3 options.  lets assume was have a packet with
selinux sid=121 and selinux context=packet_t.  We can

1) secmark=121 secctx=packet_t
	This continues to send secmark like we do and people might continue to
be baffled by the 121.

2) secmark=1 secctx=packet_t
	This sends a secmark field to userspace so if an application which
reads this exists (I doubt such an application actually exists in in the
real world) it will still get all of the information it got before but
noone will be baffled by what the number means.  1/0 is pretty obvious.

3) secctx=packet_t
	Smallest easiest, what my patches actually do.  Could theoretically
break some script that expected the field to be there, but any such
script is already broken since that field can be easily compiled
out......

James, if you are adamant about #1 I'll resend, otherwise I'm sticking
with #3.....

-Eric

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Paul Moore]

On Mon, 2010-09-27 at 15:25 -0400, Eric Paris wrote:
> On Mon, 2010-09-27 at 14:29 -0400, Paul Moore wrote:
> > On Mon, 2010-09-27 at 13:01 -0400, Eric Paris wrote:
> > > On Mon, 2010-09-27 at 10:50 +1000, James Morris wrote:
> > > > On Fri, 24 Sep 2010, Eric Paris wrote:
> > > 
> > > > For the reasons above, I think the secctx string needs to be exported in 
> > > > addition to this rather than instead of.
> > > 
> > > I won't argue, I don't agree with your reasoning, but I'm not opposed to
> > > this result.  We have 3 competing suggestions:
> > > 
> > > Jan suggested we:
> > > completely eliminate secmark from procfs+netlink and only export secctx
> > > in netlink.
> > > 
> > > Eric suggested we:
> > > completely eliminate secmark from procfs+netlink and then export secctx
> > > in procfs+netlink
> > > 
> > > sounds like James suggested we:
> > > continue to export meaningless and confusing secmark from procfs+netlink
> > > and then export secctx in procfs+netlink as well.
> > > 
> > > I'm going to implement James' idea and resend the patch series.  Any
> > > strong objections?
> > 
> > I apologize for not getting a chance to look at these patches sooner.
> > In general they look fine to me and my only real concern was addressed
> > by Pablo already (breaking userspace due to #define changes).
> > 
> > As far as exporting the 32bit secid/secmark to userspace, I think that
> > is a mistake.  James correctly points out that it does map to a LSM
> > specific value, e.g. SELinux and Smack security labels, but I don't
> > think he makes it clear that in the two LSMs that currently use secids
> > the mapping between the secid and the secctx is not constant; the secids
> > are transient values that will change with each boot in a manner that
> > userspace can not predict.  For this reason, I think exporting the
> > secids is only going to cause users/admins grief, whereas exporting the
> > associated secctx should be a much more stable value and is likely what
> > the user/admin is expecting anyway.
> 
> So it sounds to me like Paul agrees with me that exporting the SELinux
> sid as 'secmark=' was a bad idea.  It's the whole reason this thread
> started, someone wanted to be able to translate and use that field (and
> instantly realized it was useless.)
> 
> I see it as having 3 options.  lets assume was have a packet with
> selinux sid=121 and selinux context=packet_t.  We can
> 
> 1) secmark=121 secctx=packet_t
> 	This continues to send secmark like we do and people might continue to
> be baffled by the 121.
> 
> 2) secmark=1 secctx=packet_t
> 	This sends a secmark field to userspace so if an application which
> reads this exists (I doubt such an application actually exists in in the
> real world) it will still get all of the information it got before but
> noone will be baffled by what the number means.  1/0 is pretty obvious.
> 
> 3) secctx=packet_t
> 	Smallest easiest, what my patches actually do.  Could theoretically
> break some script that expected the field to be there, but any such
> script is already broken since that field can be easily compiled
> out......
> 
> James, if you are adamant about #1 I'll resend, otherwise I'm sticking
> with #3.....

James, if you do feel strongly about door #1, can you provide a good
example of how someone might use the secid to do something useful in
userspace?  I ask because I can't think of anything and it would be nice
to have the example on record for future work.

-- 
paul moore
linux @ hp

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Pablo Neira Ayuso]

On 27/09/10 21:25, Eric Paris wrote:
> I see it as having 3 options.  lets assume was have a packet with
> selinux sid=121 and selinux context=packet_t.  We can
> 
> 1) secmark=121 secctx=packet_t
> 	This continues to send secmark like we do and people might continue to
> be baffled by the 121.
> 
> 2) secmark=1 secctx=packet_t
> 	This sends a secmark field to userspace so if an application which
> reads this exists (I doubt such an application actually exists in in the
> real world) it will still get all of the information it got before but
> noone will be baffled by what the number means.  1/0 is pretty obvious.

In netlink, we can obsolete fields without breaking backward
compatibility. Applications parsing the /proc entry may break, but they
should use stable interfaces (like netlink) instead.

BTW, if we finally stop including CTA_SECMARK in netlink messages,
please add a small comment on the right of the definition in
nfnetlink_conntrack.h (something like /* obsolete */ or /* unused */).
Thanks!

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[James Morris]

On Mon, 27 Sep 2010, Eric Paris wrote:

> James, if you are adamant about #1 I'll resend, otherwise I'm sticking
> with #3.....

Looks like #3 it is.


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Jan Engelhardt]

On Tuesday 2010-09-28 00:48, Pablo Neira Ayuso wrote:

>On 27/09/10 21:25, Eric Paris wrote:
>> I see it as having 3 options.  lets assume was have a packet with
>> selinux sid=121 and selinux context=packet_t.  We can
>> 
>> 1) secmark=121 secctx=packet_t
>> 	This continues to send secmark like we do and people might continue to
>> be baffled by the 121.
>> 
>> 2) secmark=1 secctx=packet_t
>> 	This sends a secmark field to userspace so if an application which
>> reads this exists (I doubt such an application actually exists in in the
>> real world) it will still get all of the information it got before but
>> noone will be baffled by what the number means.  1/0 is pretty obvious.
>
>In netlink, we can obsolete fields without breaking backward
>compatibility. Applications parsing the /proc entry may break, but they
>should use stable interfaces (like netlink) instead.

Which I take as a pro stance on not adding any more procfs fields.

>BTW, if we finally stop including CTA_SECMARK in netlink messages,
>please add a small comment on the right of the definition in
>nfnetlink_conntrack.h (something like /* obsolete */ or /* unused */).
>Thanks!

Mh, I prefer "obsolete". A lot of times in the kernel there is "unused" 
and it reads like, "if it's unused, why is is there?" (it /is/ used, 
though as a filler).

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Mr Dash Four]

>> In netlink, we can obsolete fields without breaking backward
>> compatibility. Applications parsing the /proc entry may break, but they
>> should use stable interfaces (like netlink) instead.
>>     
>
> Which I take as a pro stance on not adding any more procfs fields.
>   
How did you figure that one out then?

Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid

[Casey Schaufler]

 On 9/27/2010 12:25 PM, Eric Paris wrote:
> On Mon, 2010-09-27 at 14:29 -0400, Paul Moore wrote:
>> On Mon, 2010-09-27 at 13:01 -0400, Eric Paris wrote:
>>> On Mon, 2010-09-27 at 10:50 +1000, James Morris wrote:
>>>> On Fri, 24 Sep 2010, Eric Paris wrote:
>>>> For the reasons above, I think the secctx string needs to be exported in 
>>>> addition to this rather than instead of.
>>> I won't argue, I don't agree with your reasoning, but I'm not opposed to
>>> this result.  We have 3 competing suggestions:
>>>
>>> Jan suggested we:
>>> completely eliminate secmark from procfs+netlink and only export secctx
>>> in netlink.
>>>
>>> Eric suggested we:
>>> completely eliminate secmark from procfs+netlink and then export secctx
>>> in procfs+netlink
>>>
>>> sounds like James suggested we:
>>> continue to export meaningless and confusing secmark from procfs+netlink
>>> and then export secctx in procfs+netlink as well.
>>>
>>> I'm going to implement James' idea and resend the patch series.  Any
>>> strong objections?
>> I apologize for not getting a chance to look at these patches sooner.
>> In general they look fine to me and my only real concern was addressed
>> by Pablo already (breaking userspace due to #define changes).
>>
>> As far as exporting the 32bit secid/secmark to userspace, I think that
>> is a mistake.  James correctly points out that it does map to a LSM
>> specific value, e.g. SELinux and Smack security labels, but I don't
>> think he makes it clear that in the two LSMs that currently use secids
>> the mapping between the secid and the secctx is not constant; the secids
>> are transient values that will change with each boot in a manner that
>> userspace can not predict.  For this reason, I think exporting the
>> secids is only going to cause users/admins grief, whereas exporting the
>> associated secctx should be a much more stable value and is likely what
>> the user/admin is expecting anyway.
> So it sounds to me like Paul agrees with me that exporting the SELinux
> sid as 'secmark=' was a bad idea.  It's the whole reason this thread
> started, someone wanted to be able to translate and use that field (and
> instantly realized it was useless.)
>
> I see it as having 3 options.  lets assume was have a packet with
> selinux sid=121 and selinux context=packet_t.  We can
>
> 1) secmark=121 secctx=packet_t
> 	This continues to send secmark like we do and people might continue to
> be baffled by the 121.
>
> 2) secmark=1 secctx=packet_t
> 	This sends a secmark field to userspace so if an application which
> reads this exists (I doubt such an application actually exists in in the
> real world) it will still get all of the information it got before but
> noone will be baffled by what the number means.  1/0 is pretty obvious.
>
> 3) secctx=packet_t
> 	Smallest easiest, what my patches actually do.  Could theoretically
> break some script that expected the field to be there, but any such
> script is already broken since that field can be easily compiled
> out......
>
> James, if you are adamant about #1 I'll resend, otherwise I'm sticking
> with #3.....

The Smack Project leadership recommends #3.