* [PATCH net] net: nexthop: fix null pointer dereference when IPv6 is not enabled
@ 2021-11-23 10:27 Nikolay Aleksandrov
2021-11-23 11:09 ` Ido Schimmel
2021-11-23 11:50 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 5+ messages in thread
From: Nikolay Aleksandrov @ 2021-11-23 10:27 UTC (permalink / raw)
To: netdev; +Cc: idosch, davem, kuba, dsahern, Nikolay Aleksandrov, stable
From: Nikolay Aleksandrov <nikolay@nvidia.com>
When we try to add an IPv6 nexthop and IPv6 is not enabled
(!CONFIG_IPV6) we'll hit a NULL pointer dereference[1] in the error path
of nh_create_ipv6() due to calling ipv6_stub->fib6_nh_release. The bug
has been present since the beginning of IPv6 nexthop gateway support.
Commit 1aefd3de7bc6 ("ipv6: Add fib6_nh_init and release to stubs") tells
us that only fib6_nh_init has a dummy stub because fib6_nh_release should
not be called if fib6_nh_init returns an error, but the commit below added
a call to ipv6_stub->fib6_nh_release in its error path. To fix it return
the dummy stub's -EAFNOSUPPORT error directly without calling
ipv6_stub->fib6_nh_release in nh_create_ipv6()'s error path.
[1]
Output is a bit truncated, but it clearly shows the error.
BUG: kernel NULL pointer dereference, address: 000000000000000000
#PF: supervisor instruction fetch in kernel modede
#PF: error_code(0x0010) - not-present pagege
PGD 0 P4D 0
Oops: 0010 [#1] PREEMPT SMP NOPTI
CPU: 4 PID: 638 Comm: ip Kdump: loaded Not tainted 5.16.0-rc1+ #446
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-4.fc34 04/01/2014
RIP: 0010:0x0
Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
RSP: 0018:ffff888109f5b8f0 EFLAGS: 00010286^Ac
RAX: 0000000000000000 RBX: ffff888109f5ba28 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8881008a2860
RBP: ffff888109f5b9d8 R08: 0000000000000000 R09: 0000000000000000
R10: ffff888109f5b978 R11: ffff888109f5b948 R12: 00000000ffffff9f
R13: ffff8881008a2a80 R14: ffff8881008a2860 R15: ffff8881008a2840
FS: 00007f98de70f100(0000) GS:ffff88822bf00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffd6 CR3: 0000000100efc000 CR4: 00000000000006e0
Call Trace:
<TASK>
nh_create_ipv6+0xed/0x10c
rtm_new_nexthop+0x6d7/0x13f3
? check_preemption_disabled+0x3d/0xf2
? lock_is_held_type+0xbe/0xfd
rtnetlink_rcv_msg+0x23f/0x26a
? check_preemption_disabled+0x3d/0xf2
? rtnl_calcit.isra.0+0x147/0x147
netlink_rcv_skb+0x61/0xb2
netlink_unicast+0x100/0x187
netlink_sendmsg+0x37f/0x3a0
? netlink_unicast+0x187/0x187
sock_sendmsg_nosec+0x67/0x9b
____sys_sendmsg+0x19d/0x1f9
? copy_msghdr_from_user+0x4c/0x5e
? rcu_read_lock_any_held+0x2a/0x78
___sys_sendmsg+0x6c/0x8c
? asm_sysvec_apic_timer_interrupt+0x12/0x20
? lockdep_hardirqs_on+0xd9/0x102
? sockfd_lookup_light+0x69/0x99
__sys_sendmsg+0x50/0x6e
do_syscall_64+0xcb/0xf2
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f98dea28914
Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b5 0f 1f 80 00 00 00 00 48 8d 05 e9 5d 0c 00 8b 00 85 c0 75 13 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 41 89 d4 55 48 89 f5 53
RSP: 002b:00007fff859f5e68 EFLAGS: 00000246 ORIG_RAX: 000000000000002e2e
RAX: ffffffffffffffda RBX: 00000000619cb810 RCX: 00007f98dea28914
RDX: 0000000000000000 RSI: 00007fff859f5ed0 RDI: 0000000000000003
RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000008
R10: fffffffffffffce6 R11: 0000000000000246 R12: 0000000000000001
R13: 000055c0097ae520 R14: 000055c0097957fd R15: 00007fff859f63a0
</TASK>
Modules linked in: bridge stp llc bonding virtio_net
Cc: stable@vger.kernel.org
Fixes: 53010f991a9f ("nexthop: Add support for IPv6 gateways")
Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com>
---
I found this while testing my recent nexthop fixes, alternatively we can
add a fib6_nh_release dummy stub. I don't have strong preference so if
anyone prefers it just let me know. This fix is smaller for backports,
that's why I went with it.
net/ipv4/nexthop.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index a69a9e76f99f..5dbd4b5505eb 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -2565,11 +2565,15 @@ static int nh_create_ipv6(struct net *net, struct nexthop *nh,
/* sets nh_dev if successful */
err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
extack);
- if (err)
+ if (err) {
+ /* IPv6 is not enabled, don't call fib6_nh_release */
+ if (err == -EAFNOSUPPORT)
+ goto out;
ipv6_stub->fib6_nh_release(fib6_nh);
- else
+ } else {
nh->nh_flags = fib6_nh->fib_nh_flags;
-
+ }
+out:
return err;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: nexthop: fix null pointer dereference when IPv6 is not enabled
2021-11-23 10:27 [PATCH net] net: nexthop: fix null pointer dereference when IPv6 is not enabled Nikolay Aleksandrov
@ 2021-11-23 11:09 ` Ido Schimmel
2021-11-23 11:33 ` Nikolay Aleksandrov
2021-11-23 11:50 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 5+ messages in thread
From: Ido Schimmel @ 2021-11-23 11:09 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, davem, kuba, dsahern, Nikolay Aleksandrov, stable
On Tue, Nov 23, 2021 at 12:27:19PM +0200, Nikolay Aleksandrov wrote:
> From: Nikolay Aleksandrov <nikolay@nvidia.com>
>
> When we try to add an IPv6 nexthop and IPv6 is not enabled
> (!CONFIG_IPV6) we'll hit a NULL pointer dereference[1] in the error path
> of nh_create_ipv6() due to calling ipv6_stub->fib6_nh_release. The bug
> has been present since the beginning of IPv6 nexthop gateway support.
> Commit 1aefd3de7bc6 ("ipv6: Add fib6_nh_init and release to stubs") tells
> us that only fib6_nh_init has a dummy stub because fib6_nh_release should
> not be called if fib6_nh_init returns an error, but the commit below added
> a call to ipv6_stub->fib6_nh_release in its error path. To fix it return
> the dummy stub's -EAFNOSUPPORT error directly without calling
> ipv6_stub->fib6_nh_release in nh_create_ipv6()'s error path.
[...]
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index a69a9e76f99f..5dbd4b5505eb 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -2565,11 +2565,15 @@ static int nh_create_ipv6(struct net *net, struct nexthop *nh,
> /* sets nh_dev if successful */
> err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
> extack);
> - if (err)
> + if (err) {
> + /* IPv6 is not enabled, don't call fib6_nh_release */
> + if (err == -EAFNOSUPPORT)
> + goto out;
> ipv6_stub->fib6_nh_release(fib6_nh);
Is the call actually necessary? If fib6_nh_init() failed, then I believe
it should clean up after itself and not rely on fib6_nh_release().
> - else
> + } else {
> nh->nh_flags = fib6_nh->fib_nh_flags;
> -
> + }
> +out:
> return err;
> }
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: nexthop: fix null pointer dereference when IPv6 is not enabled
2021-11-23 11:09 ` Ido Schimmel
@ 2021-11-23 11:33 ` Nikolay Aleksandrov
2021-11-23 11:43 ` Nikolay Aleksandrov
0 siblings, 1 reply; 5+ messages in thread
From: Nikolay Aleksandrov @ 2021-11-23 11:33 UTC (permalink / raw)
To: Ido Schimmel, Nikolay Aleksandrov; +Cc: netdev, davem, kuba, dsahern, stable
On 23/11/2021 13:09, Ido Schimmel wrote:
> On Tue, Nov 23, 2021 at 12:27:19PM +0200, Nikolay Aleksandrov wrote:
>> From: Nikolay Aleksandrov <nikolay@nvidia.com>
>>
>> When we try to add an IPv6 nexthop and IPv6 is not enabled
>> (!CONFIG_IPV6) we'll hit a NULL pointer dereference[1] in the error path
>> of nh_create_ipv6() due to calling ipv6_stub->fib6_nh_release. The bug
>> has been present since the beginning of IPv6 nexthop gateway support.
>> Commit 1aefd3de7bc6 ("ipv6: Add fib6_nh_init and release to stubs") tells
>> us that only fib6_nh_init has a dummy stub because fib6_nh_release should
>> not be called if fib6_nh_init returns an error, but the commit below added
>> a call to ipv6_stub->fib6_nh_release in its error path. To fix it return
>> the dummy stub's -EAFNOSUPPORT error directly without calling
>> ipv6_stub->fib6_nh_release in nh_create_ipv6()'s error path.
>
> [...]
>
>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>> index a69a9e76f99f..5dbd4b5505eb 100644
>> --- a/net/ipv4/nexthop.c
>> +++ b/net/ipv4/nexthop.c
>> @@ -2565,11 +2565,15 @@ static int nh_create_ipv6(struct net *net, struct nexthop *nh,
>> /* sets nh_dev if successful */
>> err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>> extack);
>> - if (err)
>> + if (err) {
>> + /* IPv6 is not enabled, don't call fib6_nh_release */
>> + if (err == -EAFNOSUPPORT)
>> + goto out;
>> ipv6_stub->fib6_nh_release(fib6_nh);
>
> Is the call actually necessary? If fib6_nh_init() failed, then I believe
> it should clean up after itself and not rely on fib6_nh_release().
>
I think it doesn't do that, or at least not entirely. For example take the following
sequence of events:
fib6_nh_init:
...
err = fib_nh_common_init(net, &fib6_nh->nh_common, cfg->fc_encap,
cfg->fc_encap_type, cfg, gfp_flags, extack);
(passes)
then after:
fib6_nh->rt6i_pcpu = alloc_percpu_gfp(struct rt6_info *, gfp_flags);
if (!fib6_nh->rt6i_pcpu) {
err = -ENOMEM;
goto out;
}
(fails)
I don't see anything in the error path that would free the fib_nh_common_init() resources,
i.e. nothing calls fib_nh_common_release(), which is called by fib6_nh_release().
By the way, I haven't checked but it looks like fib_check_nh_v6_gw() might leak memory if
fib6_nh_init() fails like that unless I'm missing something.
That change might be doable, but much riskier because there is at least 1 call site which relies
on fib6_info_release -> fib6_info_destroy_rcu() to call fib6_nh_release in its error path.
I'd prefer to fix these bugs in a straight-forward way and would go with the bigger
change for fib6_nh_init() cleanup for net-next. WDYT ?
Cheers,
Nik
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: nexthop: fix null pointer dereference when IPv6 is not enabled
2021-11-23 11:33 ` Nikolay Aleksandrov
@ 2021-11-23 11:43 ` Nikolay Aleksandrov
0 siblings, 0 replies; 5+ messages in thread
From: Nikolay Aleksandrov @ 2021-11-23 11:43 UTC (permalink / raw)
To: Ido Schimmel, Nikolay Aleksandrov; +Cc: netdev, davem, kuba, dsahern, stable
On 23/11/2021 13:33, Nikolay Aleksandrov wrote:
> On 23/11/2021 13:09, Ido Schimmel wrote:
>> On Tue, Nov 23, 2021 at 12:27:19PM +0200, Nikolay Aleksandrov wrote:
>>> From: Nikolay Aleksandrov <nikolay@nvidia.com>
>>>
>>> When we try to add an IPv6 nexthop and IPv6 is not enabled
>>> (!CONFIG_IPV6) we'll hit a NULL pointer dereference[1] in the error path
>>> of nh_create_ipv6() due to calling ipv6_stub->fib6_nh_release. The bug
>>> has been present since the beginning of IPv6 nexthop gateway support.
>>> Commit 1aefd3de7bc6 ("ipv6: Add fib6_nh_init and release to stubs") tells
>>> us that only fib6_nh_init has a dummy stub because fib6_nh_release should
>>> not be called if fib6_nh_init returns an error, but the commit below added
>>> a call to ipv6_stub->fib6_nh_release in its error path. To fix it return
>>> the dummy stub's -EAFNOSUPPORT error directly without calling
>>> ipv6_stub->fib6_nh_release in nh_create_ipv6()'s error path.
>>
>> [...]
>>
>>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>>> index a69a9e76f99f..5dbd4b5505eb 100644
>>> --- a/net/ipv4/nexthop.c
>>> +++ b/net/ipv4/nexthop.c
>>> @@ -2565,11 +2565,15 @@ static int nh_create_ipv6(struct net *net, struct nexthop *nh,
>>> /* sets nh_dev if successful */
>>> err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>>> extack);
>>> - if (err)
>>> + if (err) {
>>> + /* IPv6 is not enabled, don't call fib6_nh_release */
>>> + if (err == -EAFNOSUPPORT)
>>> + goto out;
>>> ipv6_stub->fib6_nh_release(fib6_nh);
>>
>> Is the call actually necessary? If fib6_nh_init() failed, then I believe
>> it should clean up after itself and not rely on fib6_nh_release().
>>
>
> I think it doesn't do that, or at least not entirely. For example take the following
> sequence of events:
> fib6_nh_init:
> ...
> err = fib_nh_common_init(net, &fib6_nh->nh_common, cfg->fc_encap,
> cfg->fc_encap_type, cfg, gfp_flags, extack);
> (passes)
>
> then after:
>
> fib6_nh->rt6i_pcpu = alloc_percpu_gfp(struct rt6_info *, gfp_flags);
> if (!fib6_nh->rt6i_pcpu) {
> err = -ENOMEM;
> goto out;
> }
> (fails)
>
> I don't see anything in the error path that would free the fib_nh_common_init() resources,
> i.e. nothing calls fib_nh_common_release(), which is called by fib6_nh_release().
>
> By the way, I haven't checked but it looks like fib_check_nh_v6_gw() might leak memory if
> fib6_nh_init() fails like that unless I'm missing something.
>
> That change might be doable, but much riskier because there is at least 1 call site which relies
> on fib6_info_release -> fib6_info_destroy_rcu() to call fib6_nh_release in its error path.
>
> I'd prefer to fix these bugs in a straight-forward way and would go with the bigger
> change for fib6_nh_init() cleanup for net-next. WDYT ?
>
> Cheers,
> Nik
>
>
Just to let everyone know, me and Ido had a quick offline discussion about the issue, I'll
try to untangle the places which have different cleanup expectations of fib6_nh_init and
try to make it clean up after itself, as that would fix more bugs (e.g. the memory leak I
mentioned earlier) automatically. If the change is too risky or becomes bigger than expected
we can always continue with the simpler fixes for -net and clean it all up in net-next.
I'll update the thread soon.
Thanks,
Nik
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: nexthop: fix null pointer dereference when IPv6 is not enabled
2021-11-23 10:27 [PATCH net] net: nexthop: fix null pointer dereference when IPv6 is not enabled Nikolay Aleksandrov
2021-11-23 11:09 ` Ido Schimmel
@ 2021-11-23 11:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-11-23 11:50 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, idosch, davem, kuba, dsahern, nikolay, stable
Hello:
This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:
On Tue, 23 Nov 2021 12:27:19 +0200 you wrote:
> From: Nikolay Aleksandrov <nikolay@nvidia.com>
>
> When we try to add an IPv6 nexthop and IPv6 is not enabled
> (!CONFIG_IPV6) we'll hit a NULL pointer dereference[1] in the error path
> of nh_create_ipv6() due to calling ipv6_stub->fib6_nh_release. The bug
> has been present since the beginning of IPv6 nexthop gateway support.
> Commit 1aefd3de7bc6 ("ipv6: Add fib6_nh_init and release to stubs") tells
> us that only fib6_nh_init has a dummy stub because fib6_nh_release should
> not be called if fib6_nh_init returns an error, but the commit below added
> a call to ipv6_stub->fib6_nh_release in its error path. To fix it return
> the dummy stub's -EAFNOSUPPORT error directly without calling
> ipv6_stub->fib6_nh_release in nh_create_ipv6()'s error path.
>
> [...]
Here is the summary with links:
- [net] net: nexthop: fix null pointer dereference when IPv6 is not enabled
https://git.kernel.org/netdev/net/c/1c743127cc54
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-23 11:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-23 10:27 [PATCH net] net: nexthop: fix null pointer dereference when IPv6 is not enabled Nikolay Aleksandrov
2021-11-23 11:09 ` Ido Schimmel
2021-11-23 11:33 ` Nikolay Aleksandrov
2021-11-23 11:43 ` Nikolay Aleksandrov
2021-11-23 11:50 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).