David Ahern <dsah...@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 <dsah...@gmail.com>
>>> Signed-off-by: Zhang Yu <zhangy...@baidu.com>
>>> Signed-off-by: Li RongQing <lirongq...@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
