* [PATCH 1/5] secmark: do not return early if there was no error
@ 2010-10-12 15:40 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 15:40 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netfilter
Cc: eparis, paul.moore, jmorris, selinux, sds, jengelh,
linux-security-module, mr.dash.four, pablo
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;
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 15:40 ` Eric Paris
@ 2010-10-12 15:40 ` Eric Paris
-1 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 15:40 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netfilter
Cc: eparis, paul.moore, jmorris, selinux, sds, jengelh,
linux-security-module, mr.dash.four, pablo
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 | 12 ++----
include/linux/security.h | 25 +++++++++++++
include/linux/selinux.h | 63 ----------------------------------
net/netfilter/xt_CT.c | 1 -
net/netfilter/xt_SECMARK.c | 51 +++++++++++++---------------
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, 112 insertions(+), 149 deletions(-)
diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
index 6fcd344..989092b 100644
--- a/include/linux/netfilter/xt_SECMARK.h
+++ b/include/linux/netfilter/xt_SECMARK.h
@@ -11,18 +11,12 @@
* 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_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 43771c8..f050119 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:
@@ -1596,6 +1602,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);
@@ -2556,6 +2565,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);
@@ -2710,6 +2722,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..295054e 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>
@@ -39,9 +39,8 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
switch (mode) {
case SECMARK_MODE_SEL:
- secmark = info->u.sel.selsid;
+ 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;
}
@@ -100,16 +99,16 @@ 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;
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;
@@ -119,19 +118,19 @@ static void secmark_tg_destroy(const struct xt_tgdtor_param *par)
{
switch (mode) {
case SECMARK_MODE_SEL:
- selinux_secmark_refcount_dec();
+ 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 fe2da98..a8aa414 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1143,6 +1143,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 f31b0a3..a6c063b 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 79df4a2..671273e 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 */
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH 2/5] secmark: make secmark object handling generic
@ 2010-10-12 15:40 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 15:40 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netfilter
Cc: eparis, paul.moore, jmorris, selinux, sds, jengelh,
linux-security-module, mr.dash.four, pablo
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 | 12 ++----
include/linux/security.h | 25 +++++++++++++
include/linux/selinux.h | 63 ----------------------------------
net/netfilter/xt_CT.c | 1 -
net/netfilter/xt_SECMARK.c | 51 +++++++++++++---------------
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, 112 insertions(+), 149 deletions(-)
diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
index 6fcd344..989092b 100644
--- a/include/linux/netfilter/xt_SECMARK.h
+++ b/include/linux/netfilter/xt_SECMARK.h
@@ -11,18 +11,12 @@
* 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_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 43771c8..f050119 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:
@@ -1596,6 +1602,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);
@@ -2556,6 +2565,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);
@@ -2710,6 +2722,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..295054e 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>
@@ -39,9 +39,8 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
switch (mode) {
case SECMARK_MODE_SEL:
- secmark = info->u.sel.selsid;
+ 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;
}
@@ -100,16 +99,16 @@ 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;
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;
@@ -119,19 +118,19 @@ static void secmark_tg_destroy(const struct xt_tgdtor_param *par)
{
switch (mode) {
case SECMARK_MODE_SEL:
- selinux_secmark_refcount_dec();
+ 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 fe2da98..a8aa414 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1143,6 +1143,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 f31b0a3..a6c063b 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 79df4a2..671273e 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 */
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 15:40 ` Eric Paris
(?)
@ 2010-10-12 16:26 ` Jan Engelhardt
2010-10-12 17:24 ` Pablo Neira Ayuso
-1 siblings, 1 reply; 46+ messages in thread
From: Jan Engelhardt @ 2010-10-12 16:26 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, paul.moore, jmorris,
selinux, sds, linux-security-module, mr.dash.four, pablo
On Tuesday 2010-10-12 17:40, Eric Paris wrote:
>diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
>index 6fcd344..989092b 100644
>--- a/include/linux/netfilter/xt_SECMARK.h
>+++ b/include/linux/netfilter/xt_SECMARK.h
>@@ -11,18 +11,12 @@
> * 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_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];
> };
If you make changes here, bump the .revision please, in here:
> 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,
> };
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 16:26 ` Jan Engelhardt
@ 2010-10-12 17:24 ` Pablo Neira Ayuso
2010-10-12 17:45 ` Eric Paris
0 siblings, 1 reply; 46+ messages in thread
From: Pablo Neira Ayuso @ 2010-10-12 17:24 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Eric Paris, linux-kernel, netfilter-devel, netfilter, paul.moore,
jmorris, selinux, sds, linux-security-module, mr.dash.four
On 12/10/10 18:26, Jan Engelhardt wrote:
> On Tuesday 2010-10-12 17:40, Eric Paris wrote:
>> diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
>> index 6fcd344..989092b 100644
>> --- a/include/linux/netfilter/xt_SECMARK.h
>> +++ b/include/linux/netfilter/xt_SECMARK.h
>> @@ -11,18 +11,12 @@
>> * 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_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];
>> };
>
> If you make changes here, bump the .revision please, in here:
The binary layout of this structure has not changed, it doesn't require
to bump the revision.
>> 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,
>> };
I think that we don't need that extra tab above.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 17:24 ` Pablo Neira Ayuso
@ 2010-10-12 17:45 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 17:45 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Jan Engelhardt, linux-kernel, netfilter-devel, netfilter,
paul.moore, jmorris, selinux, sds, linux-security-module,
mr.dash.four
On Tue, 2010-10-12 at 19:24 +0200, Pablo Neira Ayuso wrote:
> On 12/10/10 18:26, Jan Engelhardt wrote:
> > On Tuesday 2010-10-12 17:40, Eric Paris wrote:
> >> 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,
> >> };
>
> I think that we don't need that extra tab above.
Are you saying that you prefer lots of spaces to get alignment rather
than the single tab? I see examples of both in other struct xt_target
definitions. I didn't make any syntax changes to this struct, so my
guess is that I made this change when I discovered eight spaces in a row
as I was checking the file for tab->space screw-ups before submission.
Since this is a whitespace change in the middle of a real patch I guess
I can drop the hunk entirely if that's what you are asking for....
-Eric
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/5] secmark: make secmark object handling generic
@ 2010-10-12 17:45 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 17:45 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Jan Engelhardt, linux-kernel, netfilter-devel, netfilter,
paul.moore, jmorris, selinux, sds, linux-security-module,
mr.dash.four
On Tue, 2010-10-12 at 19:24 +0200, Pablo Neira Ayuso wrote:
> On 12/10/10 18:26, Jan Engelhardt wrote:
> > On Tuesday 2010-10-12 17:40, Eric Paris wrote:
> >> 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,
> >> };
>
> I think that we don't need that extra tab above.
Are you saying that you prefer lots of spaces to get alignment rather
than the single tab? I see examples of both in other struct xt_target
definitions. I didn't make any syntax changes to this struct, so my
guess is that I made this change when I discovered eight spaces in a row
as I was checking the file for tab->space screw-ups before submission.
Since this is a whitespace change in the middle of a real patch I guess
I can drop the hunk entirely if that's what you are asking for....
-Eric
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 17:45 ` Eric Paris
(?)
@ 2010-10-12 17:53 ` Jan Engelhardt
-1 siblings, 0 replies; 46+ messages in thread
From: Jan Engelhardt @ 2010-10-12 17:53 UTC (permalink / raw)
To: Eric Paris
Cc: Pablo Neira Ayuso, linux-kernel, netfilter-devel, netfilter,
paul.moore, jmorris, selinux, sds, linux-security-module,
mr.dash.four
On Tuesday 2010-10-12 19:45, Eric Paris wrote:
>> >> - .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,
>> >> };
>>
>> I think that we don't need that extra tab above.
>
>Are you saying that you prefer lots of spaces to get alignment rather
>than the single tab?
This discussion has been had before, and the technical result is that
tab's do not align, they only indent logical levels on the left side.
(Since people could have their editors set to display tabs in non-8-wide
fashion to either cram more onto the screen, or widen if they need it
for clarity. Unfortunately, developers don't do the aligning
consistently and instead, like the common Microsoft joke about how 100
developers to insert a lightbulb, just declare 8 spaces for a tab a
standard when that isn't really the point of a tab.)
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 17:45 ` Eric Paris
(?)
(?)
@ 2010-10-12 18:15 ` Pablo Neira Ayuso
-1 siblings, 0 replies; 46+ messages in thread
From: Pablo Neira Ayuso @ 2010-10-12 18:15 UTC (permalink / raw)
To: Eric Paris
Cc: Jan Engelhardt, linux-kernel, netfilter-devel, netfilter,
paul.moore, jmorris, selinux, sds, linux-security-module,
mr.dash.four
On 12/10/10 19:45, Eric Paris wrote:
> On Tue, 2010-10-12 at 19:24 +0200, Pablo Neira Ayuso wrote:
>> On 12/10/10 18:26, Jan Engelhardt wrote:
>>> On Tuesday 2010-10-12 17:40, Eric Paris wrote:
>
>>>> 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,
>>>> };
>>
>> I think that we don't need that extra tab above.
>
> Are you saying that you prefer lots of spaces to get alignment rather
> than the single tab? I see examples of both in other struct xt_target
> definitions. I didn't make any syntax changes to this struct, so my
> guess is that I made this change when I discovered eight spaces in a row
> as I was checking the file for tab->space screw-ups before submission.
> Since this is a whitespace change in the middle of a real patch I guess
> I can drop the hunk entirely if that's what you are asking for....
I think that this is a cleanup that should go into a different patch,
that's all.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 15:40 ` Eric Paris
@ 2010-10-12 22:55 ` Paul Moore
-1 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 22:55 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 11:40 -0400, 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)
I'm sure you have, but I just want to make sure - you've tested this
change (and the others for that matter) against the existing iptables
userspace to make sure everything still works, right?
> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
>
> include/linux/netfilter/xt_SECMARK.h | 12 ++----
> include/linux/security.h | 25 +++++++++++++
> include/linux/selinux.h | 63 ----------------------------------
> net/netfilter/xt_CT.c | 1 -
> net/netfilter/xt_SECMARK.c | 51 +++++++++++++---------------
> 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, 112 insertions(+), 149 deletions(-)
>
> diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
> index 6fcd344..989092b 100644
> --- a/include/linux/netfilter/xt_SECMARK.h
> +++ b/include/linux/netfilter/xt_SECMARK.h
> @@ -11,18 +11,12 @@
> * 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_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 43771c8..f050119 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:
> @@ -1596,6 +1602,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);
> @@ -2556,6 +2565,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);
> @@ -2710,6 +2722,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..295054e 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>
>
> @@ -39,9 +39,8 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
>
> switch (mode) {
> case SECMARK_MODE_SEL:
> - secmark = info->u.sel.selsid;
> + 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;
> }
>
> @@ -100,16 +99,16 @@ 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;
> 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;
> @@ -119,19 +118,19 @@ static void secmark_tg_destroy(const struct xt_tgdtor_param *par)
> {
> switch (mode) {
> case SECMARK_MODE_SEL:
> - selinux_secmark_refcount_dec();
> + 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 fe2da98..a8aa414 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1143,6 +1143,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 f31b0a3..a6c063b 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 79df4a2..671273e 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 */
>
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/5] secmark: make secmark object handling generic
@ 2010-10-12 22:55 ` Paul Moore
0 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 22:55 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 11:40 -0400, 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)
I'm sure you have, but I just want to make sure - you've tested this
change (and the others for that matter) against the existing iptables
userspace to make sure everything still works, right?
> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
>
> include/linux/netfilter/xt_SECMARK.h | 12 ++----
> include/linux/security.h | 25 +++++++++++++
> include/linux/selinux.h | 63 ----------------------------------
> net/netfilter/xt_CT.c | 1 -
> net/netfilter/xt_SECMARK.c | 51 +++++++++++++---------------
> 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, 112 insertions(+), 149 deletions(-)
>
> diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
> index 6fcd344..989092b 100644
> --- a/include/linux/netfilter/xt_SECMARK.h
> +++ b/include/linux/netfilter/xt_SECMARK.h
> @@ -11,18 +11,12 @@
> * 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_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 43771c8..f050119 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:
> @@ -1596,6 +1602,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);
> @@ -2556,6 +2565,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);
> @@ -2710,6 +2722,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..295054e 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>
>
> @@ -39,9 +39,8 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
>
> switch (mode) {
> case SECMARK_MODE_SEL:
> - secmark = info->u.sel.selsid;
> + 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;
> }
>
> @@ -100,16 +99,16 @@ 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;
> 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;
> @@ -119,19 +118,19 @@ static void secmark_tg_destroy(const struct xt_tgdtor_param *par)
> {
> switch (mode) {
> case SECMARK_MODE_SEL:
> - selinux_secmark_refcount_dec();
> + 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 fe2da98..a8aa414 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1143,6 +1143,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 f31b0a3..a6c063b 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 79df4a2..671273e 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 */
>
--
paul moore
linux @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 22:55 ` Paul Moore
@ 2010-10-12 23:01 ` Eric Paris
-1 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 23:01 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 18:55 -0400, Paul Moore wrote:
> On Tue, 2010-10-12 at 11:40 -0400, 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)
>
> I'm sure you have, but I just want to make sure - you've tested this
> change (and the others for that matter) against the existing iptables
> userspace to make sure everything still works, right?
I did. The only patch which needs userspace changes is the exporting of
secctx over netlink. It appears the current userspace tools just
ignores unknown field types. I have a patch to userspace to tell it
about the new field and will send it after the kernel patch goes in.
-Eric
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/5] secmark: make secmark object handling generic
@ 2010-10-12 23:01 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 23:01 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 18:55 -0400, Paul Moore wrote:
> On Tue, 2010-10-12 at 11:40 -0400, 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)
>
> I'm sure you have, but I just want to make sure - you've tested this
> change (and the others for that matter) against the existing iptables
> userspace to make sure everything still works, right?
I did. The only patch which needs userspace changes is the exporting of
secctx over netlink. It appears the current userspace tools just
ignores unknown field types. I have a patch to userspace to tell it
about the new field and will send it after the kernel patch goes in.
-Eric
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 23:01 ` Eric Paris
@ 2010-10-12 23:06 ` Paul Moore
-1 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 23:06 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:01 -0400, Eric Paris wrote:
> On Tue, 2010-10-12 at 18:55 -0400, Paul Moore wrote:
> > On Tue, 2010-10-12 at 11:40 -0400, 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)
> >
> > I'm sure you have, but I just want to make sure - you've tested this
> > change (and the others for that matter) against the existing iptables
> > userspace to make sure everything still works, right?
>
> I did. The only patch which needs userspace changes is the exporting of
> secctx over netlink. It appears the current userspace tools just
> ignores unknown field types. I have a patch to userspace to tell it
> about the new field and will send it after the kernel patch goes in.
Okay, that's good. Is the existing, i.e. unmodified, userspace still
able set a Secmark with your patches applied? That is the part I'm most
concerned about right now ...
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/5] secmark: make secmark object handling generic
@ 2010-10-12 23:06 ` Paul Moore
0 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 23:06 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:01 -0400, Eric Paris wrote:
> On Tue, 2010-10-12 at 18:55 -0400, Paul Moore wrote:
> > On Tue, 2010-10-12 at 11:40 -0400, 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)
> >
> > I'm sure you have, but I just want to make sure - you've tested this
> > change (and the others for that matter) against the existing iptables
> > userspace to make sure everything still works, right?
>
> I did. The only patch which needs userspace changes is the exporting of
> secctx over netlink. It appears the current userspace tools just
> ignores unknown field types. I have a patch to userspace to tell it
> about the new field and will send it after the kernel patch goes in.
Okay, that's good. Is the existing, i.e. unmodified, userspace still
able set a Secmark with your patches applied? That is the part I'm most
concerned about right now ...
--
paul moore
linux @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 23:06 ` Paul Moore
@ 2010-10-12 23:14 ` Eric Paris
-1 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 23:14 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:06 -0400, Paul Moore wrote:
> On Tue, 2010-10-12 at 19:01 -0400, Eric Paris wrote:
> > On Tue, 2010-10-12 at 18:55 -0400, Paul Moore wrote:
> > > On Tue, 2010-10-12 at 11:40 -0400, 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)
> > >
> > > I'm sure you have, but I just want to make sure - you've tested this
> > > change (and the others for that matter) against the existing iptables
> > > userspace to make sure everything still works, right?
> >
> > I did. The only patch which needs userspace changes is the exporting of
> > secctx over netlink. It appears the current userspace tools just
> > ignores unknown field types. I have a patch to userspace to tell it
> > about the new field and will send it after the kernel patch goes in.
>
> Okay, that's good. Is the existing, i.e. unmodified, userspace still
> able set a Secmark with your patches applied? That is the part I'm most
> concerned about right now ...
It is. Everything about secmark is still userspace ABI compatible.
(except what I indicated)
-Eric
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/5] secmark: make secmark object handling generic
@ 2010-10-12 23:14 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 23:14 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:06 -0400, Paul Moore wrote:
> On Tue, 2010-10-12 at 19:01 -0400, Eric Paris wrote:
> > On Tue, 2010-10-12 at 18:55 -0400, Paul Moore wrote:
> > > On Tue, 2010-10-12 at 11:40 -0400, 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)
> > >
> > > I'm sure you have, but I just want to make sure - you've tested this
> > > change (and the others for that matter) against the existing iptables
> > > userspace to make sure everything still works, right?
> >
> > I did. The only patch which needs userspace changes is the exporting of
> > secctx over netlink. It appears the current userspace tools just
> > ignores unknown field types. I have a patch to userspace to tell it
> > about the new field and will send it after the kernel patch goes in.
>
> Okay, that's good. Is the existing, i.e. unmodified, userspace still
> able set a Secmark with your patches applied? That is the part I'm most
> concerned about right now ...
It is. Everything about secmark is still userspace ABI compatible.
(except what I indicated)
-Eric
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/5] secmark: make secmark object handling generic
2010-10-12 23:14 ` Eric Paris
@ 2010-10-12 23:20 ` Paul Moore
-1 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 23:20 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:14 -0400, Eric Paris wrote:
> On Tue, 2010-10-12 at 19:06 -0400, Paul Moore wrote:
> > On Tue, 2010-10-12 at 19:01 -0400, Eric Paris wrote:
> > > On Tue, 2010-10-12 at 18:55 -0400, Paul Moore wrote:
> > > > On Tue, 2010-10-12 at 11:40 -0400, 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)
> > > >
> > > > I'm sure you have, but I just want to make sure - you've tested this
> > > > change (and the others for that matter) against the existing iptables
> > > > userspace to make sure everything still works, right?
> > >
> > > I did. The only patch which needs userspace changes is the exporting of
> > > secctx over netlink. It appears the current userspace tools just
> > > ignores unknown field types. I have a patch to userspace to tell it
> > > about the new field and will send it after the kernel patch goes in.
> >
> > Okay, that's good. Is the existing, i.e. unmodified, userspace still
> > able set a Secmark with your patches applied? That is the part I'm most
> > concerned about right now ...
>
> It is. Everything about secmark is still userspace ABI compatible.
> (except what I indicated)
Excellent, I figured that was the case but just wanted to see it in
writing :)
Acked-by: Paul Moore <paul.moore@hp.com>
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/5] secmark: make secmark object handling generic
@ 2010-10-12 23:20 ` Paul Moore
0 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 23:20 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:14 -0400, Eric Paris wrote:
> On Tue, 2010-10-12 at 19:06 -0400, Paul Moore wrote:
> > On Tue, 2010-10-12 at 19:01 -0400, Eric Paris wrote:
> > > On Tue, 2010-10-12 at 18:55 -0400, Paul Moore wrote:
> > > > On Tue, 2010-10-12 at 11:40 -0400, 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)
> > > >
> > > > I'm sure you have, but I just want to make sure - you've tested this
> > > > change (and the others for that matter) against the existing iptables
> > > > userspace to make sure everything still works, right?
> > >
> > > I did. The only patch which needs userspace changes is the exporting of
> > > secctx over netlink. It appears the current userspace tools just
> > > ignores unknown field types. I have a patch to userspace to tell it
> > > about the new field and will send it after the kernel patch goes in.
> >
> > Okay, that's good. Is the existing, i.e. unmodified, userspace still
> > able set a Secmark with your patches applied? That is the part I'm most
> > concerned about right now ...
>
> It is. Everything about secmark is still userspace ABI compatible.
> (except what I indicated)
Excellent, I figured that was the case but just wanted to see it in
writing :)
Acked-by: Paul Moore <paul.moore@hp.com>
--
paul moore
linux @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 3/5] security: secid_to_secctx returns len when data is NULL
2010-10-12 15:40 ` Eric Paris
@ 2010-10-12 15:40 ` Eric Paris
-1 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 15:40 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netfilter
Cc: eparis, paul.moore, jmorris, selinux, sds, jengelh,
linux-security-module, mr.dash.four, pablo
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 174aec4..bc39f40 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3004,7 +3004,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;
}
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH 3/5] security: secid_to_secctx returns len when data is NULL
@ 2010-10-12 15:40 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 15:40 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netfilter
Cc: eparis, paul.moore, jmorris, selinux, sds, jengelh,
linux-security-module, mr.dash.four, pablo
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 174aec4..bc39f40 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3004,7 +3004,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;
}
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 3/5] security: secid_to_secctx returns len when data is NULL
2010-10-12 15:40 ` Eric Paris
@ 2010-10-12 23:04 ` Paul Moore
-1 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 23:04 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 11:40 -0400, 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.
This opens the door for a potential race condition, but only with some
yet to be defined LSM where the both the mapping between secids and
secctxs and secctxs are not like they are with SELinux and Smack. With
that in mind, it looks good to me I'd just like to see some extra
commentary in security.h warning about the potential pitfalls.
Reviewed-by: Paul Moore <paul.moore@hp.com>
> 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 174aec4..bc39f40 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3004,7 +3004,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;
> }
>
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 3/5] security: secid_to_secctx returns len when data is NULL
@ 2010-10-12 23:04 ` Paul Moore
0 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 23:04 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 11:40 -0400, 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.
This opens the door for a potential race condition, but only with some
yet to be defined LSM where the both the mapping between secids and
secctxs and secctxs are not like they are with SELinux and Smack. With
that in mind, it looks good to me I'd just like to see some extra
commentary in security.h warning about the potential pitfalls.
Reviewed-by: Paul Moore <paul.moore@hp.com>
> 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 174aec4..bc39f40 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3004,7 +3004,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;
> }
>
--
paul moore
linux @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink
2010-10-12 15:40 ` Eric Paris
@ 2010-10-12 15:40 ` Eric Paris
-1 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 15:40 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netfilter
Cc: eparis, paul.moore, jmorris, selinux, sds, jengelh,
linux-security-module, mr.dash.four, pablo
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 | 10 +++++
net/netfilter/nf_conntrack_netlink.c | 46 ++++++++++++++++++++-----
2 files changed, 45 insertions(+), 11 deletions(-)
diff --git a/include/linux/netfilter/nfnetlink_conntrack.h b/include/linux/netfilter/nfnetlink_conntrack.h
index 9ed534c..70cd060 100644
--- a/include/linux/netfilter/nfnetlink_conntrack.h
+++ b/include/linux/netfilter/nfnetlink_conntrack.h
@@ -39,8 +39,9 @@ enum ctattr_type {
CTA_TUPLE_MASTER,
CTA_NAT_SEQ_ADJ_ORIG,
CTA_NAT_SEQ_ADJ_REPLY,
- CTA_SECMARK,
+ CTA_SECMARK, /* obsolete */
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 5bae1cd..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));
- 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)
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink
@ 2010-10-12 15:40 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 15:40 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netfilter
Cc: eparis, paul.moore, jmorris, selinux, sds, jengelh,
linux-security-module, mr.dash.four, pablo
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 | 10 +++++
net/netfilter/nf_conntrack_netlink.c | 46 ++++++++++++++++++++-----
2 files changed, 45 insertions(+), 11 deletions(-)
diff --git a/include/linux/netfilter/nfnetlink_conntrack.h b/include/linux/netfilter/nfnetlink_conntrack.h
index 9ed534c..70cd060 100644
--- a/include/linux/netfilter/nfnetlink_conntrack.h
+++ b/include/linux/netfilter/nfnetlink_conntrack.h
@@ -39,8 +39,9 @@ enum ctattr_type {
CTA_TUPLE_MASTER,
CTA_NAT_SEQ_ADJ_ORIG,
CTA_NAT_SEQ_ADJ_REPLY,
- CTA_SECMARK,
+ CTA_SECMARK, /* obsolete */
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 5bae1cd..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));
- 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)
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink
2010-10-12 15:40 ` Eric Paris
@ 2010-10-12 23:14 ` Paul Moore
-1 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 23:14 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 11:40 -0400, Eric Paris wrote:
> 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.
Looks fine to me in principal, just a nit-picky comment below ...
> -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));
> - 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;
> }
All the ret assignments bother me, I also don't think "nla_put_failure"
is a good choice since we run this code both on success and failure -
how about this:
ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
{
struct nlattr *nest_secctx;
int len, ret;
char *secctx;
ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
if (ret)
return ret;
nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
if (!nest_secctx) {
ret = -1;
goto dump_secctx_out;
}
NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
nla_nest_end(skb, nest_secctx);
dump_secctx_out:
security_release_secctx(secctx, len);
return ret;
}
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink
@ 2010-10-12 23:14 ` Paul Moore
0 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 23:14 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 11:40 -0400, Eric Paris wrote:
> 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.
Looks fine to me in principal, just a nit-picky comment below ...
> -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));
> - 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;
> }
All the ret assignments bother me, I also don't think "nla_put_failure"
is a good choice since we run this code both on success and failure -
how about this:
ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
{
struct nlattr *nest_secctx;
int len, ret;
char *secctx;
ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
if (ret)
return ret;
nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
if (!nest_secctx) {
ret = -1;
goto dump_secctx_out;
}
NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
nla_nest_end(skb, nest_secctx);
dump_secctx_out:
security_release_secctx(secctx, len);
return ret;
}
--
paul moore
linux @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink
2010-10-12 23:14 ` Paul Moore
@ 2010-10-12 23:24 ` Eric Paris
-1 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 23:24 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:14 -0400, Paul Moore wrote:
> On Tue, 2010-10-12 at 11:40 -0400, Eric Paris wrote:
> > 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.
>
> Looks fine to me in principal, just a nit-picky comment below ...
>
> > -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));
> > - 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;
> > }
>
> All the ret assignments bother me, I also don't think "nla_put_failure"
> is a good choice since we run this code both on success and failure -
> how about this:
#define NLA_PUT(skb, attrtype, attrlen, data) \
do { \
if (unlikely(nla_put(skb, attrtype, attrlen, data) < 0)) \
goto nla_put_failure; \
} while(0)
#define NLA_PUT_STRING(skb, attrtype, value) \
NLA_PUT(skb, attrtype, strlen(value) + 1, value)
so we can't get rid of the nla_put_failure tag and it also means your
ret values aren't quite right, we have to set ret = -1 before the
NLA_PUT_STRING()....
> ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> {
> struct nlattr *nest_secctx;
> int len, ret;
> char *secctx;
>
> ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
> if (ret)
> return ret;
>
> nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
> if (!nest_secctx) {
> ret = -1;
> goto dump_secctx_out;
> }
>
> NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
> nla_nest_end(skb, nest_secctx);
>
> dump_secctx_out:
> security_release_secctx(secctx, len);
> return ret;
> }
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink
@ 2010-10-12 23:24 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 23:24 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:14 -0400, Paul Moore wrote:
> On Tue, 2010-10-12 at 11:40 -0400, Eric Paris wrote:
> > 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.
>
> Looks fine to me in principal, just a nit-picky comment below ...
>
> > -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));
> > - 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;
> > }
>
> All the ret assignments bother me, I also don't think "nla_put_failure"
> is a good choice since we run this code both on success and failure -
> how about this:
#define NLA_PUT(skb, attrtype, attrlen, data) \
do { \
if (unlikely(nla_put(skb, attrtype, attrlen, data) < 0)) \
goto nla_put_failure; \
} while(0)
#define NLA_PUT_STRING(skb, attrtype, value) \
NLA_PUT(skb, attrtype, strlen(value) + 1, value)
so we can't get rid of the nla_put_failure tag and it also means your
ret values aren't quite right, we have to set ret = -1 before the
NLA_PUT_STRING()....
> ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> {
> struct nlattr *nest_secctx;
> int len, ret;
> char *secctx;
>
> ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
> if (ret)
> return ret;
>
> nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
> if (!nest_secctx) {
> ret = -1;
> goto dump_secctx_out;
> }
>
> NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
> nla_nest_end(skb, nest_secctx);
>
> dump_secctx_out:
> security_release_secctx(secctx, len);
> return ret;
> }
>
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink
2010-10-12 23:24 ` Eric Paris
@ 2010-10-13 13:41 ` Paul Moore
-1 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-13 13:41 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:24 -0400, Eric Paris wrote:
> On Tue, 2010-10-12 at 19:14 -0400, Paul Moore wrote:
> > > -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));
> > > - 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;
> > > }
> >
> > All the ret assignments bother me, I also don't think "nla_put_failure"
> > is a good choice since we run this code both on success and failure -
> > how about this:
>
> #define NLA_PUT(skb, attrtype, attrlen, data) \
> do { \
> if (unlikely(nla_put(skb, attrtype, attrlen, data) < 0)) \
> goto nla_put_failure; \
> } while(0)
>
> #define NLA_PUT_STRING(skb, attrtype, value) \
> NLA_PUT(skb, attrtype, strlen(value) + 1, value)
>
> so we can't get rid of the nla_put_failure tag and it also means your
> ret values aren't quite right, we have to set ret = -1 before the
> NLA_PUT_STRING()....
Ah, yes, forgot about those stupid macros and their pre-defined goto
labels ... I'm sure someone had a good reason for doing it that way, but
I've never been a fan of that approach (case in point). I'd ask you to
use the "normal" nla_put_*() functions but I see that the rest of the
file uses the macros so it probably isn't worth it ... oh well.
Reviewed-by: Paul Moore <paul.moore@hp.com>
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink
@ 2010-10-13 13:41 ` Paul Moore
0 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-13 13:41 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:24 -0400, Eric Paris wrote:
> On Tue, 2010-10-12 at 19:14 -0400, Paul Moore wrote:
> > > -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));
> > > - 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;
> > > }
> >
> > All the ret assignments bother me, I also don't think "nla_put_failure"
> > is a good choice since we run this code both on success and failure -
> > how about this:
>
> #define NLA_PUT(skb, attrtype, attrlen, data) \
> do { \
> if (unlikely(nla_put(skb, attrtype, attrlen, data) < 0)) \
> goto nla_put_failure; \
> } while(0)
>
> #define NLA_PUT_STRING(skb, attrtype, value) \
> NLA_PUT(skb, attrtype, strlen(value) + 1, value)
>
> so we can't get rid of the nla_put_failure tag and it also means your
> ret values aren't quite right, we have to set ret = -1 before the
> NLA_PUT_STRING()....
Ah, yes, forgot about those stupid macros and their pre-defined goto
labels ... I'm sure someone had a good reason for doing it that way, but
I've never been a fan of that approach (case in point). I'd ask you to
use the "normal" nla_put_*() functions but I see that the rest of the
file uses the macros so it probably isn't worth it ... oh well.
Reviewed-by: Paul Moore <paul.moore@hp.com>
--
paul moore
linux @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 5/5] secmark: export secctx, drop secmark in procfs
2010-10-12 15:40 ` Eric Paris
@ 2010-10-12 15:40 ` Eric Paris
-1 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 15:40 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netfilter
Cc: eparis, paul.moore, jmorris, selinux, sds, jengelh,
linux-security-module, mr.dash.four, pablo
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 244f7cb..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))
+ 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 eb973fc..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))
+ if (ct_show_secctx(s, ct))
goto release;
-#endif
#ifdef CONFIG_NF_CONNTRACK_ZONES
if (seq_printf(s, "zone=%u ", nf_ct_zone(ct)))
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH 5/5] secmark: export secctx, drop secmark in procfs
@ 2010-10-12 15:40 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 15:40 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netfilter
Cc: eparis, paul.moore, jmorris, selinux, sds, jengelh,
linux-security-module, mr.dash.four, pablo
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 244f7cb..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))
+ 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 eb973fc..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))
+ if (ct_show_secctx(s, ct))
goto release;
-#endif
#ifdef CONFIG_NF_CONNTRACK_ZONES
if (seq_printf(s, "zone=%u ", nf_ct_zone(ct)))
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 5/5] secmark: export secctx, drop secmark in procfs
2010-10-12 15:40 ` Eric Paris
@ 2010-10-12 23:19 ` Paul Moore
-1 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 23:19 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 11:40 -0400, Eric Paris wrote:
> 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.
Looks reasonable to me, just some small nits/questions below ...
> 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 244f7cb..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);
Here you define len as an int but the prototype calls for a u32 - is
this going to cause any compiler warnings?
> + 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))
> + 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 eb973fc..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);
Same concern as above.
> + 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))
> + if (ct_show_secctx(s, ct))
> goto release;
> -#endif
>
> #ifdef CONFIG_NF_CONNTRACK_ZONES
> if (seq_printf(s, "zone=%u ", nf_ct_zone(ct)))
>
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 5/5] secmark: export secctx, drop secmark in procfs
@ 2010-10-12 23:19 ` Paul Moore
0 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 23:19 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 11:40 -0400, Eric Paris wrote:
> 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.
Looks reasonable to me, just some small nits/questions below ...
> 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 244f7cb..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);
Here you define len as an int but the prototype calls for a u32 - is
this going to cause any compiler warnings?
> + 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))
> + 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 eb973fc..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);
Same concern as above.
> + 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))
> + if (ct_show_secctx(s, ct))
> goto release;
> -#endif
>
> #ifdef CONFIG_NF_CONNTRACK_ZONES
> if (seq_printf(s, "zone=%u ", nf_ct_zone(ct)))
>
--
paul moore
linux @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 5/5] secmark: export secctx, drop secmark in procfs
2010-10-12 23:19 ` Paul Moore
@ 2010-10-12 23:27 ` Eric Paris
-1 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 23:27 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:19 -0400, Paul Moore wrote:
> On Tue, 2010-10-12 at 11:40 -0400, Eric Paris wrote:
> > 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.
>
> Looks reasonable to me, just some small nits/questions below ...
Will switch to u32 in -v2.
-Eric
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] secmark: export secctx, drop secmark in procfs
@ 2010-10-12 23:27 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 23:27 UTC (permalink / raw)
To: Paul Moore
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 19:19 -0400, Paul Moore wrote:
> On Tue, 2010-10-12 at 11:40 -0400, Eric Paris wrote:
> > 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.
>
> Looks reasonable to me, just some small nits/questions below ...
Will switch to u32 in -v2.
-Eric
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/5] secmark: do not return early if there was no error
2010-10-12 15:40 ` Eric Paris
@ 2010-10-12 22:52 ` Paul Moore
-1 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 22:52 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 11:40 -0400, 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.
...
> Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Paul Moore <paul.moore@hp.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;
>
>
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/5] secmark: do not return early if there was no error
@ 2010-10-12 22:52 ` Paul Moore
0 siblings, 0 replies; 46+ messages in thread
From: Paul Moore @ 2010-10-12 22:52 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, jmorris, selinux, sds,
jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 2010-10-12 at 11:40 -0400, 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.
...
> Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Paul Moore <paul.moore@hp.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;
>
>
--
paul moore
linux @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/5] secmark: do not return early if there was no error
2010-10-12 15:40 ` Eric Paris
@ 2010-10-12 23:38 ` James Morris
-1 siblings, 0 replies; 46+ messages in thread
From: James Morris @ 2010-10-12 23:38 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, paul.moore, selinux,
sds, jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 12 Oct 2010, 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.
>
Does this need to go into current Linus?
> ------------[ 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;
>
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/5] secmark: do not return early if there was no error
@ 2010-10-12 23:38 ` James Morris
0 siblings, 0 replies; 46+ messages in thread
From: James Morris @ 2010-10-12 23:38 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, paul.moore, selinux,
sds, jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 12 Oct 2010, 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.
>
Does this need to go into current Linus?
> ------------[ 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;
>
>
--
James Morris
<jmorris@namei.org>
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/5] secmark: do not return early if there was no error
2010-10-12 23:38 ` James Morris
@ 2010-10-12 23:50 ` Eric Paris
-1 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 23:50 UTC (permalink / raw)
To: James Morris
Cc: linux-kernel, netfilter-devel, netfilter, paul.moore, selinux,
sds, jengelh, linux-security-module, mr.dash.four, pablo
On Wed, 2010-10-13 at 10:38 +1100, James Morris wrote:
> On Tue, 12 Oct 2010, 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.
> >
>
> Does this need to go into current Linus?
It's been broken since v2.6.35-rc1 so it's not exactly new, but yes,
it's broken and will bug like this in current Linus.
-Eric
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/5] secmark: do not return early if there was no error
@ 2010-10-12 23:50 ` Eric Paris
0 siblings, 0 replies; 46+ messages in thread
From: Eric Paris @ 2010-10-12 23:50 UTC (permalink / raw)
To: James Morris
Cc: linux-kernel, netfilter-devel, netfilter, paul.moore, selinux,
sds, jengelh, linux-security-module, mr.dash.four, pablo
On Wed, 2010-10-13 at 10:38 +1100, James Morris wrote:
> On Tue, 12 Oct 2010, 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.
> >
>
> Does this need to go into current Linus?
It's been broken since v2.6.35-rc1 so it's not exactly new, but yes,
it's broken and will bug like this in current Linus.
-Eric
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/5] secmark: do not return early if there was no error
2010-10-12 23:50 ` Eric Paris
@ 2010-10-13 4:07 ` James Morris
-1 siblings, 0 replies; 46+ messages in thread
From: James Morris @ 2010-10-13 4:07 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, paul.moore, selinux,
sds, jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 12 Oct 2010, Eric Paris wrote:
> On Wed, 2010-10-13 at 10:38 +1100, James Morris wrote:
> > On Tue, 12 Oct 2010, 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.
> > >
> >
> > Does this need to go into current Linus?
>
> It's been broken since v2.6.35-rc1 so it's not exactly new, but yes,
> it's broken and will bug like this in current Linus.
Acked-by: James Morris <jmorris@namei.org>
(Guessing this should go in via the networking tree).
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/5] secmark: do not return early if there was no error
@ 2010-10-13 4:07 ` James Morris
0 siblings, 0 replies; 46+ messages in thread
From: James Morris @ 2010-10-13 4:07 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, netfilter-devel, netfilter, paul.moore, selinux,
sds, jengelh, linux-security-module, mr.dash.four, pablo
On Tue, 12 Oct 2010, Eric Paris wrote:
> On Wed, 2010-10-13 at 10:38 +1100, James Morris wrote:
> > On Tue, 12 Oct 2010, 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.
> > >
> >
> > Does this need to go into current Linus?
>
> It's been broken since v2.6.35-rc1 so it's not exactly new, but yes,
> it's broken and will bug like this in current Linus.
Acked-by: James Morris <jmorris@namei.org>
(Guessing this should go in via the networking tree).
--
James Morris
<jmorris@namei.org>
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 46+ messages in thread