2024-01-26 09:04:56

by Zhipeng Lu

Subject: [PATCH] net: ipv4: fix a memleak in ip_setup_cork

When inetdev_valid_mtu fails, cork->opt should be freed if it is
allocated in ip_setup_cork. Otherwise there could be a memleak.

Fixes: 501a90c94510 ("inet: protect against too small mtu values.")
Signed-off-by: Zhipeng Lu <[email protected]>
---
net/ipv4/ip_output.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index b06f678b03a1..3215ea07d398 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1282,6 +1282,7 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
{
struct ip_options_rcu *opt;
struct rtable *rt;
+ int free_opt = 0;

rt = *rtp;
if (unlikely(!rt))
@@ -1297,6 +1298,7 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
sk->sk_allocation);
if (unlikely(!cork->opt))
return -ENOBUFS;
+ free_opt = 1;
}
memcpy(cork->opt, &opt->opt, sizeof(struct ip_options) + opt->opt.optlen);
cork->flags |= IPCORK_OPT;
@@ -1306,8 +1308,13 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
cork->fragsize = ip_sk_use_pmtu(sk) ?
dst_mtu(&rt->dst) : READ_ONCE(rt->dst.dev->mtu);

- if (!inetdev_valid_mtu(cork->fragsize))
+ if (!inetdev_valid_mtu(cork->fragsize)) {
+ if (opt && free_opt) {
+ kfree(cork->opt);
+ cork->opt = NULL;
+ }
return -ENETUNREACH;
+ }

cork->gso_size = ipc->gso_size;

--
2.34.1

2024-01-26 11:26:09

by Eric Dumazet

Subject: Re: [PATCH] net: ipv4: fix a memleak in ip_setup_cork

On Fri, Jan 26, 2024 at 8:51 AM Zhipeng Lu <[email protected]> wrote:
>
> When inetdev_valid_mtu fails, cork->opt should be freed if it is
> allocated in ip_setup_cork. Otherwise there could be a memleak.
>
> Fixes: 501a90c94510 ("inet: protect against too small mtu values.")
> Signed-off-by: Zhipeng Lu <[email protected]>
> ---
> net/ipv4/ip_output.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index b06f678b03a1..3215ea07d398 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1282,6 +1282,7 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
> {
> struct ip_options_rcu *opt;
> struct rtable *rt;
> + int free_opt = 0;
>
> rt = *rtp;
> if (unlikely(!rt))
> @@ -1297,6 +1298,7 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
> sk->sk_allocation);
> if (unlikely(!cork->opt))
> return -ENOBUFS;
> + free_opt = 1;
> }
> memcpy(cork->opt, &opt->opt, sizeof(struct ip_options) + opt->opt.optlen);
> cork->flags |= IPCORK_OPT;
> @@ -1306,8 +1308,13 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
> cork->fragsize = ip_sk_use_pmtu(sk) ?
> dst_mtu(&rt->dst) : READ_ONCE(rt->dst.dev->mtu);
>
> - if (!inetdev_valid_mtu(cork->fragsize))
> + if (!inetdev_valid_mtu(cork->fragsize)) {
> + if (opt && free_opt) {
> + kfree(cork->opt);
> + cork->opt = NULL;
> + }
> return -ENETUNREACH;
> + }
>
> cork->gso_size = ipc->gso_size;
>
> --
> 2.34.1
>

What about something simpler like :

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index b06f678b03a19b806fd14764a4caad60caf02919..41537d18eecfd6e1163aacc35e047c22468e04e6
100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1287,6 +1287,12 @@ static int ip_setup_cork(struct sock *sk,
struct inet_cork *cork,
if (unlikely(!rt))
return -EFAULT;

+ cork->fragsize = ip_sk_use_pmtu(sk) ?
+ dst_mtu(&rt->dst) : READ_ONCE(rt->dst.dev->mtu);
+
+ if (!inetdev_valid_mtu(cork->fragsize))
+ return -ENETUNREACH;
+
/*
* setup for corking.
*/
@@ -1303,12 +1309,6 @@ static int ip_setup_cork(struct sock *sk,
struct inet_cork *cork,
cork->addr = ipc->addr;
}

- cork->fragsize = ip_sk_use_pmtu(sk) ?
- dst_mtu(&rt->dst) : READ_ONCE(rt->dst.dev->mtu);
-
- if (!inetdev_valid_mtu(cork->fragsize))
- return -ENETUNREACH;
-
cork->gso_size = ipc->gso_size;

cork->dst = &rt->dst;

2024-01-27 08:35:11

by Eric Dumazet

Subject: Re: [PATCH] net: ipv4: fix a memleak in ip_setup_cork

On Fri, Jan 26, 2024 at 11:13 AM Eric Dumazet <[email protected]> wrote:
>
> On Fri, Jan 26, 2024 at 8:51 AM Zhipeng Lu <[email protected]> wrote:
> >
> > When inetdev_valid_mtu fails, cork->opt should be freed if it is
> > allocated in ip_setup_cork. Otherwise there could be a memleak.
> >
> > Fixes: 501a90c94510 ("inet: protect against too small mtu values.")
> > Signed-off-by: Zhipeng Lu <[email protected]>
> > ---
> > net/ipv4/ip_output.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> > index b06f678b03a1..3215ea07d398 100644
> > --- a/net/ipv4/ip_output.c
> > +++ b/net/ipv4/ip_output.c
> > @@ -1282,6 +1282,7 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
> > {
> > struct ip_options_rcu *opt;
> > struct rtable *rt;
> > + int free_opt = 0;
> >
> > rt = *rtp;
> > if (unlikely(!rt))
> > @@ -1297,6 +1298,7 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
> > sk->sk_allocation);
> > if (unlikely(!cork->opt))
> > return -ENOBUFS;
> > + free_opt = 1;
> > }
> > memcpy(cork->opt, &opt->opt, sizeof(struct ip_options) + opt->opt.optlen);
> > cork->flags |= IPCORK_OPT;
> > @@ -1306,8 +1308,13 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
> > cork->fragsize = ip_sk_use_pmtu(sk) ?
> > dst_mtu(&rt->dst) : READ_ONCE(rt->dst.dev->mtu);
> >
> > - if (!inetdev_valid_mtu(cork->fragsize))
> > + if (!inetdev_valid_mtu(cork->fragsize)) {
> > + if (opt && free_opt) {
> > + kfree(cork->opt);
> > + cork->opt = NULL;
> > + }
> > return -ENETUNREACH;
> > + }
> >
> > cork->gso_size = ipc->gso_size;
> >
> > --
> > 2.34.1
> >
>
> What about something simpler like :
>
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index b06f678b03a19b806fd14764a4caad60caf02919..41537d18eecfd6e1163aacc35e047c22468e04e6
> 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1287,6 +1287,12 @@ static int ip_setup_cork(struct sock *sk,
> struct inet_cork *cork,
> if (unlikely(!rt))
> return -EFAULT;
>
> + cork->fragsize = ip_sk_use_pmtu(sk) ?
> + dst_mtu(&rt->dst) : READ_ONCE(rt->dst.dev->mtu);
> +
> + if (!inetdev_valid_mtu(cork->fragsize))
> + return -ENETUNREACH;
> +
> /*
> * setup for corking.
> */
> @@ -1303,12 +1309,6 @@ static int ip_setup_cork(struct sock *sk,
> struct inet_cork *cork,
> cork->addr = ipc->addr;
> }
>
> - cork->fragsize = ip_sk_use_pmtu(sk) ?
> - dst_mtu(&rt->dst) : READ_ONCE(rt->dst.dev->mtu);
> -
> - if (!inetdev_valid_mtu(cork->fragsize))
> - return -ENETUNREACH;
> -
> cork->gso_size = ipc->gso_size;
>
> cork->dst = &rt->dst;

Hi Zhipeng Lu

Could you send a V2 off your patch ? I will then add a Reviewed-by:
tag, thanks !

2024-01-29 09:10:55

by Zhipeng Lu

Subject: Re: [PATCH] net: ipv4: fix a memleak in ip_setup_cork

> On Fri, Jan 26, 2024 at 11:13 AM Eric Dumazet <[email protected]> wrote:
> >
> > On Fri, Jan 26, 2024 at 8:51 AM Zhipeng Lu <[email protected]> wrote:
> > >
> > > When inetdev_valid_mtu fails, cork->opt should be freed if it is
> > > allocated in ip_setup_cork. Otherwise there could be a memleak.
> > >
> > > Fixes: 501a90c94510 ("inet: protect against too small mtu values.")
> > > Signed-off-by: Zhipeng Lu <[email protected]>
> > > ---
> > > net/ipv4/ip_output.c | 9 ++++++++-
> > > 1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> > > index b06f678b03a1..3215ea07d398 100644
> > > --- a/net/ipv4/ip_output.c
> > > +++ b/net/ipv4/ip_output.c
> > > @@ -1282,6 +1282,7 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
> > > {
> > > struct ip_options_rcu *opt;
> > > struct rtable *rt;
> > > + int free_opt = 0;
> > >
> > > rt = *rtp;
> > > if (unlikely(!rt))
> > > @@ -1297,6 +1298,7 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
> > > sk->sk_allocation);
> > > if (unlikely(!cork->opt))
> > > return -ENOBUFS;
> > > + free_opt = 1;
> > > }
> > > memcpy(cork->opt, &opt->opt, sizeof(struct ip_options) + opt->opt.optlen);
> > > cork->flags |= IPCORK_OPT;
> > > @@ -1306,8 +1308,13 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
> > > cork->fragsize = ip_sk_use_pmtu(sk) ?
> > > dst_mtu(&rt->dst) : READ_ONCE(rt->dst.dev->mtu);
> > >
> > > - if (!inetdev_valid_mtu(cork->fragsize))
> > > + if (!inetdev_valid_mtu(cork->fragsize)) {
> > > + if (opt && free_opt) {
> > > + kfree(cork->opt);
> > > + cork->opt = NULL;
> > > + }
> > > return -ENETUNREACH;
> > > + }
> > >
> > > cork->gso_size = ipc->gso_size;
> > >
> > > --
> > > 2.34.1
> > >
> >
> > What about something simpler like :
> >
> > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> > index b06f678b03a19b806fd14764a4caad60caf02919..41537d18eecfd6e1163aacc35e047c22468e04e6
> > 100644
> > --- a/net/ipv4/ip_output.c
> > +++ b/net/ipv4/ip_output.c
> > @@ -1287,6 +1287,12 @@ static int ip_setup_cork(struct sock *sk,
> > struct inet_cork *cork,
> > if (unlikely(!rt))
> > return -EFAULT;
> >
> > + cork->fragsize = ip_sk_use_pmtu(sk) ?
> > + dst_mtu(&rt->dst) : READ_ONCE(rt->dst.dev->mtu);
> > +
> > + if (!inetdev_valid_mtu(cork->fragsize))
> > + return -ENETUNREACH;
> > +
> > /*
> > * setup for corking.
> > */
> > @@ -1303,12 +1309,6 @@ static int ip_setup_cork(struct sock *sk,
> > struct inet_cork *cork,
> > cork->addr = ipc->addr;
> > }
> >
> > - cork->fragsize = ip_sk_use_pmtu(sk) ?
> > - dst_mtu(&rt->dst) : READ_ONCE(rt->dst.dev->mtu);
> > -
> > - if (!inetdev_valid_mtu(cork->fragsize))
> > - return -ENETUNREACH;
> > -
> > cork->gso_size = ipc->gso_size;
> >
> > cork->dst = &rt->dst;
>
> Hi Zhipeng Lu
>
> Could you send a V2 off your patch ? I will then add a Reviewed-by:
> tag, thanks !

Hi Eric
Sure, I'll soon send a v2 version following your suggestion.