unwind: Show that entries of struct unwind_cache is not bound by nr_entries

[PATCH] unwind: Show that entries of struct unwind_cache is not bound by nr_entries

Posted by Steven Rostedt 2 months, 3 weeks ago

From: Steven Rostedt <rostedt@goodmis.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.

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 */ ];

Link: https://lore.kernel.org/all/20251114122748.222833-1-thorsten.blum@linux.dev/

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/linux/unwind_deferred_types.h | 6 +++++-
 kernel/unwind/deferred.c              | 4 ----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
index 33b62ac25c86..61496397b0f9 100644
--- a/include/linux/unwind_deferred_types.h
+++ b/include/linux/unwind_deferred_types.h
@@ -2,10 +2,14 @@
 #ifndef _LINUX_UNWIND_USER_DEFERRED_TYPES_H
 #define _LINUX_UNWIND_USER_DEFERRED_TYPES_H
 
+/* Make the cache fit in a 4K page */
+#define UNWIND_MAX_ENTRIES					\
+	((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))
+
 struct unwind_cache {
 	unsigned long		unwind_completed;
 	unsigned int		nr_entries;
-	unsigned long		entries[];
+	unsigned long		entries[ /* UNWIND_MAX_ENTRIES */ ];
 };
 
 /*
diff --git a/kernel/unwind/deferred.c b/kernel/unwind/deferred.c
index dc6040aae3ee..3284bec6d04b 100644
--- a/kernel/unwind/deferred.c
+++ b/kernel/unwind/deferred.c
@@ -37,10 +37,6 @@ static inline bool try_assign_cnt(struct unwind_task_info *info, u32 cnt)
 }
 #endif
 
-/* Make the cache fit in a 4K page */
-#define UNWIND_MAX_ENTRIES					\
-	((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))
-
 /* Guards adding to or removing from the list of callbacks */
 static DEFINE_MUTEX(callback_mutex);
 static LIST_HEAD(callbacks);
-- 
2.51.0

Re: [PATCH] unwind: Show that entries of struct unwind_cache is not bound by nr_entries

Posted by Kees Cook 2 months, 3 weeks ago

On Fri, Nov 14, 2025 at 12:13:52PM -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.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

Re: [PATCH] unwind: Show that entries of struct unwind_cache is not bound by nr_entries

Posted by Steven Rostedt 1 month, 3 weeks ago

On Mon, 17 Nov 2025 13:28:59 -0800
Kees Cook <kees@kernel.org> wrote:

> 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)];
> };

This may help automated tooling, but it is horrendous to read. I value
readability much higher than static analyzers.

Hence, I'm leaving the code as is, and just keep NAKing patches that try to
add __counted_by() to entries.

-- Steve


> 
> #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).