Re: [PATCH] ipv4: fib: avoid NULL dereference

On Fri, Jul 06, 2018 at 09:20:44AM -0700, Eric Dumazet wrote:

On 07/06/2018 08:54 AM, Mark Rutland wrote:

quoted

On Fri, Jul 06, 2018 at 08:39:11AM -0700, Eric Dumazet wrote:

quoted

On 07/06/2018 07:57 AM, Mark Rutland wrote:

quoted

On Fri, Jul 06, 2018 at 07:47:04AM -0700, Eric Dumazet wrote:

quoted

On 07/06/2018 07:28 AM, Mark Rutland wrote:

quoted

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?

quoted

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.