Message-ID: <20250613191556.4184103-1-marc.herbert@linux.intel.com>
Date: Fri, 13 Jun 2025 19:15:56 +0000
From: marc.herbert@...ux.intel.com
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	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>
Cc: Marc Herbert <marc.herbert@...ux.intel.com>
Subject: [PATCH] driver core: faux: fix Undefined Behavior in faux_device_destroy()

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>
---
 drivers/base/faux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/faux.c b/drivers/base/faux.c
index 9054d346bd7f..94392d397986 100644
--- a/drivers/base/faux.c
+++ b/drivers/base/faux.c
@@ -218,11 +218,11 @@ EXPORT_SYMBOL_GPL(faux_device_create);
  */
 void faux_device_destroy(struct faux_device *faux_dev)
 {
-	struct device *dev = &faux_dev->dev;
-
 	if (!faux_dev)
 		return;
 
+	struct device *dev = &faux_dev->dev;
+
 	device_del(dev);
 
 	/* The final put_device() will clean up the memory we allocated for this device. */
-- 
2.49.0

Powered by blists - more mailing lists