netdev - Re: [IPSEC] flow: Cache negative results

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20070110232713.GA13180@gondor.apana.org.au>
Date:	Thu, 11 Jan 2007 10:27:13 +1100
From:	Herbert Xu <herbert.xu@...hat.com>
To:	Venkat Yekkirala <vyekkirala@...stedcs.com>
Cc:	'James Morris' <jmorris@...ei.org>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Paul Moore <paul.moore@...com>
Subject: Re: [IPSEC] flow: Cache negative results

On Wed, Jan 10, 2007 at 03:08:55PM -0600, Venkat Yekkirala wrote:
> 
> I was talking about this (the latter) as well. Currently, on a proper
> "negative", -ESRCH is returned by security_xfrm_policy_lookup(), and
> this comes back up as a 0 from resolver(), correctly indicating NO
> applicable
> xfrm policy (after taking security into account). But if
> security_xfrm_policy_lookup()
> were to return anything other than a zero or -ESRCH, such as -ENOMEM,
> you will see it come back up as such (as -ENOMEM) from resolver(),
> and in this case, it's neither a positive nor a negative, just an error.
> Hence a full lookup would be in order, the next time round.

OK, I see that now.  The EACCES error is translated to ESRCH in
the security layer.

> Also, I would fix the other bug you had noted, by something like:

I agree.

[IPSEC] flow: Fix potential memory leak

When old flow cache entries that are not at the head of their chain
trigger a transient security error they get unlinked along with all
the entries preceding them in the chain.  The preceding entries are
not freed correctly.

This patch fixes this by simply leaving the entry around.  It's based
on a suggestion by Venkat Yekkirala.

Signed-off-by: Herbert Xu <herbert@...dor.apana.org.au>
 
> I am planning to test and submit a patch to SELinux
> to invoke flow_cache_flush() on policy reloads tomorrow.

flow_cache_flush() shouldn't be used by SELinux.  You probably
want to increase the genid instead.

BTW, we should probably add some code to deal with overflows in
genid.  With the recent improvements in policy update speeds, it
could be conceivable for an overflow to occur.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/core/flow.c b/net/core/flow.c
index d137f97..5d25697 100644
--- a/net/core/flow.c
+++ b/net/core/flow.c
@@ -231,22 +231,16 @@ nocache:
 
 		err = resolver(key, family, dir, &obj, &obj_ref);
 
-		if (fle) {
-			if (err) {
-				/* Force security policy check on next lookup */
-				*head = fle->next;
-				flow_entry_kill(cpu, fle);
-			} else {
-				fle->genid = atomic_read(&flow_cache_genid);
-
-				if (fle->object)
-					atomic_dec(fle->object_ref);
-
-				fle->object = obj;
-				fle->object_ref = obj_ref;
-				if (obj)
-					atomic_inc(fle->object_ref);
-			}
+		if (fle && !err) {
+			fle->genid = atomic_read(&flow_cache_genid);
+
+			if (fle->object)
+				atomic_dec(fle->object_ref);
+
+			fle->object = obj;
+			fle->object_ref = obj_ref;
+			if (obj)
+				atomic_inc(fle->object_ref);
 		}
 		local_bh_enable();
 
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html