| Message ID | 1540968178-18894-1-git-send-email-alexey.kodanev@oracle.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset | expand |
On 31.10.2018 09:42, Alexey Kodanev wrote: > cb->args[2] can store the pointer to the struct fib6_walker, > allocated in inet6_dump_fib(). On the next loop iteration in > rtnl_dump_all(), 'memset(&cb, 0, sizeof(cb->args))' can reset > that pointer, leaking the memory [1]. > On the second thought we could as well save the state of fib6_walker in inet6_dump_fib() with cb->data. That should fix the leak too. Is it sounds reasonable? Thanks, Alexey > Fix it by calling cb->done, if it is set, before filling 'cb->args' > with zeros. > > Looks like the recent changes in rtnl_dump_all() contributed to > the appearance of this kmemleak [1], commit c63586dc9b3e ("net: > rtnl_dump_all needs to propagate error from dumpit function") > breaks the loop only on an error now. > > [1]: > unreferenced object 0xffff88001322a200 (size 96): > comm "sshd", pid 1484, jiffies 4296032768 (age 1432.542s) > hex dump (first 32 bytes): > 00 01 00 00 00 00 ad de 00 02 00 00 00 00 ad de ................ > 18 09 41 36 00 88 ff ff 18 09 41 36 00 88 ff ff ..A6......A6.... > backtrace: > [<0000000095846b39>] kmem_cache_alloc_trace+0x151/0x220 > [<000000007d12709f>] inet6_dump_fib+0x68d/0x940 > [<000000002775a316>] rtnl_dump_all+0x1d9/0x2d0 > [<00000000d7cd302b>] netlink_dump+0x945/0x11a0 > [<000000002f43485f>] __netlink_dump_start+0x55d/0x800 > [<00000000f76bbeec>] rtnetlink_rcv_msg+0x4fa/0xa00 > [<000000009b5761f3>] netlink_rcv_skb+0x29c/0x420 > [<0000000087a1dae1>] rtnetlink_rcv+0x15/0x20 > [<00000000691b703b>] netlink_unicast+0x4e3/0x6c0 > [<00000000b5be0204>] netlink_sendmsg+0x7f2/0xba0 > [<0000000096d2aa60>] sock_sendmsg+0xba/0xf0 > [<000000008c1b786f>] __sys_sendto+0x1e4/0x330 > [<0000000019587b3f>] __x64_sys_sendto+0xe1/0x1a0 > [<00000000071f4d56>] do_syscall_64+0x9f/0x300 > [<000000002737577f>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > [<0000000057587684>] 0xffffffffffffffff > > Fixes: 1b43af5480c3 ("[IPV6]: Increase number of possible routing tables to 2^32") > Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com> > --- > net/core/rtnetlink.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index f679c7a..314c683 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -3362,6 +3362,8 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb) > continue; > > if (idx > s_idx) { > + if (cb->done) > + cb->done(cb); > memset(&cb->args[0], 0, sizeof(cb->args)); > cb->prev_seq = 0; > cb->seq = 0; >
On 10/31/18 12:42 AM, Alexey Kodanev wrote: > cb->args[2] can store the pointer to the struct fib6_walker, > allocated in inet6_dump_fib(). On the next loop iteration in > rtnl_dump_all(), 'memset(&cb, 0, sizeof(cb->args))' can reset > that pointer, leaking the memory [1]. > > Fix it by calling cb->done, if it is set, before filling 'cb->args' > with zeros. > > Looks like the recent changes in rtnl_dump_all() contributed to > the appearance of this kmemleak [1], commit c63586dc9b3e ("net: > rtnl_dump_all needs to propagate error from dumpit function") > breaks the loop only on an error now. > ... It is more efficient to keep going. I think the simplest fix for 4.20 is to break the loop if ret is non-0 - restore the previous behavior. For net-next I think the done callback is not needed for ipv6; I think there is a simpler way to do it.
On 10/31/18 10:55 AM, David Ahern wrote: > I think the simplest fix for 4.20 is to break the loop if ret is non-0 - > restore the previous behavior. that is the only recourse. It has to bail if ret is non-0. Do you want to send a patch with that fix?
On 10/31/2018 08:35 PM, David Ahern wrote: > On 10/31/18 10:55 AM, David Ahern wrote: >> I think the simplest fix for 4.20 is to break the loop if ret is non-0 - >> restore the previous behavior. > > that is the only recourse. It has to bail if ret is non-0. Do you want > to send a patch with that fix? > I see, and inet6_dump_fib() cleanups fib6_walker if ret is zero. Will send the fix.
On 11/01/2018 04:11 PM, Alexey Kodanev wrote: > On 10/31/2018 08:35 PM, David Ahern wrote: >> On 10/31/18 10:55 AM, David Ahern wrote: >>> I think the simplest fix for 4.20 is to break the loop if ret is non-0 - >>> restore the previous behavior. >> >> that is the only recourse. It has to bail if ret is non-0. Do you want >> to send a patch with that fix? >> > > I see, and inet6_dump_fib() cleanups fib6_walker if ret is zero. Will send the fix. Can it happen that inet6_dump_fib() returns skb->len (0) in the below cases? * if (arg.filter.flags & RTM_F_CLONED) return skb->len; ... w = (void *)cb->args[2]; if (!w) { ... w = kzalloc(...) ... * if (arg.filter.table_id) { ... if (!tb) { if (arg.filter.dump_all_families) return skb->len; Would it be safer to add "res = skb->len; goto out;" instead of "return skb->len;" so that it can call fib6_dump_end() for "res <= 0"? Or use cb->data instead of cb->args?
On 11/1/18 7:42 AM, Alexey Kodanev wrote: > On 11/01/2018 04:11 PM, Alexey Kodanev wrote: >> On 10/31/2018 08:35 PM, David Ahern wrote: >>> On 10/31/18 10:55 AM, David Ahern wrote: >>>> I think the simplest fix for 4.20 is to break the loop if ret is non-0 - >>>> restore the previous behavior. >>> >>> that is the only recourse. It has to bail if ret is non-0. Do you want >>> to send a patch with that fix? >>> >> >> I see, and inet6_dump_fib() cleanups fib6_walker if ret is zero. Will send the fix. > > Can it happen that inet6_dump_fib() returns skb->len (0) in the below cases? > > * if (arg.filter.flags & RTM_F_CLONED) > return skb->len; > > ... > > w = (void *)cb->args[2]; > if (!w) { > ... > w = kzalloc(...) > ... > > * if (arg.filter.table_id) { > ... > if (!tb) { > if (arg.filter.dump_all_families) > return skb->len; > > > Would it be safer to add "res = skb->len; goto out;" instead of "return skb->len;" > so that it can call fib6_dump_end() for "res <= 0"? Or use cb->data instead of > cb->args? > Since res is initialized to 0, both of those can just be 'goto out;' The break in dump_all is still needed though.
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index f679c7a..314c683 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -3362,6 +3362,8 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb) continue; if (idx > s_idx) { + if (cb->done) + cb->done(cb); memset(&cb->args[0], 0, sizeof(cb->args)); cb->prev_seq = 0; cb->seq = 0;
cb->args[2] can store the pointer to the struct fib6_walker, allocated in inet6_dump_fib(). On the next loop iteration in rtnl_dump_all(), 'memset(&cb, 0, sizeof(cb->args))' can reset that pointer, leaking the memory [1]. Fix it by calling cb->done, if it is set, before filling 'cb->args' with zeros. Looks like the recent changes in rtnl_dump_all() contributed to the appearance of this kmemleak [1], commit c63586dc9b3e ("net: rtnl_dump_all needs to propagate error from dumpit function") breaks the loop only on an error now. [1]: unreferenced object 0xffff88001322a200 (size 96): comm "sshd", pid 1484, jiffies 4296032768 (age 1432.542s) hex dump (first 32 bytes): 00 01 00 00 00 00 ad de 00 02 00 00 00 00 ad de ................ 18 09 41 36 00 88 ff ff 18 09 41 36 00 88 ff ff ..A6......A6.... backtrace: [<0000000095846b39>] kmem_cache_alloc_trace+0x151/0x220 [<000000007d12709f>] inet6_dump_fib+0x68d/0x940 [<000000002775a316>] rtnl_dump_all+0x1d9/0x2d0 [<00000000d7cd302b>] netlink_dump+0x945/0x11a0 [<000000002f43485f>] __netlink_dump_start+0x55d/0x800 [<00000000f76bbeec>] rtnetlink_rcv_msg+0x4fa/0xa00 [<000000009b5761f3>] netlink_rcv_skb+0x29c/0x420 [<0000000087a1dae1>] rtnetlink_rcv+0x15/0x20 [<00000000691b703b>] netlink_unicast+0x4e3/0x6c0 [<00000000b5be0204>] netlink_sendmsg+0x7f2/0xba0 [<0000000096d2aa60>] sock_sendmsg+0xba/0xf0 [<000000008c1b786f>] __sys_sendto+0x1e4/0x330 [<0000000019587b3f>] __x64_sys_sendto+0xe1/0x1a0 [<00000000071f4d56>] do_syscall_64+0x9f/0x300 [<000000002737577f>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [<0000000057587684>] 0xffffffffffffffff Fixes: 1b43af5480c3 ("[IPV6]: Increase number of possible routing tables to 2^32") Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com> --- net/core/rtnetlink.c | 2 ++ 1 file changed, 2 insertions(+)