linux-kernel - Re: [PATCH] unwind: Show that entries of struct unwind_cache is not bound by nr

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] [day] [month] [year] [list]

Message-ID: <202511171303.1623D77@keescook>
Date: Mon, 17 Nov 2025 13:28:59 -0800
From: Kees Cook <kees@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
	Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>,
	Thorsten Blum <thorsten.blum@...ux.dev>,
	Josh Poimboeuf <jpoimboe@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	David Laight <david.laight.linux@...il.com>
Subject: Re: [PATCH] unwind: Show that entries of struct unwind_cache is not
 bound by nr_entries

On Fri, Nov 14, 2025 at 12:13:52PM -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@...dmis.org>
> 
> The structure unwind_cache has:
> 
> struct unwind_cache {
> 	unsigned long		unwind_completed;
> 	unsigned int		nr_entries;
> 	unsigned long		entries[];
> };
> 
> Which triggers lots of scripts to convert this to:
> 
> struct unwind_cache {
> 	unsigned long		unwind_completed;
> 	unsigned int		nr_entries;
> 	unsigned long		entries[] __counted_by(nr_entries);
> };
> 
> But that is incorrect. The structure is created via:
> 
>  #define UNWIND_MAX_ENTRIES					\
> 	((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))
> 
>  info->cache = kzalloc(struct_size(cache, entries, UNWIND_MAX_ENTRIES), GFP_KERNEL);
> 
> Where the size of entries is determined by the size of the rest of the
> structure subtracted from 4K. But because the size of entries has a
> dependency on the structure itself, it can't be used to define it.
> 
> The entries are filled by another function that returns how many entries it
> added and that is what nr_entries gets set to. This would most definitely
> trigger a false-positive out-of-bounds bug if the __counted_by() was added.

Just so I'm clear: "nr_entries" shows how many _valid_ entries exist in
entries[] (even if more are allocated there)? (Wouldn't you still want to
know if something tried to access an invalid entry? But I digress...)

> To stop scripts from thinking this needs a counted_by(), move the
> UNWIND_MAX_ENTRIES macro to the header, and add a comment in the entries
> size:
> 
> 	unsigned long		entries[ /* UNWIND_MAX_ENTRIES */ ];

This doesn't solve the problem that we've hidden the actual size of the
(fixed size!) object from the compiler, forcing it to avoid doing bounds
checking on it.

The problem is that the non-"entries" portion of the struct doesn't have
a "name" associated with it at declaration time, but we have a solution
for that already: struct_group. I would propose this form instead, which
requires no changes to how unwind_cache is used, and allows for the true
size of the "entries" array to be exposed to the compiler (and allows
for "normal" methods of finding the max entries):

struct unwind_cache {
	struct_group_tagged(unwind_cache_hdr, hdr,
		unsigned long unwind_completed;
		unsigned int  nr_entries;
	);
	unsigned long         entries[(SZ_4K - sizeof(struct unwind_cache_hdr)) / sizeof(long)];
};

#define UNWIND_MAX_ENTRIES ARRAY_SIZE(((struct unwind_cache*)NULL)->entries)

And this checks out for me:

UNWIND_MAX_ENTRIES:510
sizeof(struct unwind_cache):4096

No hiding things from the compiler, and you can treat "entries" like a
real array (since it is one now).

-Kees

-- 
Kees Cook