| Message ID | 1560500786-572-1-git-send-email-92siuyang@gmail.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | af_key: Fix memory leak in key_notify_policy. | expand |
On Fri, Jun 14, 2019 at 04:26:26PM +0800, Young Xiao wrote: > We leak the allocated out_skb in case pfkey_xfrm_policy2msg() fails. > Fix this by freeing it on error. > > Signed-off-by: Young Xiao <92siuyang@gmail.com> > --- > net/key/af_key.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/key/af_key.c b/net/key/af_key.c > index 4af1e1d..ec414f6 100644 > --- a/net/key/af_key.c > +++ b/net/key/af_key.c > @@ -2443,6 +2443,7 @@ static int key_pol_get_resp(struct sock *sk, struct xfrm_policy *xp, const struc > } > err = pfkey_xfrm_policy2msg(out_skb, xp, dir); > if (err < 0) > + kfree_skb(out_skb); > goto out; Did you test this? You need to add braces, otherwise 'goto out' will happen unconditionally. > > out_hdr = (struct sadb_msg *) out_skb->data; > @@ -2695,6 +2696,7 @@ static int dump_sp(struct xfrm_policy *xp, int dir, int count, void *ptr) > > err = pfkey_xfrm_policy2msg(out_skb, xp, dir); > if (err < 0) > + kfree_skb(out_skb); > return err; Same here.
On 2019-06-14, at 10:53:46 +0200, Steffen Klassert wrote: > On Fri, Jun 14, 2019 at 04:26:26PM +0800, Young Xiao wrote: > > We leak the allocated out_skb in case pfkey_xfrm_policy2msg() fails. > > Fix this by freeing it on error. > > > > Signed-off-by: Young Xiao <92siuyang@gmail.com> > > --- > > net/key/af_key.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/net/key/af_key.c b/net/key/af_key.c > > index 4af1e1d..ec414f6 100644 > > --- a/net/key/af_key.c > > +++ b/net/key/af_key.c > > @@ -2443,6 +2443,7 @@ static int key_pol_get_resp(struct sock *sk, struct xfrm_policy *xp, const struc > > } > > err = pfkey_xfrm_policy2msg(out_skb, xp, dir); > > if (err < 0) > > + kfree_skb(out_skb); > > goto out; > > Did you test this? > > You need to add braces, otherwise 'goto out' will happen unconditionally. > > > > > out_hdr = (struct sadb_msg *) out_skb->data; > > @@ -2695,6 +2696,7 @@ static int dump_sp(struct xfrm_policy *xp, int dir, int count, void *ptr) > > > > err = pfkey_xfrm_policy2msg(out_skb, xp, dir); > > if (err < 0) > > + kfree_skb(out_skb); > > return err; > > Same here. There's already a patch for this: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git/commit/?id=7c80eb1c7e2b8420477fbc998971d62a648035d9 J.
On 2019-06-14, at 10:59:22 +0100, Jeremy Sowden wrote: > On 2019-06-14, at 10:53:46 +0200, Steffen Klassert wrote: > > On Fri, Jun 14, 2019 at 04:26:26PM +0800, Young Xiao wrote: > > > We leak the allocated out_skb in case pfkey_xfrm_policy2msg() > > > fails. Fix this by freeing it on error. > > > > > > Signed-off-by: Young Xiao <92siuyang@gmail.com> > > > --- > > > net/key/af_key.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/net/key/af_key.c b/net/key/af_key.c > > > index 4af1e1d..ec414f6 100644 > > > --- a/net/key/af_key.c > > > +++ b/net/key/af_key.c > > > @@ -2443,6 +2443,7 @@ static int key_pol_get_resp(struct sock *sk, struct xfrm_policy *xp, const struc > > > } > > > err = pfkey_xfrm_policy2msg(out_skb, xp, dir); > > > if (err < 0) > > > + kfree_skb(out_skb); > > > goto out; > > > > Did you test this? > > > > You need to add braces, otherwise 'goto out' will happen unconditionally. > > > > > > > > out_hdr = (struct sadb_msg *) out_skb->data; > > > @@ -2695,6 +2696,7 @@ static int dump_sp(struct xfrm_policy *xp, int dir, int count, void *ptr) > > > > > > err = pfkey_xfrm_policy2msg(out_skb, xp, dir); > > > if (err < 0) > > > + kfree_skb(out_skb); > > > return err; > > > > Same here. > > There's already a patch for this: > > https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git/commit/?id=7c80eb1c7e2b8420477fbc998971d62a648035d9 That reminds me. Stephen Rothwell reported a problem with the "Fixes:" tag: On 2019-05-29, at 07:48:12 +1000, Stephen Rothwell wrote: > In commit > > 7c80eb1c7e2b ("af_key: fix leaks in key_pol_get_resp and dump_sp.") > > Fixes tag > > Fixes: 55569ce256ce ("Fix conversion between IPSEC_MODE_xxx and XFRM_MODE_xxx.") > > has these problem(s): > > - Subject does not match target commit subject > Just use > git log -1 --format='Fixes: %h ("%s")' What's the procedure for fixing this sort of thing? Do you need me to do anything? J.
On Fri, Jun 14, 2019 at 11:13:38AM +0100, Jeremy Sowden wrote: > > That reminds me. Stephen Rothwell reported a problem with the "Fixes:" > tag: > > On 2019-05-29, at 07:48:12 +1000, Stephen Rothwell wrote: > > In commit > > > > 7c80eb1c7e2b ("af_key: fix leaks in key_pol_get_resp and dump_sp.") > > > > Fixes tag > > > > Fixes: 55569ce256ce ("Fix conversion between IPSEC_MODE_xxx and XFRM_MODE_xxx.") > > > > has these problem(s): > > > > - Subject does not match target commit subject > > Just use > > git log -1 --format='Fixes: %h ("%s")' > > What's the procedure for fixing this sort of thing? Do you need me to > do anything? No, that patch is already commited. We can't change the commit message anymore. Just keep it in mind for next time.
On 2019-06-14, at 12:17:08 +0200, Steffen Klassert wrote: > On Fri, Jun 14, 2019 at 11:13:38AM +0100, Jeremy Sowden wrote: > > That reminds me. Stephen Rothwell reported a problem with the > > "Fixes:" tag: > > > > On 2019-05-29, at 07:48:12 +1000, Stephen Rothwell wrote: > > > In commit > > > > > > 7c80eb1c7e2b ("af_key: fix leaks in key_pol_get_resp and dump_sp.") > > > > > > Fixes tag > > > > > > Fixes: 55569ce256ce ("Fix conversion between IPSEC_MODE_xxx and XFRM_MODE_xxx.") > > > > > > has these problem(s): > > > > > > - Subject does not match target commit subject > > > Just use > > > git log -1 --format='Fixes: %h ("%s")' > > > > What's the procedure for fixing this sort of thing? Do you need me > > to do anything? > > No, that patch is already commited. We can't change the commit message > anymore. Just keep it in mind for next time. Will do. I've already made a note of that git-log invocation. Useful to know. J.
diff --git a/net/key/af_key.c b/net/key/af_key.c index 4af1e1d..ec414f6 100644 --- a/net/key/af_key.c +++ b/net/key/af_key.c @@ -2443,6 +2443,7 @@ static int key_pol_get_resp(struct sock *sk, struct xfrm_policy *xp, const struc } err = pfkey_xfrm_policy2msg(out_skb, xp, dir); if (err < 0) + kfree_skb(out_skb); goto out; out_hdr = (struct sadb_msg *) out_skb->data; @@ -2695,6 +2696,7 @@ static int dump_sp(struct xfrm_policy *xp, int dir, int count, void *ptr) err = pfkey_xfrm_policy2msg(out_skb, xp, dir); if (err < 0) + kfree_skb(out_skb); return err; out_hdr = (struct sadb_msg *) out_skb->data;
We leak the allocated out_skb in case pfkey_xfrm_policy2msg() fails. Fix this by freeing it on error. Signed-off-by: Young Xiao <92siuyang@gmail.com> --- net/key/af_key.c | 2 ++ 1 file changed, 2 insertions(+)