* [PATCH] ipv4: fib: avoid NULL dereference
@ 2018-07-06 14:28 Mark Rutland
2018-07-06 14:47 ` Eric Dumazet
2018-07-06 14:49 ` David Ahern
0 siblings, 2 replies; 10+ messages in thread
From: Mark Rutland @ 2018-07-06 14:28 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Rutland, Alexey Kuznetsov, David S . Miller,
Hideaki YOSHIFUJI, netdev
In tnode_free() we iterate over a callback_head list with a while loop.
At the start of the loop body we generate the next head pointer, and at
the end of the loop body we generate the tn pointer for the next
iteration of the loop by using container_of() on the head pointer to
find the tnode, and deriving the kv pointer from this.
In the final iteration of the loop, this means that we derive a pointer
from NULL, which is undefined behaviour, which UBSAN detects:
================================================================================
UBSAN: Undefined behaviour in net/ipv4/fib_trie.c:504:6
member access within null pointer of type 'struct tnode'
CPU: 1 PID: 94 Comm: ip Not tainted 4.18.0-rc3+ #23
Hardware name: linux,dummy-virt (DT)
Call trace:
dump_backtrace+0x0/0x458
show_stack+0x20/0x30
dump_stack+0x18c/0x248
ubsan_epilogue+0x18/0x94
handle_null_ptr_deref+0x1d4/0x228
__ubsan_handle_type_mismatch_v1+0x188/0x1e0
tnode_free+0x16c/0x1d0
replace+0x1e0/0x5d0
resize+0xd98/0x2008
fib_insert_alias+0xb38/0x10c8
fib_table_insert+0x7d0/0x1108
fib_magic+0x530/0x780
fib_add_ifaddr+0x378/0x468
fib_netdev_event+0x2ac/0x3e8
notifier_call_chain+0x190/0x2f8
raw_notifier_call_chain+0x3c/0x68
call_netdevice_notifiers_info+0x3c/0xc0
__dev_notify_flags+0x1f8/0x398
dev_change_flags+0xe8/0x150
do_setlink+0x924/0x4050
rtnl_newlink+0x8c4/0x14b0
rtnetlink_rcv_msg+0x408/0xef8
netlink_rcv_skb+0x144/0x390
rtnetlink_rcv+0x24/0x30
netlink_unicast+0x4e8/0x740
netlink_sendmsg+0x6d8/0xe78
sock_sendmsg+0x90/0x168
___sys_sendmsg+0x680/0x9b0
__sys_sendmsg+0xf0/0x230
sys_sendmsg+0x34/0x48
el0_svc_naked+0x30/0x34
================================================================================
We can avoid the undefined behaviour by generating tn for the current
iteration of the loop before we advance head, so let's do that.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: David S. Miller <davem@davemloft.net>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: netdev@vger.kernel.org
---
net/ipv4/fib_trie.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 5bc0c89e81e4..8d98c8162554 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -497,11 +497,11 @@ static void tnode_free(struct key_vector *tn)
struct callback_head *head = &tn_info(tn)->rcu;
while (head) {
+ tn = container_of(head, struct tnode, rcu)->kv;
+
head = head->next;
tnode_free_size += TNODE_SIZE(1ul << tn->bits);
node_free(tn);
-
- tn = container_of(head, struct tnode, rcu)->kv;
}
if (tnode_free_size >= PAGE_SIZE * sync_pages) {
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] ipv4: fib: avoid NULL dereference
2018-07-06 14:28 [PATCH] ipv4: fib: avoid NULL dereference Mark Rutland
@ 2018-07-06 14:47 ` Eric Dumazet
2018-07-06 14:57 ` Mark Rutland
2018-07-06 14:49 ` David Ahern
1 sibling, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2018-07-06 14:47 UTC (permalink / raw)
To: Mark Rutland, linux-kernel
Cc: Alexey Kuznetsov, David S . Miller, Hideaki YOSHIFUJI, netdev
On 07/06/2018 07:28 AM, Mark Rutland wrote:
> In tnode_free() we iterate over a callback_head list with a while loop.
> At the start of the loop body we generate the next head pointer, and at
> the end of the loop body we generate the tn pointer for the next
> iteration of the loop by using container_of() on the head pointer to
> find the tnode, and deriving the kv pointer from this.
>
> In the final iteration of the loop, this means that we derive a pointer
> from NULL, which is undefined behaviour, which UBSAN detects:
There is no dereference, your patch title is misleading.
UBSAN might be fooled, not the C compiler.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ipv4: fib: avoid NULL dereference
2018-07-06 14:28 [PATCH] ipv4: fib: avoid NULL dereference Mark Rutland
2018-07-06 14:47 ` Eric Dumazet
@ 2018-07-06 14:49 ` David Ahern
2018-07-06 14:55 ` Mark Rutland
1 sibling, 1 reply; 10+ messages in thread
From: David Ahern @ 2018-07-06 14:49 UTC (permalink / raw)
To: Mark Rutland, linux-kernel
Cc: Alexey Kuznetsov, David S . Miller, Hideaki YOSHIFUJI, netdev
On 7/6/18 8:28 AM, Mark Rutland wrote:
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 5bc0c89e81e4..8d98c8162554 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -497,11 +497,11 @@ static void tnode_free(struct key_vector *tn)
> struct callback_head *head = &tn_info(tn)->rcu;
>
> while (head) {
> + tn = container_of(head, struct tnode, rcu)->kv;
> +
> head = head->next;
> tnode_free_size += TNODE_SIZE(1ul << tn->bits);
> node_free(tn);
you are changing tn before the above 2 calls are made on the tn passed in
> -
> - tn = container_of(head, struct tnode, rcu)->kv;
> }
>
> if (tnode_free_size >= PAGE_SIZE * sync_pages) {
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ipv4: fib: avoid NULL dereference
2018-07-06 14:49 ` David Ahern
@ 2018-07-06 14:55 ` Mark Rutland
0 siblings, 0 replies; 10+ messages in thread
From: Mark Rutland @ 2018-07-06 14:55 UTC (permalink / raw)
To: David Ahern
Cc: linux-kernel, Alexey Kuznetsov, David S . Miller,
Hideaki YOSHIFUJI, netdev
On Fri, Jul 06, 2018 at 08:49:14AM -0600, David Ahern wrote:
> On 7/6/18 8:28 AM, Mark Rutland wrote:
> > diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> > index 5bc0c89e81e4..8d98c8162554 100644
> > --- a/net/ipv4/fib_trie.c
> > +++ b/net/ipv4/fib_trie.c
> > @@ -497,11 +497,11 @@ static void tnode_free(struct key_vector *tn)
> > struct callback_head *head = &tn_info(tn)->rcu;
> >
> > while (head) {
> > + tn = container_of(head, struct tnode, rcu)->kv;
> > +
> > head = head->next;
> > tnode_free_size += TNODE_SIZE(1ul << tn->bits);
> > node_free(tn);
>
> you are changing tn before the above 2 calls are made on the tn passed in
As tn_info(tn) is container_of(kv, struct tnode, tn), the end result is
the same tn value, so this is not a problem.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ipv4: fib: avoid NULL dereference
2018-07-06 14:47 ` Eric Dumazet
@ 2018-07-06 14:57 ` Mark Rutland
2018-07-06 15:39 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2018-07-06 14:57 UTC (permalink / raw)
To: Eric Dumazet
Cc: linux-kernel, Alexey Kuznetsov, David S . Miller,
Hideaki YOSHIFUJI, netdev
On Fri, Jul 06, 2018 at 07:47:04AM -0700, Eric Dumazet wrote:
>
>
> On 07/06/2018 07:28 AM, Mark Rutland wrote:
> > In tnode_free() we iterate over a callback_head list with a while loop.
> > At the start of the loop body we generate the next head pointer, and at
> > the end of the loop body we generate the tn pointer for the next
> > iteration of the loop by using container_of() on the head pointer to
> > find the tnode, and deriving the kv pointer from this.
> >
> > In the final iteration of the loop, this means that we derive a pointer
> > from NULL, which is undefined behaviour, which UBSAN detects:
>
> There is no dereference, your patch title is misleading.
>
> UBSAN might be fooled, not the C compiler.
I'm happy to change the title to "avoid undefined behaviour".
Thanks,
Mark.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ipv4: fib: avoid NULL dereference
2018-07-06 14:57 ` Mark Rutland
@ 2018-07-06 15:39 ` Eric Dumazet
2018-07-06 15:54 ` Mark Rutland
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2018-07-06 15:39 UTC (permalink / raw)
To: Mark Rutland, Eric Dumazet
Cc: linux-kernel, Alexey Kuznetsov, David S . Miller,
Hideaki YOSHIFUJI, netdev
On 07/06/2018 07:57 AM, Mark Rutland wrote:
> On Fri, Jul 06, 2018 at 07:47:04AM -0700, Eric Dumazet wrote:
>>
>>
>> On 07/06/2018 07:28 AM, Mark Rutland wrote:
>>> In tnode_free() we iterate over a callback_head list with a while loop.
>>> At the start of the loop body we generate the next head pointer, and at
>>> the end of the loop body we generate the tn pointer for the next
>>> iteration of the loop by using container_of() on the head pointer to
>>> find the tnode, and deriving the kv pointer from this.
>>>
>>> In the final iteration of the loop, this means that we derive a pointer
>>> from NULL, which is undefined behaviour, which UBSAN detects:
>>
>> There is no dereference, your patch title is misleading.
>>
>> UBSAN might be fooled, not the C compiler.
>
> I'm happy to change the title to "avoid undefined behaviour".
>
Are you planning to change this as well ?
include/linux/stddef.h:19:#define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER)
(And probably dozens of other locations)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ipv4: fib: avoid NULL dereference
2018-07-06 15:39 ` Eric Dumazet
@ 2018-07-06 15:54 ` Mark Rutland
2018-07-06 16:20 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2018-07-06 15:54 UTC (permalink / raw)
To: Eric Dumazet
Cc: linux-kernel, Alexey Kuznetsov, David S . Miller,
Hideaki YOSHIFUJI, netdev
On Fri, Jul 06, 2018 at 08:39:11AM -0700, Eric Dumazet wrote:
>
>
> On 07/06/2018 07:57 AM, Mark Rutland wrote:
> > On Fri, Jul 06, 2018 at 07:47:04AM -0700, Eric Dumazet wrote:
> >>
> >>
> >> On 07/06/2018 07:28 AM, Mark Rutland wrote:
> >>> In tnode_free() we iterate over a callback_head list with a while loop.
> >>> At the start of the loop body we generate the next head pointer, and at
> >>> the end of the loop body we generate the tn pointer for the next
> >>> iteration of the loop by using container_of() on the head pointer to
> >>> find the tnode, and deriving the kv pointer from this.
> >>>
> >>> In the final iteration of the loop, this means that we derive a pointer
> >>> from NULL, which is undefined behaviour, which UBSAN detects:
> >>
> >> There is no dereference, your patch title is misleading.
> >>
> >> UBSAN might be fooled, not the C compiler.
> >
> > I'm happy to change the title to "avoid undefined behaviour".
> >
>
> Are you planning to change this as well ?
>
> include/linux/stddef.h:19:#define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER)
No, because __builtin_offsetof() is used these days (since GCC 4),
avoiding the undefined behaviour.
> (And probably dozens of other locations)
I do concede that if this is everywhere it's not worth the effort, and
from the looks of things, the gnaliest cases are where we do things
like:
get_user(var, &struct->field)
... where the user could validly pass a NULL pointer if it wished.
So I guess I'll give up.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ipv4: fib: avoid NULL dereference
2018-07-06 15:54 ` Mark Rutland
@ 2018-07-06 16:20 ` Eric Dumazet
2018-07-06 16:33 ` Mark Rutland
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2018-07-06 16:20 UTC (permalink / raw)
To: Mark Rutland, Eric Dumazet
Cc: linux-kernel, Alexey Kuznetsov, David S . Miller,
Hideaki YOSHIFUJI, netdev
On 07/06/2018 08:54 AM, Mark Rutland wrote:
> On Fri, Jul 06, 2018 at 08:39:11AM -0700, Eric Dumazet wrote:
>>
>>
>> On 07/06/2018 07:57 AM, Mark Rutland wrote:
>>> On Fri, Jul 06, 2018 at 07:47:04AM -0700, Eric Dumazet wrote:
>>>>
>>>>
>>>> On 07/06/2018 07:28 AM, Mark Rutland wrote:
>>>>> In tnode_free() we iterate over a callback_head list with a while loop.
>>>>> At the start of the loop body we generate the next head pointer, and at
>>>>> the end of the loop body we generate the tn pointer for the next
>>>>> iteration of the loop by using container_of() on the head pointer to
>>>>> find the tnode, and deriving the kv pointer from this.
>>>>>
>>>>> In the final iteration of the loop, this means that we derive a pointer
>>>>> from NULL, which is undefined behaviour, which UBSAN detects:
>>>>
>>>> There is no dereference, your patch title is misleading.
>>>>
>>>> UBSAN might be fooled, not the C compiler.
>>>
>>> I'm happy to change the title to "avoid undefined behaviour".
>>>
>>
>> Are you planning to change this as well ?
>>
>> include/linux/stddef.h:19:#define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER)
>
> No, because __builtin_offsetof() is used these days (since GCC 4),
> avoiding the undefined behaviour.
Ah... should we remove the line and declare linux must be compiled
with GCC 4 at least ?
>
>> (And probably dozens of other locations)
>
> I do concede that if this is everywhere it's not worth the effort, and
> from the looks of things, the gnaliest cases are where we do things
> like:
>
> get_user(var, &struct->field)
>
> ... where the user could validly pass a NULL pointer if it wished.
>
> So I guess I'll give up.
There is value to your patch, since it makes UBSAN happy.
But please change the title and changelog accordingly.
Thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ipv4: fib: avoid NULL dereference
2018-07-06 16:20 ` Eric Dumazet
@ 2018-07-06 16:33 ` Mark Rutland
2018-07-06 16:39 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2018-07-06 16:33 UTC (permalink / raw)
To: Eric Dumazet
Cc: linux-kernel, Alexey Kuznetsov, David S . Miller,
Hideaki YOSHIFUJI, netdev, arnd
On Fri, Jul 06, 2018 at 09:20:44AM -0700, Eric Dumazet wrote:
>
>
> On 07/06/2018 08:54 AM, Mark Rutland wrote:
> > On Fri, Jul 06, 2018 at 08:39:11AM -0700, Eric Dumazet wrote:
> >>
> >>
> >> On 07/06/2018 07:57 AM, Mark Rutland wrote:
> >>> On Fri, Jul 06, 2018 at 07:47:04AM -0700, Eric Dumazet wrote:
> >>>>
> >>>>
> >>>> On 07/06/2018 07:28 AM, Mark Rutland wrote:
> >>>>> In tnode_free() we iterate over a callback_head list with a while loop.
> >>>>> At the start of the loop body we generate the next head pointer, and at
> >>>>> the end of the loop body we generate the tn pointer for the next
> >>>>> iteration of the loop by using container_of() on the head pointer to
> >>>>> find the tnode, and deriving the kv pointer from this.
> >>>>>
> >>>>> In the final iteration of the loop, this means that we derive a pointer
> >>>>> from NULL, which is undefined behaviour, which UBSAN detects:
> >>>>
> >>>> There is no dereference, your patch title is misleading.
> >>>>
> >>>> UBSAN might be fooled, not the C compiler.
> >>>
> >>> I'm happy to change the title to "avoid undefined behaviour".
> >>>
> >>
> >> Are you planning to change this as well ?
> >>
> >> include/linux/stddef.h:19:#define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER)
> >
> > No, because __builtin_offsetof() is used these days (since GCC 4),
> > avoiding the undefined behaviour.
>
> Ah... should we remove the line and declare linux must be compiled
> with GCC 4 at least ?
Good question -- IIUC x86 already mandates at least GCC 4.6, as this is
necessary for jump labels.
There was a push to mandate a more recent compiler generally, but I'm
not sure of the current state of things.
Arnd, were we planning to mandate at least GCC 4.x?
> > I do concede that if this is everywhere it's not worth the effort, and
> > from the looks of things, the gnaliest cases are where we do things
> > like:
> >
> > get_user(var, &struct->field)
> >
> > ... where the user could validly pass a NULL pointer if it wished.
> >
> > So I guess I'll give up.
>
> There is value to your patch, since it makes UBSAN happy.
>
> But please change the title and changelog accordingly.
Sure thing. Are you happy with the "deriving a pointer from NULL"
wording in the explanation? That wasn't intended to mean a dereference.
I'll change the title to:
ipv4: fib: avoid undefined behaviour
Thanks,
Mark.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ipv4: fib: avoid NULL dereference
2018-07-06 16:33 ` Mark Rutland
@ 2018-07-06 16:39 ` Eric Dumazet
0 siblings, 0 replies; 10+ messages in thread
From: Eric Dumazet @ 2018-07-06 16:39 UTC (permalink / raw)
To: Mark Rutland, Eric Dumazet
Cc: linux-kernel, Alexey Kuznetsov, David S . Miller,
Hideaki YOSHIFUJI, netdev, arnd
On 07/06/2018 09:33 AM, Mark Rutland wrote:
> Sure thing. Are you happy with the "deriving a pointer from NULL"
> wording in the explanation? That wasn't intended to mean a dereference.
>
> I'll change the title to:
>
> ipv4: fib: avoid undefined behaviour
This would be fine for me, thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-07-06 16:39 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-06 14:28 [PATCH] ipv4: fib: avoid NULL dereference Mark Rutland
2018-07-06 14:47 ` Eric Dumazet
2018-07-06 14:57 ` Mark Rutland
2018-07-06 15:39 ` Eric Dumazet
2018-07-06 15:54 ` Mark Rutland
2018-07-06 16:20 ` Eric Dumazet
2018-07-06 16:33 ` Mark Rutland
2018-07-06 16:39 ` Eric Dumazet
2018-07-06 14:49 ` David Ahern
2018-07-06 14:55 ` Mark Rutland
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.