linux-kernel - Re: [RFC] Mitigating unexpected arithmetic overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wgKyP2ffZPa6aKYtytzzFibiNVN5MS=D2cn7_UGCECKdw@mail.gmail.com>
Date: Thu, 9 May 2024 08:38:28 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: "Theodore Ts'o" <tytso@....edu>
Cc: Kees Cook <keescook@...omium.org>, Justin Stitt <justinstitt@...gle.com>, 
	Peter Zijlstra <peterz@...radead.org>, Mark Rutland <mark.rutland@....com>, 
	linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org, 
	llvm@...ts.linux.dev
Subject: Re: [RFC] Mitigating unexpected arithmetic overflow

On Thu, 9 May 2024 at 07:09, Theodore Ts'o <tytso@....edu> wrote:
>
> struct ext2_super {
>        ...
>        __u32    time_lo;
>        __u32    time_high;
>        ...
> }
>
>         time_t  now;
>
>         sb->time_low = now;
>         sb->time_high = now >> 32;
>
> This is obviously (to a human) correct,

Actually, it's not correct at all.

If time_t is 32-bit, that "now >> 32" is broken due to how C shifts
are defined. For a 32-bit type, only shifts of 0-31 bits are
well-defined (due to hardware differences).

> but because of stupid compiler
> tricks, in order to silence compiler-level and ubsan complaints, this
> got turned into:
>
>         sb->time_low = now & 0xffffffff;
> #if (SIZEOF_TIME_T > 4)
>         sb->time_high = (now >> 32) & EXT4_EPOCH_MASK;
> #else
>         sb->time_high = 0;
> #endi

This is the wrong solution.

But the solution to the undefined shift isn't some #ifdef.

The idiomatic C solution is to do

        high_bits = (all_bits >> 16) >> 16;

which works even if 'all_bits' is 32 bit, and the compiler will know
this idiom, and turn it into just 0.

Going the other way is similar:

        all_bits = low_bits + ((u64) high_bits << 16) << 16);

and again, the compiler will recognize this idiom and do the right
thing (and if 'all_bits' is only 32-bit, the compiler will optimize
the high bit noise away).

And yes, this is not great, but there you have it: C was designed to
be a high-level assembler, and you see the effects of "this is how
many hardware shifters work". The shift instructions on many (most?)
architectures end up being limited to the word width.

We actually have some helpers for this in <linux/wordpart.h>:

  #define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
  #define lower_32_bits(n) ((u32)((n) & 0xffffffff))

but we don't have that "combine 32 bits into 64 bits" helper. Maybe
worth adding.

                Linus