| Message ID | 20240126075127.2825068-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipv4: fix a memleak in ip_setup_cork | expand |
On Fri, Jan 26, 2024 at 8:51 AM Zhipeng Lu <alexious@zju.edu.cn> 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 <alexious@zju.edu.cn> > --- > 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;
On Fri, Jan 26, 2024 at 11:13 AM Eric Dumazet <edumazet@google.com> wrote: > > On Fri, Jan 26, 2024 at 8:51 AM Zhipeng Lu <alexious@zju.edu.cn> 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 <alexious@zju.edu.cn> > > --- > > 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 !
> On Fri, Jan 26, 2024 at 11:13 AM Eric Dumazet <edumazet@google.com> wrote: > > > > On Fri, Jan 26, 2024 at 8:51 AM Zhipeng Lu <alexious@zju.edu.cn> 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 <alexious@zju.edu.cn> > > > --- > > > 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.
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;
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 <alexious@zju.edu.cn> --- net/ipv4/ip_output.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)