selinux: xfrm - notify users on dropped packets

In selinux_xfrm_state_pol_flow_match we have cases where we drop packets without asking the avc. No audit message is generated in this case. Lets at least print out a message to the logs, so the users don't need to dig in the code to find out why these packets are dropped.

Signed-off-by Steffen Klassert <[email protected]>

Acked-by Paul Moore <[email protected]>

---

Paul,

I removed the exclamation marks and I changed this one
to print a message too if the flow label does not match the SA label. I kept your ACK on this one, I hope it is ok.
If not please complain.

 security/selinux/xfrm.c |   26 +++++++++++++++++---------
 1 files changed, 17 insertions(+), 9 deletions(-)

security/selinux/xfrm.c +0 -0

index 728c57e..0062613 100644

security/selinux/xfrm.c +6 -3

 			return 0;
-		else
-			if (!selinux_authorizable_xfrm(x))
-				/* Not a SELinux-labeled SA */
+		} else {
+			if (!selinux_authorizable_xfrm(x)) {
+				if (net_ratelimit())
+					printk("selinux: Not a SELinux-labeled SA!\n");
 				return 0;
+			}
+		}
 
 	state_sid = x->security->ctx_sid;

I still see the exclamation marks in the patch below; perhaps you sent the wrong version by mistake?

In selinux_xfrm_state_pol_flow_match we have cases where we drop packets without asking the avc. No audit message is generated in this case. Lets at least print out a message to the logs, so the users don't need to dig in the code to find out why these packets are dropped.

Signed-off-by Steffen Klassert <[email protected]>

---
 security/selinux/xfrm.c |   26 +++++++++++++++++---------

 1 files changed, 17 insertions(+), 9 deletions(-)

security/selinux/xfrm.c +4 -3

index 728c57e..b0dd401 100644
@@ -118,25 +118,33 @@ int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x, struct xfrm_policy *
        int rc;
 
        if (!xp->security)
-               if (x->security)
-                       /* unlabeled policy and labeled SA can't match */
+               if (x->security) {
+                       if (net_ratelimit())
+                               printk("selinux: unlabeled policy and labeled SA can't match\n");
                        return 0;
-               else
+               } else
                        /* unlabeled policy and unlabeled SA match all flows */
                        return 1;

       else
-               if (!x->security)
-                       /* unlabeled SA and labeled policy can't match */
+               if (!x->security) {
+                       if (net_ratelimit())
+                               printk("selinux: unlabeled SA and labeled policy can't match\n"); return 0;
-               else
-                       if (!selinux_authorizable_xfrm(x))
-                               /* Not a SELinux-labeled SA */
+               } else {
+                       if (!selinux_authorizable_xfrm(x)) {
+                               if (net_ratelimit())
+                                       printk("selinux: Not a SELinux-labeled SA\n"); return 0;
+                       }
+               }

       state_sid = x->security->ctx_sid;

-       if (fl->secid != state_sid)
+       if (fl->secid != state_sid) {
+               if (net_ratelimit())
+                       printk("selinux: Flow label does not match SA label\n"); return 0;
+       }

       rc = avc_has_perm(fl->secid, state_sid, SECCLASS_ASSOCIATION, ASSOCIATION__SENDTO,
--
1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

All printk() statement require a KERN_*. I've often heard upstream people say that every printk should only be printed if you expect the user to do something with it. If it doesn't give the user enough information to fix whatever the problem is, or know how to fix whatever the problem is, it's a bad printk. I don't know this code at all, but I'm pretty sure if I got those printk's I'd just feel dumb....

Quoted 8 lines · Eric Paris

All printk() statement require a KERN_*.  I've often heard upstream people say that every printk should only be printed if you expect the user to do something with it.  If it doesn't give the user enough information to fix whatever the problem is, or know how to fix whatever the problem is, it's a bad printk.  I don't know this code at all, but I'm pretty sure if I got those printk's I'd just feel dumb....

The problem with this patch is, that it was one of my debugging patches when I tried to find out why my packets are dropped. It was not contemplated to push it upstream when I wrote it. Then I thought it would be good to print out a message if these packets are dropped and I picked the patch from my debugging branch, which was a bad idea. I think we should drop this patch entirely and focus on the remaining 3 paches for now, as they are real fixes. So I could resend the remaining patches for another round of review, or does anybody want to thake them as they are?

Steffen