LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: David Miller <davem@davemloft.net>
To: latten@austin.ibm.com
Cc: ce@ruault.com, herbert@gondor.apana.org.au,
	linux-kernel@vger.kernel.org, linux-net@vger.kernel.org
Subject: Re: [BUG] 2.6.20 Oopses in xfrm_audit_log
Date: Mon, 12 Feb 2007 13:46:28 -0800 (PST)	[thread overview]
Message-ID: <20070212.134628.98748410.davem@davemloft.net> (raw)
In-Reply-To: <200702121744.l1CHiUD2026684@faith.austin.ibm.com>

From: Joy Latten <latten@austin.ibm.com>
Date: Mon, 12 Feb 2007 11:44:30 -0600

> This is similar to another bug reported last month.
> Here is the patch I sent out then. Please let me know
> how it goes.
> 
> Signed-off-by: Joy Latten <latten@austin.ibm.com>

This whole interface is a complete mess.

Calling xfrm_audit_log() without the proper object being non-NULL
should be a bug.  And that's exactly what you fixed in the xfrm_user
case, so there is zero reason to silently allow this condition, we
should just BUG() on it.

But the logging function has this "result" thing, that in some cases
is set to 1 if "xp" or "x" is not-NULL by the callers, this is just
silly.

You can't log the event if the proper object is NULL, so the "result"
parameter and log information is useless in those cases.

Also, you missed the same exact identical bug in the AF_KEY code.

Thus, below is the patch I will use to fix this bug:

1) Calling xfrm_audit_log() with a NULL object is a BUG()
2) Setting "result" based upon NULL'ness of the object makes no
   sense, either set it to "1" in these cases or use an appropriate
   error check.

How does this look to others?

diff --git a/net/key/af_key.c b/net/key/af_key.c
index f3a026f..1c58204 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -2297,16 +2297,17 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, struct sadb_msg
 				   &sel, tmp.security, 1);
 	security_xfrm_policy_free(&tmp);
 
-	xfrm_audit_log(audit_get_loginuid(current->audit_context), 0,
-		       AUDIT_MAC_IPSEC_DELSPD, (xp) ? 1 : 0, xp, NULL);
-
 	if (xp == NULL)
 		return -ENOENT;
 
-	err = 0;
+	err = security_xfrm_policy_delete(xp);
 
-	if ((err = security_xfrm_policy_delete(xp)))
+	xfrm_audit_log(audit_get_loginuid(current->audit_context), 0,
+		       AUDIT_MAC_IPSEC_DELSPD, err ? 0 : 1, xp, NULL);
+
+	if (err)
 		goto out;
+
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
 	c.event = XFRM_MSG_DELPOLICY;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index a24f385..c394b41 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1997,9 +1997,14 @@ void xfrm_audit_log(uid_t auid, u32 sid, int type, int result,
 	if (audit_enabled == 0)
 		return;
 
+	BUG_ON((type == AUDIT_MAC_IPSEC_ADDSA ||
+		type == AUDIT_MAC_IPSEC_DELSA) && !x);
+	BUG_ON((type == AUDIT_MAC_IPSEC_ADDSPD ||
+		type == AUDIT_MAC_IPSEC_DELSPD) && !xp);
+
 	audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC, type);
 	if (audit_buf == NULL)
-	return;
+		return;
 
 	switch(type) {
 	case AUDIT_MAC_IPSEC_ADDSA:
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index d55436d..2567453 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1273,10 +1273,6 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 		xp = xfrm_policy_bysel_ctx(type, p->dir, &p->sel, tmp.security, delete);
 		security_xfrm_policy_free(&tmp);
 	}
-	if (delete)
-		xfrm_audit_log(NETLINK_CB(skb).loginuid, NETLINK_CB(skb).sid,
-			       AUDIT_MAC_IPSEC_DELSPD, (xp) ? 1 : 0, xp, NULL);
-
 	if (xp == NULL)
 		return -ENOENT;
 
@@ -1292,8 +1288,14 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 					      MSG_DONTWAIT);
 		}
 	} else {
-		if ((err = security_xfrm_policy_delete(xp)) != 0)
+		err = security_xfrm_policy_delete(xp);
+
+		xfrm_audit_log(NETLINK_CB(skb).loginuid, NETLINK_CB(skb).sid,
+			       AUDIT_MAC_IPSEC_DELSPD, err ? 0 : 1, xp, NULL);
+
+		if (err != 0)
 			goto out;
+
 		c.data.byid = p->index;
 		c.event = nlh->nlmsg_type;
 		c.seq = nlh->nlmsg_seq;

  parent reply	other threads:[~2007-02-12 21:46 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-12 17:44 Joy Latten
2007-02-12 20:50 ` [BUG] " David Miller
2007-02-12 21:04 ` Charles-Edouard Ruault
2007-02-12 21:46 ` David Miller [this message]
2007-02-13  1:02   ` James Morris
2007-02-15  8:22 ` Charles-Edouard Ruault
2007-02-26 10:36 ` Charles-Edouard Ruault
  -- strict thread matches above, loose matches on Subject: below --
2007-02-12 14:16 Charles-Edouard Ruault

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070212.134628.98748410.davem@davemloft.net \
    --to=davem@davemloft.net \
    --cc=ce@ruault.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=latten@austin.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-net@vger.kernel.org \
    --subject='Re: [BUG] 2.6.20 Oopses in xfrm_audit_log' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).