On Tue, 17 May 2005, jaegert wrote:

+#define nethooks_debug(fmt, args...) {if (DEBUG) printk(KERN_NOTICE fmt,## args);}

Why isn't this debugging using KERN_DEBUG?

+static inline struct inode_security_struct *get_sock_isec(struct sock *sk)
+{
+       if (!sk->sk_socket) {
+               nethooks_debug("%s: no socket on this sock (!?)\n",
+                              __FUNCTION__);
+               return (struct inode_security_struct *) NULL;
+       }

No need to cast NULL here.

+int selinux_xfrm_policy_lookup(struct sock *sk, struct xfrm_selector *sel, struct flowi *fl, u8 dir)
+{
+	int rc = 0;
+	struct inode_security_struct *isec = NULL;
+	u32 sock_sid, sel_sid = SECINITSID_UNLABELED;
+
+	nethooks_debug("%s: authorize\n", __FUNCTION__);
+
+	if (!sk) {
+                /* no sock to send -- e.g., icmp reply */
+		/* authorize as kernel packet */
+		if (fl && fl->proto == IPPROTO_ICMP) {
+			nethooks_debug("%s: ICMP case\n",
+				       __FUNCTION__);
+			sock_sid = SECINITSID_KERNEL;
+			goto authorize;
+		}
+		/*
+		 * hooks.c accepts packets with no socket unconditionally
+		 */
+		nethooks_debug("%s: no sock on this skbuff: non-ICMP\n",
+			       __FUNCTION__);
+		goto out;
+	}

I'm not sure that ICMP is the only protocol which might be caught here;  
are there any other cases where there's no sk that also get caught here?  
The debugging in any case is not all that useful, I'd suggest adding a
protocol number.

+	read_lock_bh(&sk->sk_callback_lock);

You only need this lock for (dir == FLOW_DIR_IN).

+static inline int selinux_xfrm_selector_alloc(struct xfrm_selector *sel, struct sadb_x_sec_ctx *sec_ctx)
+{
+	int rc = 0;
+
+	if (!sec_ctx)
+		return -EINVAL;

Shouldn't this be a BUG_ON() ?

+	memcpy(sel->security->ctx_str,
+	       sec_ctx+1,
+	       sel->security->ctx_len);

Don't you need to validate that ctx_len is within bounds of the data 
copied from userspace?

+	rc = security_context_to_sid(sel->security->ctx_str,
+				     sel->security->ctx_len,
+				     &sel->security->ctx_sid);
+	sel->security->ctx_str[sel->security->ctx_len] = 0;

Why null terminate here?  security_context_to_sid() was looking for it 
already, assuming we haven't crashed yet.

+int selinux_xfrm_policy_alloc(struct xfrm_policy *xp, struct sadb_x_sec_ctx *sec_ctx)
+{
+	int rc = 0;
+
+	if (!xp)
+		return -EINVAL;

Another case for BUG_ON().

+	nethooks_debug(KERN_NOTICE "%s: start alloc\n", __FUNCTION__);
+
+	rc = selinux_xfrm_selector_alloc(&xp->selector, sec_ctx);
+
+	return rc;
+}

How about getting rid of rc and just return the value of the last 
function.

+void selinux_xfrm_policy_free(struct xfrm_policy *xp)
+{
+	struct xfrm_sec_ctx *xfrm_ctx = xfrm_policy_security(xp);
+
+	if (xfrm_ctx)
+		kfree(xfrm_ctx);
+}

Just call kfree() unconditionally.

+int selinux_xfrm_state_alloc(struct xfrm_state *x, struct sadb_x_sec_ctx *sec_ctx)
+{
+	int rc = 0;
+
+	if (!x)
+		return -EINVAL;

BUG_ON()

+	rc = selinux_xfrm_selector_alloc(&x->sel, sec_ctx);
+
+	return rc;

No need for rc again.

+void selinux_xfrm_state_free(struct xfrm_state *x)
+{
+	struct xfrm_sec_ctx *xfrm_ctx = xfrm_state_security(x);
+
+	if (xfrm_ctx)
+		kfree(xfrm_ctx);
+}

kfree() again.


(Still reviewing...)


- James
-- 
James Morris
[off-list ref]