Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init

[Arnd Bergmann]

On Thu, Jun 22, 2017 at 11:02 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
>> On Thu, Jun 22, 2017 at 9:59 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>> >
>> > So, to continue this side thought about uninitialized_var(), it is dangerous
>> > because the following buggy pattern does not generate a compiler warning:
>> >
>> >         long uninitialized_var(error);
>> >
>> >         ...
>> >
>> >         if (error)
>> >                 return error;
>> >
>> >
>> > ... and still there are over 290 uses of uninitialized_var() in the kernel - and
>> > any of them could turn into a silent but real uninitialized variable bugs due to
>> > subsequent changes.
>>
>> Right, absolutely agreed on that. A related problem however is blindly
>> initializing variables to NULL to get rid of uninitialized variable warnings,
>> such as
>>
>>       struct subsystem_specific *obj = NULL;
>>       if (function_argument > 10)
>>               goto err;
>>      obj = create_obj();
>> ...
>> err:
>>       clean_up(obj->member);
>>
>>
>> I've seen a couple of variations of that problem, so simply outlawing
>> uninitialized_var() will only solve a subset of these issues, and ideally
>> we should also make sure that initializations at declaration time are
>> used properly, and not just to shut up compiler warnings.
>
> Well, a deterministic crash on a NULL dereference is still (much) better than a
> non-deterministic 'use random value from stack and corrupt memory or crash' bug
> pattern, right?

The NULL initialization is more reproducible, but has also led to easier
exploits in the past, when user space could more easily map a writable
memory to virtual address zero and make the kernel jump there for
privilege escalation.

> Also, static analysis tools ought to be pretty good about finding control flows
> where a NULL gets dereferenced.

I think the tooling we have for uninitialized data (assuming uninitialized_var()
is not used) is better than that for NULL dereferences. While gcc can detect
cases where a NULL pointer is dereferenced, it won't warn about that
and instead invoke its undefined-behavior optimization: it will assume that
this code path is never hit and may run off the end of a function
or worse instead of actually doing the NULL pointer dereference. I think with
CONFIG_UBSAN, it will insert a trapping instruction instead for a
known NULL dereference.

A similar thing happens for integer divide-by-zero, which can also go unnoticed
because of extraneous initializations. gcc-7 can detect more often now than it
used to, but often simply assumes that any code path leading up to
div0 exception
cannot happen, and will instead trap when it gets there, and silently discard
any code following it.

        Arnd