netdev - Re: [net] xfrm_user: use xfrm_state_put to free xfrm_state

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: <20181121200923.GA12460@jig.fritz.box>
Date:   Wed, 21 Nov 2018 21:09:23 +0100
From:   Mathias Krause <minipli@...glemail.com>
To:     Herbert Xu <herbert@...dor.apana.org.au>,
        Steffen Klassert <steffen.klassert@...unet.com>
Cc:     Pan Bian <bianpan2016@....com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Pan Bian <bianpan2013@....com>,
        minipli@...glemail.com
Subject: Re: [net] xfrm_user: use xfrm_state_put to free xfrm_state_alloc
 return value

On Wed, Nov 21, 2018 at 04:00:45PM +0800, Herbert Xu wrote:
> On Wed, Nov 21, 2018 at 02:57:48PM +0800, Pan Bian wrote:
> > From: Pan Bian <bianpan2013@....com>
> > 
> > The memory chunk allocated by xfrm_state_alloc() should be released with
> > xfrm_state_put(), not kfree.
> > 
> > Signed-off-by: Pan Bian <bianpan2013@....com>
> 
> This bug was introduced by
> 
> 	commit 565f0fa902b64020d5d147ff1708567e9e0b6e49
> 	Author: Mathias Krause <minipli@...glemail.com>
> 	Date:   Thu May 3 10:55:07 2018 +0200
> 

Oh, snap. You're totally right. I missed the kfree() in xfrm_user.c.
Sorry for that!

> While using xfrm_state_put may work it's certainly not the designed
> to do this.  We should instead export a function that calls
> kmem_cache_free on xfrm_state directly and use that here.

Maybe something like the below patch? Steffen?

-- >8 --

Subject: [PATCH] xfrm_user: fix freeing of xfrm states on acquire

Commit 565f0fa902b6 ("xfrm: use a dedicated slab cache for struct
xfrm_state") moved xfrm state objects to use their own slab cache.
However, it missed to adapt xfrm_user to use this new cache when
freeing xfrm states.

Fix this by introducing and make use of a new helper for freeing
xfrm_state objects.

Fixes: 565f0fa902b6 ("xfrm: use a dedicated slab cache for struct xfrm_state")
Reported-by: Pan Bian <bianpan2016@....com>
Cc: <stable@...r.kernel.org> # v4.18+
Signed-off-by: Mathias Krause <minipli@...glemail.com>
---
 include/net/xfrm.h    | 1 +
 net/xfrm/xfrm_state.c | 8 +++++++-
 net/xfrm/xfrm_user.c  | 4 ++--
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 0eb390c205af..da588def3c61 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1552,6 +1552,7 @@ int xfrm_state_walk(struct net *net, struct xfrm_state_walk *walk,
 		    int (*func)(struct xfrm_state *, int, void*), void *);
 void xfrm_state_walk_done(struct xfrm_state_walk *walk, struct net *net);
 struct xfrm_state *xfrm_state_alloc(struct net *net);
+void xfrm_state_free(struct xfrm_state *x);
 struct xfrm_state *xfrm_state_find(const xfrm_address_t *daddr,
 				   const xfrm_address_t *saddr,
 				   const struct flowi *fl,
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index dc4a9f1fb941..0a0b01b688d7 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -426,6 +426,12 @@ static void xfrm_put_mode(struct xfrm_mode *mode)
 	module_put(mode->owner);
 }
 
+void xfrm_state_free(struct xfrm_state *x)
+{
+	kmem_cache_free(xfrm_state_cache, x);
+}
+EXPORT_SYMBOL(xfrm_state_free);
+
 static void xfrm_state_gc_destroy(struct xfrm_state *x)
 {
 	tasklet_hrtimer_cancel(&x->mtimer);
@@ -452,7 +458,7 @@ static void xfrm_state_gc_destroy(struct xfrm_state *x)
 	}
 	xfrm_dev_state_free(x);
 	security_xfrm_state_free(x);
-	kmem_cache_free(xfrm_state_cache, x);
+	xfrm_state_free(x);
 }
 
 static void xfrm_state_gc_task(struct work_struct *work)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index c9a84e22f5d5..277c1c46fe94 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2288,13 +2288,13 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	}
 
-	kfree(x);
+	xfrm_state_free(x);
 	kfree(xp);
 
 	return 0;
 
 free_state:
-	kfree(x);
+	xfrm_state_free(x);
 nomem:
 	return err;
 }