[PATCH 1/2] userns: Add __counted_by_ptr attribute to struct uid_gid_map

Bill Wendling <[email protected]>
Newsgroups org.kernel.vger.linux-hardening,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
The compiler attribute __counted_by_ptr associates a pointer field of a
struct with a sibling field within the same struct that specifies the
element count of the allocated memory. This enables KASAN and fortified
bounds-checking to detect out-of-bounds accesses to the pointer field at
runtime.

We can add the __counted_by_ptr attribute to the 'forward' and 'reverse'
pointer fields of 'struct uid_gid_map', which are counted by
'nr_extents'. Since 'nr_extents' is defined in a sibling anonymous
struct inside an anonymous union, the nearest common non-anonymous
struct level is 'struct uid_gid_map' itself, which is supported by the
compiler.

However, doing so has runtime implications. In the original
implementation of insert_extent(), elements are written to
map->forward[map->nr_extents] before map->nr_extents is incremented:

	if (map->nr_extents < UID_GID_MAP_MAX_BASE_EXTENTS)
		dest = &map->extent[map->nr_extents];
	else
		dest = &map->forward[map->nr_extents];

	*dest = *extent;
	map->nr_extents++;

At the time of writing to 'map->forward[map->nr_extents]',
map->nr_extents is still 5, but we are accessing index 5 (which is the
6th element). Under __counted_by_ptr(nr_extents), the compiler and
KASAN expect the accessed index to be strictly less than
map->nr_extents. Therefore, accessing index 5 when the count is 5
triggers an out-of-bounds panic/trap at runtime.

To resolve this, insert_extent() is refactored to increment
map->nr_extents first, and then use map->nr_extents - 1 as the index:

	map->nr_extents++;
	if (map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
		dest = &map->extent[map->nr_extents - 1];
	else
		dest = &map->forward[map->nr_extents - 1];

	*dest = *extent;

Assisted-by: Gemini Next
Signed-off-by: Bill Wendling <[email protected]>
---
Cc: Kees Cook <[email protected]>
Cc: "Gustavo A. R. Silva" <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Aleksa Sarai <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Miguel Ojeda <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Nicolas Schier <[email protected]>
Cc: Gary Guo <[email protected]>
Cc: "Thomas Weißschuh" <[email protected]>
Cc: Alice Ryhl <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Anand Moon <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
 include/linux/user_namespace.h | 4 ++--
 kernel/user_namespace.c        | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index e38d9e60569f..2962256eddf7 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -29,8 +29,8 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */
 			u32 nr_extents;
 		};
 		struct {
-			struct uid_gid_extent *forward;
-			struct uid_gid_extent *reverse;
+			struct uid_gid_extent *forward __counted_by_ptr(nr_extents);
+			struct uid_gid_extent *reverse __counted_by_ptr(nr_extents);
 		};
 	};
 };
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 0bed462e9b2a..7e5371d8f515 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -809,13 +809,13 @@ static int insert_extent(struct uid_gid_map *map, struct uid_gid_extent *extent)
 		map->reverse = NULL;
 	}
 
-	if (map->nr_extents < UID_GID_MAP_MAX_BASE_EXTENTS)
-		dest = &map->extent[map->nr_extents];
+	map->nr_extents++;
+	if (map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
+		dest = &map->extent[map->nr_extents - 1];
 	else
-		dest = &map->forward[map->nr_extents];
+		dest = &map->forward[map->nr_extents - 1];
 
 	*dest = *extent;
-	map->nr_extents++;
 	return 0;
 }
 
-- 
2.55.0.860.g4b6b3295ed-goog
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.