Re: [PATCH 7/9] secid reconciliation-v04: Enforcement for SELinux

[Paul Moore <paul.moore@hp.com>]

Stephen Smalley wrote:
> On Mon, 2006-10-02 at 12:12 -0400, Paul Moore wrote:
> 
>>Venkat Yekkirala wrote:
>>
>>>This defines SELinux enforcement of the 2 new LSM hooks as well
>>>as related changes elsewhere in the SELinux code.
>>>
>>>This also now keeps track of the peersid thru the establishment
>>>of a connection on the server (tracking peersid on the client
>>>is covered later in this patch set).
>>>
>>>Signed-off-by: Venkat Yekkirala <vyekkirala@TrustedCS.com>
>>>
>>>{snip}
>>>
>>>+static int selinux_skb_flow_in(struct sk_buff *skb, unsigned short family)
>>>+{
>>>+	u32 xfrm_sid;
>>>+	int err;
>>>+
>>>+	if (selinux_compat_net)
>>>+		return 1;
>>>+
>>>+	/*
>>>+	 * loopback traffic already labeled and
>>>+	 * flow-controlled on outbound. We may
>>>+	 * need to flow-control on the inbound
>>>+	 * as well if there's ever a use-case for it.
>>>+	 */
>>>+	if (skb->dev == &loopback_dev)
>>>+		return 1;
>>>+
>>>+	err = selinux_xfrm_decode_session(skb, &xfrm_sid, 0);
>>>+	BUG_ON(err);
>>
>>Just a quick question that has been nagging me for awhile - any
>>particular reason why this is a BUG_ON() and not an "if (err) goto out;"?
>  
> It appears that selinux_xfrm_decode_session() can only legitimately
> return an error if the last argument (ckall) is non-zero.
> security_skb_classify_flow() was doing the same thing prior to this
> patch series.  It would be clearer if there were two separate interfaces
> that internally use the same helper, with one of the functions returning
> void.

My immediate concern is not really what selinux_xfrm_decode_session()
returns, but how to handle it, or rather errors in general, in
selinux_skb_flow_in().  I'm in the process of creating a patch to add
the missing NetLabel support to the secid patches and I am wondering if
I should BUG_ON() for an error condition or simply jump to "out".
Jumping seems a bit cleaner to me, although perhaps harder to debug, so
I was just wondering what the reasoning was behind the use of BUG_ON().

I honestly don't care at this point, it's a rather minor detail, I'd
just like to "do the right thing" with the NetLabel patch.

-- 
paul moore
linux security @ hp