| Message ID | 1540373788-15078-1-git-send-email-lirongqing@baidu.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net/ipv4: fix a net leak | expand |
On 10/24/18 3:36 AM, Li RongQing wrote: > put net when input a invalid ifindex, otherwise it will be leaked > > Fixes: 5fcd266a9f64("net/ipv4: Add support for dumping addresses for a specific device") > Cc: David Ahern <dsahern@gmail.com> > Signed-off-by: Zhang Yu <zhangyu31@baidu.com> > Signed-off-by: Li RongQing <lirongqing@baidu.com> > --- > net/ipv4/devinet.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c > index 63d5b58fbfdb..fd0c5a47e742 100644 > --- a/net/ipv4/devinet.c > +++ b/net/ipv4/devinet.c > @@ -1775,8 +1775,10 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) > > if (fillargs.ifindex) { > dev = __dev_get_by_index(tgt_net, fillargs.ifindex); > - if (!dev) > + if (!dev) { > + put_net(tgt_net); > return -ENODEV; > + } > > in_dev = __in_dev_get_rtnl(dev); > if (in_dev) { > Good catch. IPv6 has the same problem. Will fix that one. Reviewed-by: David Ahern <dsahern@gmail.com>
On 10/24/18 9:02 AM, David Ahern wrote: > On 10/24/18 3:36 AM, Li RongQing wrote: >> put net when input a invalid ifindex, otherwise it will be leaked >> >> Fixes: 5fcd266a9f64("net/ipv4: Add support for dumping addresses for a specific device") >> Cc: David Ahern <dsahern@gmail.com> >> Signed-off-by: Zhang Yu <zhangyu31@baidu.com> >> Signed-off-by: Li RongQing <lirongqing@baidu.com> >> --- >> net/ipv4/devinet.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c >> index 63d5b58fbfdb..fd0c5a47e742 100644 >> --- a/net/ipv4/devinet.c >> +++ b/net/ipv4/devinet.c >> @@ -1775,8 +1775,10 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) >> >> if (fillargs.ifindex) { >> dev = __dev_get_by_index(tgt_net, fillargs.ifindex); >> - if (!dev) >> + if (!dev) { >> + put_net(tgt_net); >> return -ENODEV; >> + } >> >> in_dev = __in_dev_get_rtnl(dev); >> if (in_dev) { >> > > Good catch. IPv6 has the same problem. Will fix that one. > Actually remove that 'Reviewed-by'. You should only call put_net if (fillargs.netnsid >= 0) DaveM: just want to call this out since I mistakenly added the Reviewed-by. This patch should be dropped.
David Ahern <dsahern@gmail.com> writes: > On 10/24/18 9:02 AM, David Ahern wrote: >> On 10/24/18 3:36 AM, Li RongQing wrote: >>> put net when input a invalid ifindex, otherwise it will be leaked >>> >>> Fixes: 5fcd266a9f64("net/ipv4: Add support for dumping addresses for a specific device") >>> Cc: David Ahern <dsahern@gmail.com> >>> Signed-off-by: Zhang Yu <zhangyu31@baidu.com> >>> Signed-off-by: Li RongQing <lirongqing@baidu.com> >>> --- >>> net/ipv4/devinet.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c >>> index 63d5b58fbfdb..fd0c5a47e742 100644 >>> --- a/net/ipv4/devinet.c >>> +++ b/net/ipv4/devinet.c >>> @@ -1775,8 +1775,10 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) >>> >>> if (fillargs.ifindex) { >>> dev = __dev_get_by_index(tgt_net, fillargs.ifindex); >>> - if (!dev) >>> + if (!dev) { >>> + put_net(tgt_net); >>> return -ENODEV; >>> + } >>> >>> in_dev = __in_dev_get_rtnl(dev); >>> if (in_dev) { >>> >> >> Good catch. IPv6 has the same problem. Will fix that one. >> > Actually remove that 'Reviewed-by'. You should only call put_net if > (fillargs.netnsid >= 0) > > DaveM: just want to call this out since I mistakenly added the > Reviewed-by. This patch should be dropped. Hmm, I see that you implemented that. But I believe it's still buggy if called with an invalid netnsid. inet_valid_dump_ifaddr_req() will bail out with an error, but only *after* setting fillargs->netnsid: if (i == IFA_TARGET_NETNSID) { struct net *net; fillargs->netnsid = nla_get_s32(tb[i]); net = rtnl_get_net_ns_capable(sk, fillargs->netnsid); if (IS_ERR(net)) { NL_SET_ERR_MSG(extack, "ipv4: Invalid target network namespace id"); return PTR_ERR(net); } *tgt_net = net; } else { So inet_dump_ifaddr() ends up doing put_net(tgt_net): err = inet_valid_dump_ifaddr_req(nlh, &fillargs, &tgt_net, skb->sk, cb); if (err < 0) goto put_tgt_net; .. put_tgt_net: if (fillargs.netnsid >= 0) put_net(tgt_net); I believe you should set fillargs->netnsid back to -1 in the inet_valid_dump_ifaddr_req() error path, or use a temp variable to avoid changing it unless get_net is successful. Bjørn
On 10/25/18 12:43 PM, Bjørn Mork wrote: > > inet_valid_dump_ifaddr_req() will bail out with an error, but only > *after* setting fillargs->netnsid: > > if (i == IFA_TARGET_NETNSID) { > struct net *net; > > fillargs->netnsid = nla_get_s32(tb[i]); > > net = rtnl_get_net_ns_capable(sk, fillargs->netnsid); > if (IS_ERR(net)) { > NL_SET_ERR_MSG(extack, "ipv4: Invalid target network namespace id"); > return PTR_ERR(net); > } > *tgt_net = net; > } else { > > > > So inet_dump_ifaddr() ends up doing put_net(tgt_net): > > > err = inet_valid_dump_ifaddr_req(nlh, &fillargs, &tgt_net, > skb->sk, cb); > if (err < 0) > goto put_tgt_net; > .. > put_tgt_net: > if (fillargs.netnsid >= 0) > put_net(tgt_net); > > > > I believe you should set fillargs->netnsid back to -1 in the > inet_valid_dump_ifaddr_req() error path, or use a temp variable to avoid > changing it unless get_net is successful. good point. either use of an intermediate or resetting nsid on failure. Will you send a patch to fix ipv4 and v6? Thanks,
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index 63d5b58fbfdb..fd0c5a47e742 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -1775,8 +1775,10 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) if (fillargs.ifindex) { dev = __dev_get_by_index(tgt_net, fillargs.ifindex); - if (!dev) + if (!dev) { + put_net(tgt_net); return -ENODEV; + } in_dev = __in_dev_get_rtnl(dev); if (in_dev) {
put net when input a invalid ifindex, otherwise it will be leaked Fixes: 5fcd266a9f64("net/ipv4: Add support for dumping addresses for a specific device") Cc: David Ahern <dsahern@gmail.com> Signed-off-by: Zhang Yu <zhangyu31@baidu.com> Signed-off-by: Li RongQing <lirongqing@baidu.com> --- net/ipv4/devinet.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)