On Mon, 2006-10-02 at 14:06 -0400, [EMAIL PROTECTED] wrote:
> plain text document attachment (netlabel-secid_support)
> This patch provides the missing NetLabel support to the secid reconciliation
> patchset.
>
> Signed-off-by: Paul Moore <[EMAIL PROTECTED]>
> ---
> security/selinux/hooks.c | 67 +++++++++++------
> security/selinux/include/objsec.h | 1
> security/selinux/include/selinux_netlabel.h | 28 +++----
> security/selinux/ss/services.c | 106
> ++++++++++------------------
> 4 files changed, 98 insertions(+), 104 deletions(-)
> @@ -3725,7 +3723,16 @@ static int selinux_skb_flow_in(struct sk
> if (xfrm_sid)
> skb->secmark = xfrm_sid;
>
> - /* See if NetLabel can flow in thru the current secmark here */
> + err = selinux_netlbl_skb_sid(skb, skb->secmark, &nlbl_sid);
> + BUG_ON(err);
If this selinux_netlbl_skb_sid() call can fail for any reason other than
a kernel bug, then this needs to goto out instead of using BUG_ON. For
example, if the function can fail due to temporary memory pressure
leading to a failed allocation, then you want to simply drop the packet,
not panic the kernel.
> +
> + err = avc_has_perm(nlbl_sid, skb->secmark, SECCLASS_PACKET,
> + PACKET__FLOW_IN, NULL);
This means we end up with two flow_in checks each time, even if only one
or none of the two labeling mechanisms was used, right? Given the
conclusion on the discussion of what it means to use them together (just
redundant), this seems to be pointless overhead.
> @@ -3749,6 +3757,12 @@ static int selinux_skb_flow_out(struct s
> struct sk_security_struct *sksec = skb->sk->sk_security;
> skb->secmark = sksec->sid;
> }
> +
> + err = selinux_netlbl_skb_sid(skb, skb->secmark, &nlbl_sid);
> + BUG_ON(err);
Same concern about BUG_ON as above. Also, I'd have expected this to
happen at the same point that selinux_skb_xfrm_sid() is called.
> + if (nlbl_sid)
> + skb->secmark = nlbl_sid;
And then I'd expect this to be moved up prior to the if (xfrm_sid)
clause, turning that into an else clause, e.g.:
if (nlbl_sid)
skb->secmark = nlbl_sid;
else if (xfrm_sid)
skb->secmark = xfrm_sid;
else if (skb->sk) ...
> @@ -3913,9 +3928,15 @@ static unsigned int selinux_ip_postroute
> skb->sk->sk_security;
> skb->secmark = sksec->sid;
> }
> +
> + err = selinux_netlbl_skb_sid(skb,
> + skb->secmark,
> + &nlbl_sid);
> + BUG_ON(err);
> +
> + if (nlbl_sid)
> + skb->secmark = nlbl_sid;
Similar comments as above.
> }
> - err = avc_has_perm(skb->secmark, SECINITSID_UNLABELED,
> - SECCLASS_PACKET, PACKET__FLOW_OUT, &ad);
Why do you think you can remove this?
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
