Re: [PATCH 4/6] time: Avoid undefined behaviour in timespec64_add_safe()

[Vegard Nossum <vegard.nossum@oracle.com>]

On 09/01/2016 10:02 AM, Richard Cochran wrote:
> On Wed, Aug 31, 2016 at 02:50:20PM -0700, John Stultz wrote:
>>     UBSAN: Undefined behaviour in kernel/time/time.c:783:2
>>     signed integer overflow:
>>     5273 + 9223372036854771711 cannot be represented in type 'long int'
>
> ...
>
>> Line 783 is this:
>>
>> 783         set_normalized_timespec64(&res, lhs.tv_sec + rhs.tv_sec,
>> 784                         lhs.tv_nsec + rhs.tv_nsec);
>
> ...
>
>> Note that this is not currently a huge concern since the kernel should be
>> built with -fno-strict-overflow by default, but could be a problem in the
>> future, a problem with older compilers, or other compilers than gcc.
>
> Is this really a concern at all?  The value 9223372036854771711 is a
> huge number of seconds.

The problem is that "undefined behaviour" means the compiler is
technically free to do whatever it wants -- it could issue an invalid
opcode, which would crash the kernel -- and this bit of code is easily
reached from userspace, making this a potential DOS vector (or worse,
depending on what exactly the compiler chooses to do).

Now the kernel is compiled with -fno-strict-overflow when it is
supported by the compiler, which in practice makes signed integer
overflow defined the same way as unsigned integer overflow (i.e. it
wraps around).

As the patch message says, this could be a problem with older and
non-gcc compilers.

So the concern is not the huge value or an overflow in itself, the
concern is the undefined behaviour where we may not be in control
of what happens.

There's probably at least a dozen other easily reachable overflows like
this in the kernel currently and I have no evidence of any actual
compilers doing really bad things, so yes, this is all somewhat
theoretical, but it's easy to fix which doesn't complicate the code and
adheres to the C language standard, so why not?


Vegard