linux-kernel - Re: [PATCH] driver core: faux: fix Undefined Behavior in faux_device

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: <2025061313-theater-surrender-944c@gregkh>
Date: Fri, 13 Jun 2025 20:33:42 -0400
From: Greg KH <gregkh@...uxfoundation.org>
To: marc.herbert@...ux.intel.com
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	Dan Williams <dan.j.williams@...el.com>, rafael.j.wysocki@...el.com,
	linux-cxl@...r.kernel.org, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Sudeep Holla <sudeep.holla@....com>,
	Ben Cheatham <Benjamin.Cheatham@....com>,
	Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH] driver core: faux: fix Undefined Behavior in
 faux_device_destroy()

On Fri, Jun 13, 2025 at 07:15:56PM +0000, marc.herbert@...ux.intel.com wrote:
> From: Marc Herbert <marc.herbert@...ux.intel.com>
> 
> Fixes undefined behavior that was spotted by Jonathan Cameron in
> https://lore.kernel.org/linux-cxl/20250609170509.00003625@huawei.com/
> 
> The possible consequences of the undefined behavior fixed here are fairly
> well documented across the Internet but to save research time and avoid
> doubts, I include a very short and simple demo below. I imagine kernel
> compilation flags and various other conditions may not make the
> consequences as bad as this example, however those conditions could change
> and this type of code is still Undefined Behavior no matter what.
> One of the best articles - there are many others:
> https://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html
> 
> Since commit b5ec6fd286dfa4 ("kbuild: Drop -Wdeclaration-after-statement"),
> it's now possible to use C99 declarations; the kernel is not constrained
> anymore to group all declarations at the top of a block like single-pass
> compilers used to require. This allows combining declarations and
> definitions in one place - like literally every other language and project
> does - and trivially fix undefined behavior like this.  This also reduces
> variable scope and avoids misuse between declaration and definition like
> uninitialized reads or writing to the wrong variable by mistake. C99
> declarations also allow using a lot more `const` (the default in some
> languages) which avoids some misuse after legitimate use.
> tl;dr: C99 declarations are not just a "codestyle" or "taste" issue;
> they are an important (and not mandatory) feature.
> 
> cc --version
>   cc (GCC) 15.1.1 20250425
> 
> for i in 0 1 2 g; do printf "gcc -O$i: "; gcc -O$i nullptrUB.c &&
>    ./a.out; done
> 
> gcc -O0: Segmentation fault (core dumped)
> gcc -O1: ptr is zero
> gcc -O2: ptr is NOT zero!!!
> gcc -O3: ptr is NOT zero!!!
> gcc -Og: ptr is zero
> 
> clang --version
>   clang version 19.1.7
> 
> clang -O0: Segmentation fault (core dumped)
> clang -O1: ptr is NOT zero!!!
> clang -O2: ptr is NOT zero!!!
> clang -O3: ptr is NOT zero!!!
> clang -Og: ptr is NOT zero!!!
> 
> int faux_device_destroy(int *ptr)
> {
>   int i = *ptr;  i++;
> 
>   // Because we dereferenced ptr, the compiler knows the pointer cannot
>   // be null (even when it is!) and can optimize this away.
>   if (!ptr) {
>     printf("ptr is zero\n");
>     return 0;
>   }
> 
>   printf("ptr is NOT zero!!!\n");
>   return 1;
> }
> 
> int main()
> {
>   struct timespec t1, t2;
>   clock_gettime(CLOCK_MONOTONIC, &t1);
>   clock_gettime(CLOCK_MONOTONIC, &t2);
> 
>   // Use the clock to hide zero from the compiler
>   int * zeroptr = (int *)(t2.tv_sec - t1.tv_sec);
> 
>   return faux_device_destroy(zeroptr);
> }
> 
> Fixes: 35fa2d88ca94 ("driver core: add a faux bus for use when a simple device/bus is needed")
> Signed-off-by: Marc Herbert <marc.herbert@...ux.intel.com>

Great writeup, but as Miguel says, this isn't needed at all, the kernel
relies on the compiler to be sane :)

thanks,

greg k-h