* Re: [PATCH 4/4] selinux: xfrm - notify users on dropped packets [not found] ` <20110223115715.GL20852@secunet.com> @ 2011-02-23 14:56 ` Paul Moore 2011-02-24 7:33 ` Steffen Klassert 2011-02-24 8:22 ` [PATCH 4/4 v2] " Steffen Klassert 0 siblings, 2 replies; 6+ messages in thread From: Paul Moore @ 2011-02-23 14:56 UTC (permalink / raw) To: Steffen Klassert; +Cc: James Morris, Eric Paris, linux-security-module, selinux On Wednesday, February 23, 2011 6:57:15 AM Steffen Klassert wrote: > 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 <steffen.klassert@secunet.com> > Acked-by: Paul Moore <paul.moore@hp.com> > --- > > 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. I still see the exclamation marks in the patch below; perhaps you sent the wrong version by mistake? Also, as far as keeping ACKs on modified patches, I think it is okay as long as you are just porting a patch, e.g. minor changes necessary so that the patch applies to the current stable kernel. If you have to do anything beyond that, I would appreciate it if you could drop my ACK; I don't have a problem with re-reviewing patches and re-ACKing them if they look okay. > security/selinux/xfrm.c | 26 +++++++++++++++++--------- > 1 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c > index 728c57e..0062613 100644 > --- a/security/selinux/xfrm.c > +++ b/security/selinux/xfrm.c > @@ -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, -- 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] 6+ messages in thread
* Re: [PATCH 4/4] selinux: xfrm - notify users on dropped packets 2011-02-23 14:56 ` [PATCH 4/4] selinux: xfrm - notify users on dropped packets Paul Moore @ 2011-02-24 7:33 ` Steffen Klassert 2011-02-24 8:22 ` [PATCH 4/4 v2] " Steffen Klassert 1 sibling, 0 replies; 6+ messages in thread From: Steffen Klassert @ 2011-02-24 7:33 UTC (permalink / raw) To: Paul Moore; +Cc: James Morris, Eric Paris, linux-security-module, selinux On Wed, Feb 23, 2011 at 09:56:52AM -0500, Paul Moore wrote: > > I still see the exclamation marks in the patch below; perhaps you sent the > wrong version by mistake? > Argh, yes. I'll update the git branch and resend the patch. -- 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] 6+ messages in thread
* [PATCH 4/4 v2] selinux: xfrm - notify users on dropped packets 2011-02-23 14:56 ` [PATCH 4/4] selinux: xfrm - notify users on dropped packets Paul Moore 2011-02-24 7:33 ` Steffen Klassert @ 2011-02-24 8:22 ` Steffen Klassert 2011-02-24 14:51 ` Eric Paris 1 sibling, 1 reply; 6+ messages in thread From: Steffen Klassert @ 2011-02-24 8:22 UTC (permalink / raw) To: Paul Moore; +Cc: James Morris, Eric Paris, linux-security-module, selinux 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 <steffen.klassert@secunet.com> --- security/selinux/xfrm.c | 26 +++++++++++++++++--------- 1 files changed, 17 insertions(+), 9 deletions(-) diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c index 728c57e..b0dd401 100644 --- a/security/selinux/xfrm.c +++ b/security/selinux/xfrm.c @@ -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 -- 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] 6+ messages in thread
* Re: [PATCH 4/4 v2] selinux: xfrm - notify users on dropped packets 2011-02-24 8:22 ` [PATCH 4/4 v2] " Steffen Klassert @ 2011-02-24 14:51 ` Eric Paris 2011-02-25 9:56 ` Steffen Klassert 0 siblings, 1 reply; 6+ messages in thread From: Eric Paris @ 2011-02-24 14:51 UTC (permalink / raw) To: Steffen Klassert Cc: Paul Moore, James Morris, Eric Paris, linux-security-module, selinux 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.... -Eric On Thu, Feb 24, 2011 at 3:22 AM, Steffen Klassert <steffen.klassert@secunet.com> wrote: > 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 <steffen.klassert@secunet.com> > --- > security/selinux/xfrm.c | 26 +++++++++++++++++--------- > 1 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c > index 728c57e..b0dd401 100644 > --- a/security/selinux/xfrm.c > +++ b/security/selinux/xfrm.c > @@ -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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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] 6+ messages in thread
* Re: [PATCH 4/4 v2] selinux: xfrm - notify users on dropped packets 2011-02-24 14:51 ` Eric Paris @ 2011-02-25 9:56 ` Steffen Klassert 2011-02-25 20:07 ` Eric Paris 0 siblings, 1 reply; 6+ messages in thread From: Steffen Klassert @ 2011-02-25 9:56 UTC (permalink / raw) To: Eric Paris Cc: Paul Moore, James Morris, Eric Paris, linux-security-module, selinux On Thu, Feb 24, 2011 at 09:51:04AM -0500, Eric Paris wrote: > 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 -- 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] 6+ messages in thread
* Re: [PATCH 4/4 v2] selinux: xfrm - notify users on dropped packets 2011-02-25 9:56 ` Steffen Klassert @ 2011-02-25 20:07 ` Eric Paris 0 siblings, 0 replies; 6+ messages in thread From: Eric Paris @ 2011-02-25 20:07 UTC (permalink / raw) To: Steffen Klassert Cc: Paul Moore, James Morris, Eric Paris, linux-security-module, selinux Patches 1-3 have been committed to http://git.infradead.org/users/eparis/selinux.git thanks! -Eric On Fri, Feb 25, 2011 at 4:56 AM, Steffen Klassert <steffen.klassert@secunet.com> wrote: > On Thu, Feb 24, 2011 at 09:51:04AM -0500, Eric Paris wrote: >> 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 > -- 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] 6+ messages in thread
end of thread, other threads:[~2011-02-25 20:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20110223115343.GH20852@secunet.com>
[not found] ` <20110223115715.GL20852@secunet.com>
2011-02-23 14:56 ` [PATCH 4/4] selinux: xfrm - notify users on dropped packets Paul Moore
2011-02-24 7:33 ` Steffen Klassert
2011-02-24 8:22 ` [PATCH 4/4 v2] " Steffen Klassert
2011-02-24 14:51 ` Eric Paris
2011-02-25 9:56 ` Steffen Klassert
2011-02-25 20:07 ` Eric Paris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.