Hi,
This is a series to work around a deficiency in GCC (>=11) and Clang
(<16) where the __alloc_size attribute does not apply to inlines. :(
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503
This manifests as reduced overflow detection coverage for many allocation
sites under CONFIG_FORTIFY_SOURCE=y, where the allocation size was
not actually being propagated to __builtin_dynamic_object_size(). In
addition to working around the issue, expand use of __alloc_size (and
__realloc_size) to more places and provide KUnit tests to validate all
the covered allocator APIs.
-Kees
Kees Cook (6):
slab: Clean up SLOB vs kmalloc() definition
slab: Remove special-casing of const 0 size allocations
slab: Provide functional __alloc_size() hints to kmalloc_trace*()
string: Add __realloc_size hint to kmemdup()
driver core: Add __alloc_size hint to devm allocators
kunit/fortify: Validate __alloc_size attribute results
include/linux/device.h | 7 +-
include/linux/fortify-string.h | 2 +-
include/linux/slab.h | 36 ++---
include/linux/string.h | 2 +-
lib/Makefile | 1 +
lib/fortify_kunit.c | 255 +++++++++++++++++++++++++++++++++
mm/slab_common.c | 14 ++
7 files changed, 296 insertions(+), 21 deletions(-)
--
2.34.1
Since GCC cannot apply the __alloc_size attributes to inlines[1], all
allocator inlines need to explicitly call into extern functions that
contain a size argument. Provide these wrappers that end up just
ignoring the size argument for the actual allocation.
This allows CONFIG_FORTIFY_SOURCE=y to see all various dynamic allocation
sizes under GCC 12+ and all supported Clang versions.
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503
Cc: Vlastimil Babka <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Roman Gushchin <[email protected]>
Cc: Hyeonggon Yoo <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/slab.h | 8 ++++++--
mm/slab_common.c | 14 ++++++++++++++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 970e9504949e..051d86ca31a8 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -442,6 +442,8 @@ static_assert(PAGE_SHIFT <= 20);
void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
+void *kmem_cache_alloc_sized(struct kmem_cache *s, gfp_t flags, size_t size)
+ __assume_slab_alignment __alloc_size(3);
void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
gfp_t gfpflags) __assume_slab_alignment __malloc;
void kmem_cache_free(struct kmem_cache *s, void *objp);
@@ -469,6 +471,8 @@ void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_kmalloc_alignm
__alloc_size(1);
void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t flags, int node) __assume_slab_alignment
__malloc;
+void *kmem_cache_alloc_node_sized(struct kmem_cache *s, gfp_t flags, int node, size_t size)
+ __assume_slab_alignment __alloc_size(4);
#ifdef CONFIG_TRACING
void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
@@ -482,7 +486,7 @@ void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
static __always_inline __alloc_size(3)
void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
{
- void *ret = kmem_cache_alloc(s, flags);
+ void *ret = kmem_cache_alloc_sized(s, flags, size);
ret = kasan_kmalloc(s, ret, size, flags);
return ret;
@@ -492,7 +496,7 @@ static __always_inline __alloc_size(4)
void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
int node, size_t size)
{
- void *ret = kmem_cache_alloc_node(s, gfpflags, node);
+ void *ret = kmem_cache_alloc_node_sized(s, gfpflags, node, size);
ret = kasan_kmalloc(s, ret, size, gfpflags);
return ret;
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 33b1886b06eb..5fa547539a6a 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -1457,6 +1457,20 @@ size_t ksize(const void *objp)
}
EXPORT_SYMBOL(ksize);
+/* Wrapper so __alloc_size() can see the actual allocation size. */
+void *kmem_cache_alloc_sized(struct kmem_cache *s, gfp_t flags, size_t size)
+{
+ return kmem_cache_alloc(s, flags);
+}
+EXPORT_SYMBOL(kmem_cache_alloc_sized);
+
+/* Wrapper so __alloc_size() can see the actual allocation size. */
+void *kmem_cache_alloc_node_sized(struct kmem_cache *s, gfp_t flags, int node, size_t size)
+{
+ return kmem_cache_alloc_node(s, flags, node);
+}
+EXPORT_SYMBOL(kmem_cache_alloc_node_sized);
+
/* Tracepoints definitions. */
EXPORT_TRACEPOINT_SYMBOL(kmalloc);
EXPORT_TRACEPOINT_SYMBOL(kmem_cache_alloc);
--
2.34.1
Add __realloc_size() hint to kmemdup() so the compiler can reason about
the length of the returned buffer. (These must not use __alloc_size,
since those include __malloc which says the contents aren't defined[1]).
[1] https://lore.kernel.org/linux-hardening/[email protected]/
Cc: Rasmus Villemoes <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/fortify-string.h | 2 +-
include/linux/string.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 9e2d96993c30..aa31f54f8b57 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -670,7 +670,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
}
extern void *__real_kmemdup(const void *src, size_t len, gfp_t gfp) __RENAME(kmemdup)
- __alloc_size(2);
+ __realloc_size(2);
__FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp)
{
size_t p_size = __struct_size(p);
diff --git a/include/linux/string.h b/include/linux/string.h
index af1d69e5610e..db28802ab0a6 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -176,7 +176,7 @@ extern void kfree_const(const void *x);
extern char *kstrdup(const char *s, gfp_t gfp) __malloc;
extern const char *kstrdup_const(const char *s, gfp_t gfp);
extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
-extern void *kmemdup(const void *src, size_t len, gfp_t gfp) __alloc_size(2);
+extern void *kmemdup(const void *src, size_t len, gfp_t gfp) __realloc_size(2);
extern char *kmemdup_nul(const char *s, size_t len, gfp_t gfp);
extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
--
2.34.1
Validate the effect of the __alloc_size attribute on allocators. If the
compiler doesn't support __builtin_dynamic_object_size(), skip the
associated tests.
(For GCC, just remove the "--make_options" line below...)
$ ./tools/testing/kunit/kunit.py run --arch x86_64 \
--kconfig_add CONFIG_FORTIFY_SOURCE=y \
--make_options LLVM=1
fortify
...
[15:16:30] ================== fortify (10 subtests) ===================
[15:16:30] [PASSED] known_sizes_test
[15:16:30] [PASSED] control_flow_split_test
[15:16:30] [PASSED] alloc_size_kmalloc_const_test
[15:16:30] [PASSED] alloc_size_kmalloc_dynamic_test
[15:16:30] [PASSED] alloc_size_vmalloc_const_test
[15:16:30] [PASSED] alloc_size_vmalloc_dynamic_test
[15:16:30] [PASSED] alloc_size_kvmalloc_const_test
[15:16:30] [PASSED] alloc_size_kvmalloc_dynamic_test
[15:16:30] [PASSED] alloc_size_devm_kmalloc_const_test
[15:16:30] [PASSED] alloc_size_devm_kmalloc_dynamic_test
[15:16:30] ===================== [PASSED] fortify =====================
[15:16:30] ============================================================
[15:16:30] Testing complete. Ran 10 tests: passed: 10
[15:16:31] Elapsed time: 8.348s total, 0.002s configuring, 6.923s building, 1.075s running
For earlier GCC prior to version 12, the dynamic tests will be skipped:
[15:18:59] ================== fortify (10 subtests) ===================
[15:18:59] [PASSED] known_sizes_test
[15:18:59] [PASSED] control_flow_split_test
[15:18:59] [PASSED] alloc_size_kmalloc_const_test
[15:18:59] [SKIPPED] alloc_size_kmalloc_dynamic_test
[15:18:59] [PASSED] alloc_size_vmalloc_const_test
[15:18:59] [SKIPPED] alloc_size_vmalloc_dynamic_test
[15:18:59] [PASSED] alloc_size_kvmalloc_const_test
[15:18:59] [SKIPPED] alloc_size_kvmalloc_dynamic_test
[15:18:59] [PASSED] alloc_size_devm_kmalloc_const_test
[15:18:59] [SKIPPED] alloc_size_devm_kmalloc_dynamic_test
[15:18:59] ===================== [PASSED] fortify =====================
[15:18:59] ============================================================
[15:18:59] Testing complete. Ran 10 tests: passed: 6, skipped: 4
[15:18:59] Elapsed time: 11.965s total, 0.002s configuring, 10.540s building, 1.068s running
Cc: David Gow <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
lib/Makefile | 1 +
lib/fortify_kunit.c | 255 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 256 insertions(+)
diff --git a/lib/Makefile b/lib/Makefile
index 77c7951c8cf0..d197079ef22a 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -377,6 +377,7 @@ obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_kunit.o
obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable)
obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o
+CFLAGS_fortify_kunit.o += $(call cc-disable-warning, unsequenced)
obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o
obj-$(CONFIG_STRSCPY_KUNIT_TEST) += strscpy_kunit.o
obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o
diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c
index 409af07f340a..78acfdbda835 100644
--- a/lib/fortify_kunit.c
+++ b/lib/fortify_kunit.c
@@ -16,7 +16,10 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <kunit/test.h>
+#include <linux/device.h>
+#include <linux/slab.h>
#include <linux/string.h>
+#include <linux/vmalloc.h>
static const char array_of_10[] = "this is 10";
static const char *ptr_of_11 = "this is 11!";
@@ -60,9 +63,261 @@ static void control_flow_split_test(struct kunit *test)
KUNIT_EXPECT_EQ(test, want_minus_one(pick), SIZE_MAX);
}
+#define KUNIT_EXPECT_BOS(test, p, expected, name) \
+ KUNIT_EXPECT_EQ_MSG(test, __builtin_object_size(p, 1), \
+ expected, \
+ "__alloc_size() not working with __bos on " name "\n")
+
+#if !__has_builtin(__builtin_dynamic_object_size)
+#define KUNIT_EXPECT_BDOS(test, p, expected, name) \
+ /* Silence "unused variable 'expected'" warning. */ \
+ KUNIT_EXPECT_EQ(test, expected, expected)
+#else
+#define KUNIT_EXPECT_BDOS(test, p, expected, name) \
+ KUNIT_EXPECT_EQ_MSG(test, __builtin_dynamic_object_size(p, 1), \
+ expected, \
+ "__alloc_size() not working with __bdos on " name "\n")
+#endif
+
+/* If the execpted size is a constant value, __bos can see it. */
+#define check_const(_expected, alloc, free) do { \
+ size_t expected = (_expected); \
+ void *p = alloc; \
+ KUNIT_EXPECT_TRUE_MSG(test, p != NULL, #alloc " failed?!\n"); \
+ KUNIT_EXPECT_BOS(test, p, expected, #alloc); \
+ KUNIT_EXPECT_BDOS(test, p, expected, #alloc); \
+ free; \
+} while (0)
+
+/* If the execpted size is NOT a constant value, __bos CANNOT see it. */
+#define check_dynamic(_expected, alloc, free) do { \
+ size_t expected = (_expected); \
+ void *p = alloc; \
+ KUNIT_EXPECT_TRUE_MSG(test, p != NULL, #alloc " failed?!\n"); \
+ KUNIT_EXPECT_BOS(test, p, SIZE_MAX, #alloc); \
+ KUNIT_EXPECT_BDOS(test, p, expected, #alloc); \
+ free; \
+} while (0)
+
+/* Assortment of constant-value kinda-edge cases. */
+#define CONST_TEST_BODY(TEST_alloc) do { \
+ /* Special-case vmalloc()-family to skip 0-sized allocs. */ \
+ if (strcmp(#TEST_alloc, "TEST_vmalloc") != 0) \
+ TEST_alloc(check_const, 0, 0); \
+ TEST_alloc(check_const, 1, 1); \
+ TEST_alloc(check_const, 128, 128); \
+ TEST_alloc(check_const, 1023, 1023); \
+ TEST_alloc(check_const, 1025, 1025); \
+ TEST_alloc(check_const, 4096, 4096); \
+ TEST_alloc(check_const, 4097, 4097); \
+} while (0)
+
+static volatile size_t zero_size;
+static volatile size_t unknown_size = 50;
+
+#if !__has_builtin(__builtin_dynamic_object_size)
+#define DYNAMIC_TEST_BODY(TEST_alloc) \
+ kunit_skip(test, "Compiler is missing __builtin_dynamic_object_size() support\n")
+#else
+#define DYNAMIC_TEST_BODY(TEST_alloc) do { \
+ size_t size = unknown_size; \
+ \
+ /* \
+ * Expected size is "size" in each test, before it is then \
+ * internally incremented in each test. Requires we disable \
+ * -Wunsequenced. \
+ */ \
+ TEST_alloc(check_dynamic, size, size++); \
+ /* Make sure incrementing actually happened. */ \
+ KUNIT_EXPECT_NE(test, size, unknown_size); \
+} while (0)
+#endif
+
+#define DEFINE_ALLOC_SIZE_TEST_PAIR(allocator) \
+static void alloc_size_##allocator##_const_test(struct kunit *test) \
+{ \
+ CONST_TEST_BODY(TEST_##allocator); \
+} \
+static void alloc_size_##allocator##_dynamic_test(struct kunit *test) \
+{ \
+ DYNAMIC_TEST_BODY(TEST_##allocator); \
+}
+
+#define TEST_kmalloc(checker, expected_size, alloc_size) do { \
+ gfp_t gfp = GFP_KERNEL | __GFP_NOWARN; \
+ void *orig; \
+ size_t len; \
+ \
+ checker(expected_size, kmalloc(alloc_size, gfp), \
+ kfree(p)); \
+ checker(expected_size, \
+ kmalloc_node(alloc_size, gfp, NUMA_NO_NODE), \
+ kfree(p)); \
+ checker(expected_size, kzalloc(alloc_size, gfp), \
+ kfree(p)); \
+ checker(expected_size, \
+ kzalloc_node(alloc_size, gfp, NUMA_NO_NODE), \
+ kfree(p)); \
+ checker(expected_size, kcalloc(1, alloc_size, gfp), \
+ kfree(p)); \
+ checker(expected_size, kcalloc(alloc_size, 1, gfp), \
+ kfree(p)); \
+ checker(expected_size, \
+ kcalloc_node(1, alloc_size, gfp, NUMA_NO_NODE), \
+ kfree(p)); \
+ checker(expected_size, \
+ kcalloc_node(alloc_size, 1, gfp, NUMA_NO_NODE), \
+ kfree(p)); \
+ checker(expected_size, kmalloc_array(1, alloc_size, gfp), \
+ kfree(p)); \
+ checker(expected_size, kmalloc_array(alloc_size, 1, gfp), \
+ kfree(p)); \
+ checker(expected_size, \
+ kmalloc_array_node(1, alloc_size, gfp, NUMA_NO_NODE), \
+ kfree(p)); \
+ checker(expected_size, \
+ kmalloc_array_node(alloc_size, 1, gfp, NUMA_NO_NODE), \
+ kfree(p)); \
+ checker(expected_size, __kmalloc(alloc_size, gfp), \
+ kfree(p)); \
+ checker(expected_size, \
+ __kmalloc_node(alloc_size, gfp, NUMA_NO_NODE), \
+ kfree(p)); \
+ \
+ orig = kmalloc(alloc_size, gfp); \
+ KUNIT_EXPECT_TRUE(test, orig != NULL); \
+ checker((expected_size) * 2, \
+ krealloc(orig, (alloc_size) * 2, gfp), \
+ kfree(p)); \
+ orig = kmalloc(alloc_size, gfp); \
+ KUNIT_EXPECT_TRUE(test, orig != NULL); \
+ checker((expected_size) * 2, \
+ krealloc_array(orig, 1, (alloc_size) * 2, gfp), \
+ kfree(p)); \
+ orig = kmalloc(alloc_size, gfp); \
+ KUNIT_EXPECT_TRUE(test, orig != NULL); \
+ checker((expected_size) * 2, \
+ krealloc_array(orig, (alloc_size) * 2, 1, gfp), \
+ kfree(p)); \
+ \
+ len = 11; \
+ /* Using memdup() with fixed size, so force unknown length. */ \
+ if (!__builtin_constant_p(expected_size)) \
+ len += zero_size; \
+ checker(len, kmemdup("hello there", len, gfp), kfree(p)); \
+} while (0)
+DEFINE_ALLOC_SIZE_TEST_PAIR(kmalloc)
+
+/* Sizes are in pages, not bytes. */
+#define TEST_vmalloc(checker, expected_pages, alloc_pages) do { \
+ gfp_t gfp = GFP_KERNEL | __GFP_NOWARN; \
+ checker((expected_pages) * PAGE_SIZE, \
+ vmalloc((alloc_pages) * PAGE_SIZE), vfree(p)); \
+ checker((expected_pages) * PAGE_SIZE, \
+ vzalloc((alloc_pages) * PAGE_SIZE), vfree(p)); \
+ checker((expected_pages) * PAGE_SIZE, \
+ __vmalloc((alloc_pages) * PAGE_SIZE, gfp), vfree(p)); \
+} while (0)
+DEFINE_ALLOC_SIZE_TEST_PAIR(vmalloc)
+
+/* Sizes are in pages (and open-coded for side-effects), not bytes. */
+#define TEST_kvmalloc(checker, expected_pages, alloc_pages) do { \
+ gfp_t gfp = GFP_KERNEL | __GFP_NOWARN; \
+ size_t prev_size; \
+ void *orig; \
+ \
+ checker((expected_pages) * PAGE_SIZE, \
+ kvmalloc((alloc_pages) * PAGE_SIZE, gfp), \
+ vfree(p)); \
+ checker((expected_pages) * PAGE_SIZE, \
+ kvmalloc_node((alloc_pages) * PAGE_SIZE, gfp, NUMA_NO_NODE), \
+ vfree(p)); \
+ checker((expected_pages) * PAGE_SIZE, \
+ kvzalloc((alloc_pages) * PAGE_SIZE, gfp), \
+ vfree(p)); \
+ checker((expected_pages) * PAGE_SIZE, \
+ kvzalloc_node((alloc_pages) * PAGE_SIZE, gfp, NUMA_NO_NODE), \
+ vfree(p)); \
+ checker((expected_pages) * PAGE_SIZE, \
+ kvcalloc(1, (alloc_pages) * PAGE_SIZE, gfp), \
+ vfree(p)); \
+ checker((expected_pages) * PAGE_SIZE, \
+ kvcalloc((alloc_pages) * PAGE_SIZE, 1, gfp), \
+ vfree(p)); \
+ checker((expected_pages) * PAGE_SIZE, \
+ kvmalloc_array(1, (alloc_pages) * PAGE_SIZE, gfp), \
+ vfree(p)); \
+ checker((expected_pages) * PAGE_SIZE, \
+ kvmalloc_array((alloc_pages) * PAGE_SIZE, 1, gfp), \
+ vfree(p)); \
+ \
+ prev_size = (expected_pages) * PAGE_SIZE; \
+ orig = kvmalloc(prev_size, gfp); \
+ KUNIT_EXPECT_TRUE(test, orig != NULL); \
+ checker(((expected_pages) * PAGE_SIZE) * 2, \
+ kvrealloc(orig, prev_size, \
+ ((alloc_pages) * PAGE_SIZE) * 2, gfp), \
+ kvfree(p)); \
+} while (0)
+DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)
+
+#define TEST_devm_kmalloc(checker, expected_size, alloc_size) do { \
+ gfp_t gfp = GFP_KERNEL | __GFP_NOWARN; \
+ const char const dev_name[] = "fortify-test"; \
+ struct device *dev; \
+ void *orig; \
+ size_t len; \
+ \
+ /* Create dummy device for devm_kmalloc()-family tests. */ \
+ dev = root_device_register(dev_name); \
+ KUNIT_ASSERT_FALSE_MSG(test, IS_ERR(dev), \
+ "Cannot register test device\n"); \
+ \
+ checker(expected_size, devm_kmalloc(dev, alloc_size, gfp), \
+ devm_kfree(dev, p)); \
+ checker(expected_size, devm_kzalloc(dev, alloc_size, gfp), \
+ devm_kfree(dev, p)); \
+ checker(expected_size, \
+ devm_kmalloc_array(dev, 1, alloc_size, gfp), \
+ devm_kfree(dev, p)); \
+ checker(expected_size, \
+ devm_kmalloc_array(dev, alloc_size, 1, gfp), \
+ devm_kfree(dev, p)); \
+ checker(expected_size, \
+ devm_kcalloc(dev, 1, alloc_size, gfp), \
+ devm_kfree(dev, p)); \
+ checker(expected_size, \
+ devm_kcalloc(dev, alloc_size, 1, gfp), \
+ devm_kfree(dev, p)); \
+ \
+ orig = devm_kmalloc(dev, alloc_size, gfp); \
+ KUNIT_EXPECT_TRUE(test, orig != NULL); \
+ checker((expected_size) * 2, \
+ devm_krealloc(dev, orig, (alloc_size) * 2, gfp), \
+ devm_kfree(dev, p)); \
+ \
+ len = 4; \
+ /* Using memdup() with fixed size, so force unknown length. */ \
+ if (!__builtin_constant_p(expected_size)) \
+ len += zero_size; \
+ checker(len, devm_kmemdup(dev, "Ohai", len, gfp), \
+ devm_kfree(dev, p)); \
+ \
+ device_unregister(dev); \
+} while (0)
+DEFINE_ALLOC_SIZE_TEST_PAIR(devm_kmalloc)
+
static struct kunit_case fortify_test_cases[] = {
KUNIT_CASE(known_sizes_test),
KUNIT_CASE(control_flow_split_test),
+ KUNIT_CASE(alloc_size_kmalloc_const_test),
+ KUNIT_CASE(alloc_size_kmalloc_dynamic_test),
+ KUNIT_CASE(alloc_size_vmalloc_const_test),
+ KUNIT_CASE(alloc_size_vmalloc_dynamic_test),
+ KUNIT_CASE(alloc_size_kvmalloc_const_test),
+ KUNIT_CASE(alloc_size_kvmalloc_dynamic_test),
+ KUNIT_CASE(alloc_size_devm_kmalloc_const_test),
+ KUNIT_CASE(alloc_size_devm_kmalloc_dynamic_test),
{}
};
--
2.34.1
Mark the devm_*alloc()-family of allocations with appropriate
__alloc_size()/__realloc_size() hints so the compiler can attempt to
reason about buffer lengths from allocations.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Nishanth Menon <[email protected]>
Cc: Michael Kelley <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Won Chung <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
This is already in -next, but I'm including it here again to avoid any
confusion about this series landing (or being tested) via another tree.
---
include/linux/device.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/linux/device.h b/include/linux/device.h
index 424b55df0272..5e4cd857e74f 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -197,9 +197,9 @@ void devres_remove_group(struct device *dev, void *id);
int devres_release_group(struct device *dev, void *id);
/* managed devm_k.alloc/kfree for device drivers */
-void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __malloc;
+void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __alloc_size(2);
void *devm_krealloc(struct device *dev, void *ptr, size_t size,
- gfp_t gfp) __must_check;
+ gfp_t gfp) __must_check __realloc_size(3);
__printf(3, 0) char *devm_kvasprintf(struct device *dev, gfp_t gfp,
const char *fmt, va_list ap) __malloc;
__printf(3, 4) char *devm_kasprintf(struct device *dev, gfp_t gfp,
@@ -226,7 +226,8 @@ static inline void *devm_kcalloc(struct device *dev,
void devm_kfree(struct device *dev, const void *p);
char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp);
-void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp);
+void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp)
+ __realloc_size(3);
unsigned long devm_get_free_pages(struct device *dev,
gfp_t gfp_mask, unsigned int order);
--
2.34.1
As already done for kmalloc_node(), clean up the #ifdef usage in the
definition of kmalloc() so that the SLOB-only version is an entirely
separate and much more readable function.
Cc: Vlastimil Babka <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Roman Gushchin <[email protected]>
Cc: Hyeonggon Yoo <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/slab.h | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 90877fcde70b..e08fe7978b5c 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -559,15 +559,15 @@ void *kmalloc_large_node(size_t size, gfp_t flags, int node) __assume_page_align
* Try really hard to succeed the allocation but fail
* eventually.
*/
+#ifndef CONFIG_SLOB
static __always_inline __alloc_size(1) void *kmalloc(size_t size, gfp_t flags)
{
if (__builtin_constant_p(size)) {
-#ifndef CONFIG_SLOB
unsigned int index;
-#endif
+
if (size > KMALLOC_MAX_CACHE_SIZE)
return kmalloc_large(size, flags);
-#ifndef CONFIG_SLOB
+
index = kmalloc_index(size);
if (!index)
@@ -576,10 +576,18 @@ static __always_inline __alloc_size(1) void *kmalloc(size_t size, gfp_t flags)
return kmalloc_trace(
kmalloc_caches[kmalloc_type(flags)][index],
flags, size);
-#endif
}
return __kmalloc(size, flags);
}
+#else
+static __always_inline __alloc_size(1) void *kmalloc(size_t size, gfp_t flags)
+{
+ if (__builtin_constant_p(size) && size > KMALLOC_MAX_CACHE_SIZE)
+ return kmalloc_large(size, flags);
+
+ return __kmalloc(size, flags);
+}
+#endif
#ifndef CONFIG_SLOB
static __always_inline __alloc_size(1) void *kmalloc_node(size_t size, gfp_t flags, int node)
--
2.34.1
On 01/11/2022 23.33, Kees Cook wrote:
> Add __realloc_size() hint to kmemdup() so the compiler can reason about
> the length of the returned buffer. (These must not use __alloc_size,
> since those include __malloc which says the contents aren't defined[1]).
>
> [1] https://lore.kernel.org/linux-hardening/[email protected]/
>
> extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
> -extern void *kmemdup(const void *src, size_t len, gfp_t gfp) __alloc_size(2);
> +extern void *kmemdup(const void *src, size_t len, gfp_t gfp) __realloc_size(2);
What tree is this based on? I see that kmemdup() has grown that bogus
__alloc_size in next-20221101, but in next-20221102 this commit seems to
DTRT, namely
-extern void *kmemdup(const void *src, size_t len, gfp_t gfp);
+extern void *kmemdup(const void *src, size_t len, gfp_t gfp)
__realloc_size(2);
(i.e. there should never be an intermediate commit where kmemdup has
__alloc_size()).
Rasmus
On Wed, Nov 02, 2022 at 10:26:40AM +0100, Rasmus Villemoes wrote:
> On 01/11/2022 23.33, Kees Cook wrote:
> > Add __realloc_size() hint to kmemdup() so the compiler can reason about
> > the length of the returned buffer. (These must not use __alloc_size,
> > since those include __malloc which says the contents aren't defined[1]).
> >
> > [1] https://lore.kernel.org/linux-hardening/[email protected]/
> >
>
> > extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
> > -extern void *kmemdup(const void *src, size_t len, gfp_t gfp) __alloc_size(2);
> > +extern void *kmemdup(const void *src, size_t len, gfp_t gfp) __realloc_size(2);
>
> What tree is this based on? I see that kmemdup() has grown that bogus
> __alloc_size in next-20221101, but in next-20221102 this commit seems to
> DTRT, namely
>
> -extern void *kmemdup(const void *src, size_t len, gfp_t gfp);
> +extern void *kmemdup(const void *src, size_t len, gfp_t gfp)
> __realloc_size(2);
>
> (i.e. there should never be an intermediate commit where kmemdup has
> __alloc_size()).
Right -- I fixed in in my -next tree to not use __alloc_size.
--
Kees Cook
On Tue, Nov 01, 2022 at 03:33:09PM -0700, Kees Cook wrote:
> As already done for kmalloc_node(), clean up the #ifdef usage in the
> definition of kmalloc() so that the SLOB-only version is an entirely
> separate and much more readable function.
>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Cc: David Rientjes <[email protected]>
> Cc: Joonsoo Kim <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Roman Gushchin <[email protected]>
> Cc: Hyeonggon Yoo <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> include/linux/slab.h | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 90877fcde70b..e08fe7978b5c 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -559,15 +559,15 @@ void *kmalloc_large_node(size_t size, gfp_t flags, int node) __assume_page_align
> * Try really hard to succeed the allocation but fail
> * eventually.
> */
> +#ifndef CONFIG_SLOB
> static __always_inline __alloc_size(1) void *kmalloc(size_t size, gfp_t flags)
> {
> if (__builtin_constant_p(size)) {
> -#ifndef CONFIG_SLOB
> unsigned int index;
> -#endif
> +
> if (size > KMALLOC_MAX_CACHE_SIZE)
> return kmalloc_large(size, flags);
> -#ifndef CONFIG_SLOB
> +
> index = kmalloc_index(size);
>
> if (!index)
> @@ -576,10 +576,18 @@ static __always_inline __alloc_size(1) void *kmalloc(size_t size, gfp_t flags)
> return kmalloc_trace(
> kmalloc_caches[kmalloc_type(flags)][index],
> flags, size);
> -#endif
> }
> return __kmalloc(size, flags);
> }
> +#else
> +static __always_inline __alloc_size(1) void *kmalloc(size_t size, gfp_t flags)
> +{
> + if (__builtin_constant_p(size) && size > KMALLOC_MAX_CACHE_SIZE)
> + return kmalloc_large(size, flags);
> +
> + return __kmalloc(size, flags);
> +}
> +#endif
>
> #ifndef CONFIG_SLOB
> static __always_inline __alloc_size(1) void *kmalloc_node(size_t size, gfp_t flags, int node)
> --
> 2.34.1
Looks good to me.
Reviewed-by: Hyeonggon Yoo <[email protected]>
--
Thanks,
Hyeonggon
On Tue, Nov 01, 2022 at 03:33:11PM -0700, Kees Cook wrote:
> Since GCC cannot apply the __alloc_size attributes to inlines[1], all
> allocator inlines need to explicitly call into extern functions that
> contain a size argument. Provide these wrappers that end up just
> ignoring the size argument for the actual allocation.
>
> This allows CONFIG_FORTIFY_SOURCE=y to see all various dynamic allocation
> sizes under GCC 12+ and all supported Clang versions.
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503
>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Cc: David Rientjes <[email protected]>
> Cc: Joonsoo Kim <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Roman Gushchin <[email protected]>
> Cc: Hyeonggon Yoo <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> include/linux/slab.h | 8 ++++++--
> mm/slab_common.c | 14 ++++++++++++++
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 970e9504949e..051d86ca31a8 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -442,6 +442,8 @@ static_assert(PAGE_SHIFT <= 20);
>
> void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
> void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
> +void *kmem_cache_alloc_sized(struct kmem_cache *s, gfp_t flags, size_t size)
> + __assume_slab_alignment __alloc_size(3);
> void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> gfp_t gfpflags) __assume_slab_alignment __malloc;
> void kmem_cache_free(struct kmem_cache *s, void *objp);
> @@ -469,6 +471,8 @@ void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_kmalloc_alignm
> __alloc_size(1);
> void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t flags, int node) __assume_slab_alignment
> __malloc;
> +void *kmem_cache_alloc_node_sized(struct kmem_cache *s, gfp_t flags, int node, size_t size)
> + __assume_slab_alignment __alloc_size(4);
>
> #ifdef CONFIG_TRACING
> void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
> @@ -482,7 +486,7 @@ void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
> static __always_inline __alloc_size(3)
> void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
> {
> - void *ret = kmem_cache_alloc(s, flags);
> + void *ret = kmem_cache_alloc_sized(s, flags, size);
>
> ret = kasan_kmalloc(s, ret, size, flags);
> return ret;
> @@ -492,7 +496,7 @@ static __always_inline __alloc_size(4)
> void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
> int node, size_t size)
> {
> - void *ret = kmem_cache_alloc_node(s, gfpflags, node);
> + void *ret = kmem_cache_alloc_node_sized(s, gfpflags, node, size);
>
> ret = kasan_kmalloc(s, ret, size, gfpflags);
> return ret;
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 33b1886b06eb..5fa547539a6a 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1457,6 +1457,20 @@ size_t ksize(const void *objp)
> }
> EXPORT_SYMBOL(ksize);
>
> +/* Wrapper so __alloc_size() can see the actual allocation size. */
> +void *kmem_cache_alloc_sized(struct kmem_cache *s, gfp_t flags, size_t size)
> +{
> + return kmem_cache_alloc(s, flags);
> +}
> +EXPORT_SYMBOL(kmem_cache_alloc_sized);
> +
> +/* Wrapper so __alloc_size() can see the actual allocation size. */
> +void *kmem_cache_alloc_node_sized(struct kmem_cache *s, gfp_t flags, int node, size_t size)
> +{
> + return kmem_cache_alloc_node(s, flags, node);
> +}
> +EXPORT_SYMBOL(kmem_cache_alloc_node_sized);
The reason that we have two implementations of kmalloc_trace*
depending on CONFIG_TRACING is to save additional function call when
CONFIG_TRACING is not set.
With this patch there is no reason to keep both.
So let's drop #ifdefs and use single implementation in mm/slab_common.c.
Thanks!
> /* Tracepoints definitions. */
> EXPORT_TRACEPOINT_SYMBOL(kmalloc);
> EXPORT_TRACEPOINT_SYMBOL(kmem_cache_alloc);
> --
> 2.34.1
>
--
Hyeonggon
On Thu, Nov 03, 2022 at 11:16:53PM +0900, Hyeonggon Yoo wrote:
> On Tue, Nov 01, 2022 at 03:33:11PM -0700, Kees Cook wrote:
> > Since GCC cannot apply the __alloc_size attributes to inlines[1], all
> > allocator inlines need to explicitly call into extern functions that
> > contain a size argument. Provide these wrappers that end up just
> > ignoring the size argument for the actual allocation.
> >
> > This allows CONFIG_FORTIFY_SOURCE=y to see all various dynamic allocation
> > sizes under GCC 12+ and all supported Clang versions.
> >
> > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503
> >
> > Cc: Vlastimil Babka <[email protected]>
> > Cc: Christoph Lameter <[email protected]>
> > Cc: Pekka Enberg <[email protected]>
> > Cc: David Rientjes <[email protected]>
> > Cc: Joonsoo Kim <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Roman Gushchin <[email protected]>
> > Cc: Hyeonggon Yoo <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > include/linux/slab.h | 8 ++++++--
> > mm/slab_common.c | 14 ++++++++++++++
> > 2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 970e9504949e..051d86ca31a8 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -442,6 +442,8 @@ static_assert(PAGE_SHIFT <= 20);
> >
> > void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
> > void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
> > +void *kmem_cache_alloc_sized(struct kmem_cache *s, gfp_t flags, size_t size)
> > + __assume_slab_alignment __alloc_size(3);
> > void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> > gfp_t gfpflags) __assume_slab_alignment __malloc;
> > void kmem_cache_free(struct kmem_cache *s, void *objp);
> > @@ -469,6 +471,8 @@ void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_kmalloc_alignm
> > __alloc_size(1);
> > void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t flags, int node) __assume_slab_alignment
> > __malloc;
> > +void *kmem_cache_alloc_node_sized(struct kmem_cache *s, gfp_t flags, int node, size_t size)
> > + __assume_slab_alignment __alloc_size(4);
> >
> > #ifdef CONFIG_TRACING
> > void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
> > @@ -482,7 +486,7 @@ void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
> > static __always_inline __alloc_size(3)
> > void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
> > {
> > - void *ret = kmem_cache_alloc(s, flags);
> > + void *ret = kmem_cache_alloc_sized(s, flags, size);
> >
> > ret = kasan_kmalloc(s, ret, size, flags);
> > return ret;
> > @@ -492,7 +496,7 @@ static __always_inline __alloc_size(4)
> > void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
> > int node, size_t size)
> > {
> > - void *ret = kmem_cache_alloc_node(s, gfpflags, node);
> > + void *ret = kmem_cache_alloc_node_sized(s, gfpflags, node, size);
> >
> > ret = kasan_kmalloc(s, ret, size, gfpflags);
> > return ret;
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index 33b1886b06eb..5fa547539a6a 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -1457,6 +1457,20 @@ size_t ksize(const void *objp)
> > }
> > EXPORT_SYMBOL(ksize);
> >
> > +/* Wrapper so __alloc_size() can see the actual allocation size. */
> > +void *kmem_cache_alloc_sized(struct kmem_cache *s, gfp_t flags, size_t size)
> > +{
> > + return kmem_cache_alloc(s, flags);
> > +}
> > +EXPORT_SYMBOL(kmem_cache_alloc_sized);
> > +
> > +/* Wrapper so __alloc_size() can see the actual allocation size. */
> > +void *kmem_cache_alloc_node_sized(struct kmem_cache *s, gfp_t flags, int node, size_t size)
> > +{
> > + return kmem_cache_alloc_node(s, flags, node);
> > +}
> > +EXPORT_SYMBOL(kmem_cache_alloc_node_sized);
>
> The reason that we have two implementations of kmalloc_trace*
> depending on CONFIG_TRACING is to save additional function call when
> CONFIG_TRACING is not set.
>
> With this patch there is no reason to keep both.
> So let's drop #ifdefs and use single implementation in mm/slab_common.c.
Okay, I'll respin...
--
Kees Cook
On Fri, Nov 04, 2022 at 11:22:42AM -0700, Kees Cook wrote:
> On Thu, Nov 03, 2022 at 11:16:53PM +0900, Hyeonggon Yoo wrote:
> > On Tue, Nov 01, 2022 at 03:33:11PM -0700, Kees Cook wrote:
> > > Since GCC cannot apply the __alloc_size attributes to inlines[1], all
> > > allocator inlines need to explicitly call into extern functions that
> > > contain a size argument. Provide these wrappers that end up just
> > > ignoring the size argument for the actual allocation.
> > >
> > > This allows CONFIG_FORTIFY_SOURCE=y to see all various dynamic allocation
> > > sizes under GCC 12+ and all supported Clang versions.
> > >
> > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503
> > >
> > > Cc: Vlastimil Babka <[email protected]>
> > > Cc: Christoph Lameter <[email protected]>
> > > Cc: Pekka Enberg <[email protected]>
> > > Cc: David Rientjes <[email protected]>
> > > Cc: Joonsoo Kim <[email protected]>
> > > Cc: Andrew Morton <[email protected]>
> > > Cc: Roman Gushchin <[email protected]>
> > > Cc: Hyeonggon Yoo <[email protected]>
> > > Cc: [email protected]
> > > Signed-off-by: Kees Cook <[email protected]>
> > > ---
> > > include/linux/slab.h | 8 ++++++--
> > > mm/slab_common.c | 14 ++++++++++++++
> > > 2 files changed, 20 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > > index 970e9504949e..051d86ca31a8 100644
> > > --- a/include/linux/slab.h
> > > +++ b/include/linux/slab.h
> > > @@ -442,6 +442,8 @@ static_assert(PAGE_SHIFT <= 20);
> > >
> > > void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
> > > void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
> > > +void *kmem_cache_alloc_sized(struct kmem_cache *s, gfp_t flags, size_t size)
> > > + __assume_slab_alignment __alloc_size(3);
> > > void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> > > gfp_t gfpflags) __assume_slab_alignment __malloc;
> > > void kmem_cache_free(struct kmem_cache *s, void *objp);
> > > @@ -469,6 +471,8 @@ void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_kmalloc_alignm
> > > __alloc_size(1);
> > > void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t flags, int node) __assume_slab_alignment
> > > __malloc;
> > > +void *kmem_cache_alloc_node_sized(struct kmem_cache *s, gfp_t flags, int node, size_t size)
> > > + __assume_slab_alignment __alloc_size(4);
> > >
> > > #ifdef CONFIG_TRACING
> > > void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
> > > @@ -482,7 +486,7 @@ void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
> > > static __always_inline __alloc_size(3)
> > > void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
> > > {
> > > - void *ret = kmem_cache_alloc(s, flags);
> > > + void *ret = kmem_cache_alloc_sized(s, flags, size);
> > >
> > > ret = kasan_kmalloc(s, ret, size, flags);
> > > return ret;
> > > @@ -492,7 +496,7 @@ static __always_inline __alloc_size(4)
> > > void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
> > > int node, size_t size)
> > > {
> > > - void *ret = kmem_cache_alloc_node(s, gfpflags, node);
> > > + void *ret = kmem_cache_alloc_node_sized(s, gfpflags, node, size);
> > >
> > > ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > return ret;
> > > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > > index 33b1886b06eb..5fa547539a6a 100644
> > > --- a/mm/slab_common.c
> > > +++ b/mm/slab_common.c
> > > @@ -1457,6 +1457,20 @@ size_t ksize(const void *objp)
> > > }
> > > EXPORT_SYMBOL(ksize);
> > >
> > > +/* Wrapper so __alloc_size() can see the actual allocation size. */
> > > +void *kmem_cache_alloc_sized(struct kmem_cache *s, gfp_t flags, size_t size)
> > > +{
> > > + return kmem_cache_alloc(s, flags);
> > > +}
> > > +EXPORT_SYMBOL(kmem_cache_alloc_sized);
> > > +
> > > +/* Wrapper so __alloc_size() can see the actual allocation size. */
> > > +void *kmem_cache_alloc_node_sized(struct kmem_cache *s, gfp_t flags, int node, size_t size)
> > > +{
> > > + return kmem_cache_alloc_node(s, flags, node);
> > > +}
> > > +EXPORT_SYMBOL(kmem_cache_alloc_node_sized);
> >
> > The reason that we have two implementations of kmalloc_trace*
> > depending on CONFIG_TRACING is to save additional function call when
> > CONFIG_TRACING is not set.
> >
> > With this patch there is no reason to keep both.
> > So let's drop #ifdefs and use single implementation in mm/slab_common.c.
>
> Okay, I'll respin...
>
> --
> Kees Cook
Oh, it seems Vlastimil already did that.
Maybe simply drop this patch in next spin?
https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=slab/for-6.1-rc4/fixes&id=eb4940d4adf590590a9d0c47e38d2799c2ff9670
Thanks!
--
Hyeonggon
On Sat, Nov 05, 2022 at 10:09:32AM +0900, Hyeonggon Yoo wrote:
> On Fri, Nov 04, 2022 at 11:22:42AM -0700, Kees Cook wrote:
> > On Thu, Nov 03, 2022 at 11:16:53PM +0900, Hyeonggon Yoo wrote:
> > > On Tue, Nov 01, 2022 at 03:33:11PM -0700, Kees Cook wrote:
> > > > Since GCC cannot apply the __alloc_size attributes to inlines[1], all
> > > > allocator inlines need to explicitly call into extern functions that
> > > > contain a size argument. Provide these wrappers that end up just
> > > > ignoring the size argument for the actual allocation.
> > > >
> > > > This allows CONFIG_FORTIFY_SOURCE=y to see all various dynamic allocation
> > > > sizes under GCC 12+ and all supported Clang versions.
> > > >
> > > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503
> > > >
> > > > Cc: Vlastimil Babka <[email protected]>
> > > > Cc: Christoph Lameter <[email protected]>
> > > > Cc: Pekka Enberg <[email protected]>
> > > > Cc: David Rientjes <[email protected]>
> > > > Cc: Joonsoo Kim <[email protected]>
> > > > Cc: Andrew Morton <[email protected]>
> > > > Cc: Roman Gushchin <[email protected]>
> > > > Cc: Hyeonggon Yoo <[email protected]>
> > > > Cc: [email protected]
> > > > Signed-off-by: Kees Cook <[email protected]>
> > > > ---
> > > > include/linux/slab.h | 8 ++++++--
> > > > mm/slab_common.c | 14 ++++++++++++++
> > > > 2 files changed, 20 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > > > index 970e9504949e..051d86ca31a8 100644
> > > > --- a/include/linux/slab.h
> > > > +++ b/include/linux/slab.h
> > > > @@ -442,6 +442,8 @@ static_assert(PAGE_SHIFT <= 20);
> > > >
> > > > void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
> > > > void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
> > > > +void *kmem_cache_alloc_sized(struct kmem_cache *s, gfp_t flags, size_t size)
> > > > + __assume_slab_alignment __alloc_size(3);
> > > > void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> > > > gfp_t gfpflags) __assume_slab_alignment __malloc;
> > > > void kmem_cache_free(struct kmem_cache *s, void *objp);
> > > > @@ -469,6 +471,8 @@ void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_kmalloc_alignm
> > > > __alloc_size(1);
> > > > void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t flags, int node) __assume_slab_alignment
> > > > __malloc;
> > > > +void *kmem_cache_alloc_node_sized(struct kmem_cache *s, gfp_t flags, int node, size_t size)
> > > > + __assume_slab_alignment __alloc_size(4);
> > > >
> > > > #ifdef CONFIG_TRACING
> > > > void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
> > > > @@ -482,7 +486,7 @@ void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
> > > > static __always_inline __alloc_size(3)
> > > > void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
> > > > {
> > > > - void *ret = kmem_cache_alloc(s, flags);
> > > > + void *ret = kmem_cache_alloc_sized(s, flags, size);
> > > >
> > > > ret = kasan_kmalloc(s, ret, size, flags);
> > > > return ret;
> > > > @@ -492,7 +496,7 @@ static __always_inline __alloc_size(4)
> > > > void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
> > > > int node, size_t size)
> > > > {
> > > > - void *ret = kmem_cache_alloc_node(s, gfpflags, node);
> > > > + void *ret = kmem_cache_alloc_node_sized(s, gfpflags, node, size);
> > > >
> > > > ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > return ret;
> > > > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > > > index 33b1886b06eb..5fa547539a6a 100644
> > > > --- a/mm/slab_common.c
> > > > +++ b/mm/slab_common.c
> > > > @@ -1457,6 +1457,20 @@ size_t ksize(const void *objp)
> > > > }
> > > > EXPORT_SYMBOL(ksize);
> > > >
> > > > +/* Wrapper so __alloc_size() can see the actual allocation size. */
> > > > +void *kmem_cache_alloc_sized(struct kmem_cache *s, gfp_t flags, size_t size)
> > > > +{
> > > > + return kmem_cache_alloc(s, flags);
> > > > +}
> > > > +EXPORT_SYMBOL(kmem_cache_alloc_sized);
> > > > +
> > > > +/* Wrapper so __alloc_size() can see the actual allocation size. */
> > > > +void *kmem_cache_alloc_node_sized(struct kmem_cache *s, gfp_t flags, int node, size_t size)
> > > > +{
> > > > + return kmem_cache_alloc_node(s, flags, node);
> > > > +}
> > > > +EXPORT_SYMBOL(kmem_cache_alloc_node_sized);
> > >
> > > The reason that we have two implementations of kmalloc_trace*
> > > depending on CONFIG_TRACING is to save additional function call when
> > > CONFIG_TRACING is not set.
> > >
> > > With this patch there is no reason to keep both.
> > > So let's drop #ifdefs and use single implementation in mm/slab_common.c.
> >
> > Okay, I'll respin...
> >
> > --
> > Kees Cook
>
> Oh, it seems Vlastimil already did that.
> Maybe simply drop this patch in next spin?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=slab/for-6.1-rc4/fixes&id=eb4940d4adf590590a9d0c47e38d2799c2ff9670
Oh! Well, yes, that makes that easy. :)
--
Kees Cook
On Tue, Nov 29, 2022, at 13:24, Conor Dooley wrote:
> On Tue, Nov 01, 2022 at 03:33:08PM -0700, Kees Cook wrote:
>> Hi,
>>
>> This is a series to work around a deficiency in GCC (>=11) and Clang
>> (<16) where the __alloc_size attribute does not apply to inlines. :(
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503
>>
>> This manifests as reduced overflow detection coverage for many allocation
>> sites under CONFIG_FORTIFY_SOURCE=y, where the allocation size was
>> not actually being propagated to __builtin_dynamic_object_size(). In
>> addition to working around the issue, expand use of __alloc_size (and
>> __realloc_size) to more places and provide KUnit tests to validate all
>> the covered allocator APIs.
>
> Hello Kees!
>
> It would appear that one of the macros you've added here is doing Bad
> Things^TM to allmodconfig on RISC-V since the 22nd:
>
> ../lib/fortify_kunit.c: In function 'alloc_size_kmalloc_const_test':
> ../lib/fortify_kunit.c:140:1: error: the frame size of 2384 bytes is
> larger than 2048 bytes [-Werror=frame-larger-than=]
> 140 | }
> \
> | ^
> ../lib/fortify_kunit.c:209:1: note: in expansion of macro
> 'DEFINE_ALLOC_SIZE_TEST_PAIR'
> 209 | DEFINE_ALLOC_SIZE_TEST_PAIR(kmalloc)
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> CONFIG_GCC_VERSION=110100
> CONFIG_AS_VERSION=23700
> CONFIG_LD_VERSION=23700
>
> The report came out of my CI (which I should have passed on sooner) so
> I do not have anything other than stderr - I can get you anything else
> you'd like/need though if you LMK.
There is generally a conflict between kunit and the structleak
gcc plugin, I think the Makefile needs a line like
CFLAGS_fortify_kunit.o += $(DISABLE_STRUCTLEAK_PLUGIN)
Arnd
On Tue, Nov 01, 2022 at 03:33:08PM -0700, Kees Cook wrote:
> Hi,
>
> This is a series to work around a deficiency in GCC (>=11) and Clang
> (<16) where the __alloc_size attribute does not apply to inlines. :(
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503
>
> This manifests as reduced overflow detection coverage for many allocation
> sites under CONFIG_FORTIFY_SOURCE=y, where the allocation size was
> not actually being propagated to __builtin_dynamic_object_size(). In
> addition to working around the issue, expand use of __alloc_size (and
> __realloc_size) to more places and provide KUnit tests to validate all
> the covered allocator APIs.
Hello Kees!
It would appear that one of the macros you've added here is doing Bad
Things^TM to allmodconfig on RISC-V since the 22nd:
../lib/fortify_kunit.c: In function 'alloc_size_kmalloc_const_test':
../lib/fortify_kunit.c:140:1: error: the frame size of 2384 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
140 | } \
| ^
../lib/fortify_kunit.c:209:1: note: in expansion of macro 'DEFINE_ALLOC_SIZE_TEST_PAIR'
209 | DEFINE_ALLOC_SIZE_TEST_PAIR(kmalloc)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
CONFIG_GCC_VERSION=110100
CONFIG_AS_VERSION=23700
CONFIG_LD_VERSION=23700
The report came out of my CI (which I should have passed on sooner) so
I do not have anything other than stderr - I can get you anything else
you'd like/need though if you LMK.
Thanks,
Conor.
> Kees Cook (6):
> slab: Clean up SLOB vs kmalloc() definition
> slab: Remove special-casing of const 0 size allocations
> slab: Provide functional __alloc_size() hints to kmalloc_trace*()
> string: Add __realloc_size hint to kmemdup()
> driver core: Add __alloc_size hint to devm allocators
> kunit/fortify: Validate __alloc_size attribute results
>
> include/linux/device.h | 7 +-
> include/linux/fortify-string.h | 2 +-
> include/linux/slab.h | 36 ++---
> include/linux/string.h | 2 +-
> lib/Makefile | 1 +
> lib/fortify_kunit.c | 255 +++++++++++++++++++++++++++++++++
> mm/slab_common.c | 14 ++
> 7 files changed, 296 insertions(+), 21 deletions(-)
>
> --
> 2.34.1
>
>
On Tue, Nov 29, 2022 at 01:33:03PM +0100, Arnd Bergmann wrote:
> On Tue, Nov 29, 2022, at 13:24, Conor Dooley wrote:
> > On Tue, Nov 01, 2022 at 03:33:08PM -0700, Kees Cook wrote:
> >> Hi,
> >>
> >> This is a series to work around a deficiency in GCC (>=11) and Clang
> >> (<16) where the __alloc_size attribute does not apply to inlines. :(
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503
> >>
> >> This manifests as reduced overflow detection coverage for many allocation
> >> sites under CONFIG_FORTIFY_SOURCE=y, where the allocation size was
> >> not actually being propagated to __builtin_dynamic_object_size(). In
> >> addition to working around the issue, expand use of __alloc_size (and
> >> __realloc_size) to more places and provide KUnit tests to validate all
> >> the covered allocator APIs.
> >
> > Hello Kees!
> >
> > It would appear that one of the macros you've added here is doing Bad
> > Things^TM to allmodconfig on RISC-V since the 22nd:
> >
> > ../lib/fortify_kunit.c: In function 'alloc_size_kmalloc_const_test':
> > ../lib/fortify_kunit.c:140:1: error: the frame size of 2384 bytes is
> > larger than 2048 bytes [-Werror=frame-larger-than=]
> > 140 | }
> > \
> > | ^
> > ../lib/fortify_kunit.c:209:1: note: in expansion of macro
> > 'DEFINE_ALLOC_SIZE_TEST_PAIR'
> > 209 | DEFINE_ALLOC_SIZE_TEST_PAIR(kmalloc)
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> >
> > CONFIG_GCC_VERSION=110100
> > CONFIG_AS_VERSION=23700
> > CONFIG_LD_VERSION=23700
> >
> > The report came out of my CI (which I should have passed on sooner) so
> > I do not have anything other than stderr - I can get you anything else
> > you'd like/need though if you LMK.
>
> There is generally a conflict between kunit and the structleak
> gcc plugin, I think the Makefile needs a line like
>
> CFLAGS_fortify_kunit.o += $(DISABLE_STRUCTLEAK_PLUGIN)
Thanks for the report! I've taken Anders's patch for this now.
--
Kees Cook
Hi, Kees
This change causes "Kernel panic - not syncing: BRK handler: Fatal exception"
for the android-mainline based hikey960 build, with this commit reverted,
there is no problem for the build to boot to the homescreen.
Not sure if you have any idea about it and give some suggestions.
Here is part of the kernel panic log:
[ 9.479878][ T122] ueventd: Loading module
/vendor/lib/modules/spi-pl022.ko with args ''
[ 9.480276][ T115] apexd-bootstrap: Pre-allocated loop device 29
[ 9.480517][ T123] ueventd: LoadWithAliases was unable to load
of:Nhi3660_i2sT(null)Chisilicon,hi3660-i2s-1.0
[ 9.480632][ T121] Unexpected kernel BRK exception at EL1
[ 9.480637][ T121] Internal error: BRK handler:
00000000f2000001 [#1] PREEMPT SMP
[ 9.480644][ T121] Modules linked in: cpufreq_dt(E+)
hisi_thermal(E+) phy_hi3660_usb3(E) btqca(E) hi6421_pmic_core(E)
btbcm(E) spi_pl022(E) hi3660_mailbox(E) i2c_designware_platform(E)
mali_kbase(OE) dw_mmc_k3(E) bluetooth(E) dw_mmc_pltfm(E) dw_mmc(E)
kirin_drm(E) rfkill(E) kirin_dsi(E) i2c_designware_core(E) k3dma(E)
drm_dma_helper(E) cma_heap(E) system_heap(E)
[ 9.480688][ T121] CPU: 4 PID: 121 Comm: ueventd Tainted: G
OE 6.2.0-rc6-mainline-14196-g1d9f94ec75b9 #1
[ 9.480694][ T121] Hardware name: HiKey960 (DT)
[ 9.480697][ T121] pstate: 20400005 (nzCv daif +PAN -UAO -TCO
-DIT -SSBS BTYPE=--)
[ 9.480703][ T121] pc : hi3660_thermal_probe+0x6c/0x74 [hisi_thermal]
[ 9.480722][ T121] lr : hi3660_thermal_probe+0x38/0x74 [hisi_thermal]
[ 9.480733][ T121] sp : ffffffc00aa13700
[ 9.480735][ T121] x29: ffffffc00aa13700 x28: 0000007ff8ae8531
x27: 00000000000008c0
[ 9.480743][ T121] x26: ffffffc00aa2a300 x25: ffffffc00aa2ab40
x24: 000000000000001d
[ 9.480749][ T121] x23: ffffffc00a29d000 x22: 0000000000000000
x21: ffffff8001fa4a80
[ 9.480755][ T121] x20: 0000000000000001 x19: ffffff8001fa4a80
x18: ffffffc00a8810b0
[ 9.480761][ T121] x17: 000000007ab542f2 x16: 000000007ab542f2
x15: ffffffc00aa01000
[ 9.480767][ T121] x14: ffffffc00966f250 x13: ffffffc0b58f9000
x12: ffffffc00a055f10
[ 9.480771][ T123] ueventd: LoadWithAliases was unable to load
cpu:type:aarch64:feature:,0000,0001,0002,0003,0004,0005,0006,0007,000B
[ 9.480773][ T121]
[ 9.480774][ T121] x11: 0000000000000000 x10: 0000000000000001
x9 : 0000000100000000
[ 9.480780][ T123] ueventd:
[ 9.480780][ T121] x8 : ffffffc0044154cb x7 : 0000000000000000
x6 : 000000000000003f
[ 9.480786][ T121] x5 : 0000000000000020 x4 : ffffffc0098db323
x3 : ffffff801aeb62c0
[ 9.480792][ T121] x2 : ffffff801aeb62c0 x1 : 0000000000000000
x0 : ffffff8001fa4c80
[ 9.480798][ T121] Call trace:
[ 9.480801][ T121] hi3660_thermal_probe+0x6c/0x74 [hisi_thermal]
[ 9.480813][ T121] hisi_thermal_probe+0xbc/0x284 [hisi_thermal]
[ 9.480823][ T121] platform_probe+0xcc/0xf8
[ 9.480836][ T121] really_probe+0x19c/0x390
[ 9.480842][ T121] __driver_probe_device+0xc0/0xf0
[ 9.480848][ T121] driver_probe_device+0x4c/0x228
[ 9.480853][ T121] __driver_attach+0x110/0x1e0
[ 9.480858][ T121] bus_for_each_dev+0xa0/0xf4
[ 9.480864][ T121] driver_attach+0x2c/0x40
[ 9.480868][ T121] bus_add_driver+0x118/0x208
[ 9.480873][ T121] driver_register+0x80/0x124
[ 9.480878][ T121] __platform_driver_register+0x2c/0x40
[ 9.480884][ T121] init_module+0x28/0xfe4 [hisi_thermal]
[ 9.480895][ T121] do_one_initcall+0xe4/0x334
[ 9.480902][ T121] do_init_module+0x50/0x1f0
[ 9.480909][ T121] load_module+0x1034/0x1204
[ 9.480914][ T121] __arm64_sys_finit_module+0xc8/0x11c
[ 9.480919][ T121] invoke_syscall+0x60/0x130
[ 9.480926][ T121] el0_svc_common+0xbc/0x100
[ 9.480931][ T121] do_el0_svc+0x38/0xc4
[ 9.480937][ T121] el0_svc+0x34/0xc4
[ 9.480945][ T121] el0t_64_sync_handler+0x8c/0xfc
[ 9.480950][ T121] el0t_64_sync+0x1a4/0x1a8
[ 9.480957][ T121] Code: 91132d08 b9001814 f9000013 f9000808 (d4200020)
[ 9.480960][ T121] ---[ end trace 0000000000000000 ]---
[ 9.482201][ T72] dwmmc_k3 ff37f000.dwmmc1: IDMAC supports
64-bit address mode.
[ 9.482225][ T72] dwmmc_k3 ff37f000.dwmmc1: Using internal
DMA controller.
[ 9.482232][ T72] dwmmc_k3 ff37f000.dwmmc1: Version ID is 270a
[ 9.482261][ T72] dwmmc_k3 ff37f000.dwmmc1: DW MMC controller
at irq 72,32 bit host data width,128 deep fifo
[ 9.482406][ T117] cpu cpu0: EM: created perf domain
[ 9.482677][ T118] ueventd: Loaded kernel module
/vendor/lib/modules/btqca.ko
[ 9.482745][ T118] ueventd: Loading module
/vendor/lib/modules/hci_uart.ko with args ''
[ 9.483117][ T117] cpu cpu4: EM: created perf domain
[ 9.483767][ T117] ueventd: Loaded kernel module
/vendor/lib/modules/cpufreq-dt.ko
[ 9.484265][ T72] dwmmc_k3 ff37f000.dwmmc1: fifo-depth
property not found, using value of FIFOTH register as default
[ 9.484326][ T117] ueventd: LoadWithAliases was unable to load
cpu:type:aarch64:feature:,0000,0001,0002,0003,0004,0005,0006,0007,000B
[ 9.484335][ T117] ueventd:
[ 9.486508][ T72] dwmmc_k3 ff37f000.dwmmc1: IDMAC supports
64-bit address mode.
[ 9.486564][ T72] dwmmc_k3 ff37f000.dwmmc1: Using internal
DMA controller.
[ 9.486572][ T72] dwmmc_k3 ff37f000.dwmmc1: Version ID is 270a
[ 9.486620][ T72] dwmmc_k3 ff37f000.dwmmc1: DW MMC controller
at irq 72,32 bit host data width,64 deep fifo
[ 9.488281][ T121] Kernel panic - not syncing: BRK handler:
Fatal exception
for the full serial console log, please check here:
http://ix.io/4mLg
Thanks,
Yongqin Liu
On Wed, 2 Nov 2022 at 06:34, Kees Cook <[email protected]> wrote:
>
> Mark the devm_*alloc()-family of allocations with appropriate
> __alloc_size()/__realloc_size() hints so the compiler can attempt to
> reason about buffer lengths from allocations.
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Rasmus Villemoes <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Nishanth Menon <[email protected]>
> Cc: Michael Kelley <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Won Chung <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> Reviewed-by: Greg Kroah-Hartman <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> ---
> This is already in -next, but I'm including it here again to avoid any
> confusion about this series landing (or being tested) via another tree.
> ---
> include/linux/device.h | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 424b55df0272..5e4cd857e74f 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -197,9 +197,9 @@ void devres_remove_group(struct device *dev, void *id);
> int devres_release_group(struct device *dev, void *id);
>
> /* managed devm_k.alloc/kfree for device drivers */
> -void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __malloc;
> +void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __alloc_size(2);
> void *devm_krealloc(struct device *dev, void *ptr, size_t size,
> - gfp_t gfp) __must_check;
> + gfp_t gfp) __must_check __realloc_size(3);
> __printf(3, 0) char *devm_kvasprintf(struct device *dev, gfp_t gfp,
> const char *fmt, va_list ap) __malloc;
> __printf(3, 4) char *devm_kasprintf(struct device *dev, gfp_t gfp,
> @@ -226,7 +226,8 @@ static inline void *devm_kcalloc(struct device *dev,
> void devm_kfree(struct device *dev, const void *p);
> char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
> const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp);
> -void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp);
> +void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp)
> + __realloc_size(3);
>
> unsigned long devm_get_free_pages(struct device *dev,
> gfp_t gfp_mask, unsigned int order);
> --
> 2.34.1
>
--
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/linaro-android
On Tue, Jan 31, 2023 at 11:36 PM Yongqin Liu <[email protected]> wrote:
>
> Hi, Kees
>
> This change causes "Kernel panic - not syncing: BRK handler: Fatal exception"
> for the android-mainline based hikey960 build, with this commit reverted,
> there is no problem for the build to boot to the homescreen.
> Not sure if you have any idea about it and give some suggestions.
>
> Here is part of the kernel panic log:
>
> [ 9.479878][ T122] ueventd: Loading module
> /vendor/lib/modules/spi-pl022.ko with args ''
> [ 9.480276][ T115] apexd-bootstrap: Pre-allocated loop device 29
> [ 9.480517][ T123] ueventd: LoadWithAliases was unable to load
> of:Nhi3660_i2sT(null)Chisilicon,hi3660-i2s-1.0
> [ 9.480632][ T121] Unexpected kernel BRK exception at EL1
> [ 9.480637][ T121] Internal error: BRK handler:
> 00000000f2000001 [#1] PREEMPT SMP
> [ 9.480644][ T121] Modules linked in: cpufreq_dt(E+)
> hisi_thermal(E+) phy_hi3660_usb3(E) btqca(E) hi6421_pmic_core(E)
> btbcm(E) spi_pl022(E) hi3660_mailbox(E) i2c_designware_platform(E)
> mali_kbase(OE) dw_mmc_k3(E) bluetooth(E) dw_mmc_pltfm(E) dw_mmc(E)
> kirin_drm(E) rfkill(E) kirin_dsi(E) i2c_designware_core(E) k3dma(E)
> drm_dma_helper(E) cma_heap(E) system_heap(E)
> [ 9.480688][ T121] CPU: 4 PID: 121 Comm: ueventd Tainted: G
> OE 6.2.0-rc6-mainline-14196-g1d9f94ec75b9 #1
> [ 9.480694][ T121] Hardware name: HiKey960 (DT)
> [ 9.480697][ T121] pstate: 20400005 (nzCv daif +PAN -UAO -TCO
> -DIT -SSBS BTYPE=--)
> [ 9.480703][ T121] pc : hi3660_thermal_probe+0x6c/0x74 [hisi_thermal]
> [ 9.480722][ T121] lr : hi3660_thermal_probe+0x38/0x74 [hisi_thermal]
> [ 9.480733][ T121] sp : ffffffc00aa13700
> [ 9.480735][ T121] x29: ffffffc00aa13700 x28: 0000007ff8ae8531
> x27: 00000000000008c0
> [ 9.480743][ T121] x26: ffffffc00aa2a300 x25: ffffffc00aa2ab40
> x24: 000000000000001d
> [ 9.480749][ T121] x23: ffffffc00a29d000 x22: 0000000000000000
> x21: ffffff8001fa4a80
> [ 9.480755][ T121] x20: 0000000000000001 x19: ffffff8001fa4a80
> x18: ffffffc00a8810b0
> [ 9.480761][ T121] x17: 000000007ab542f2 x16: 000000007ab542f2
> x15: ffffffc00aa01000
> [ 9.480767][ T121] x14: ffffffc00966f250 x13: ffffffc0b58f9000
> x12: ffffffc00a055f10
> [ 9.480771][ T123] ueventd: LoadWithAliases was unable to load
> cpu:type:aarch64:feature:,0000,0001,0002,0003,0004,0005,0006,0007,000B
> [ 9.480773][ T121]
> [ 9.480774][ T121] x11: 0000000000000000 x10: 0000000000000001
> x9 : 0000000100000000
> [ 9.480780][ T123] ueventd:
> [ 9.480780][ T121] x8 : ffffffc0044154cb x7 : 0000000000000000
> x6 : 000000000000003f
> [ 9.480786][ T121] x5 : 0000000000000020 x4 : ffffffc0098db323
> x3 : ffffff801aeb62c0
> [ 9.480792][ T121] x2 : ffffff801aeb62c0 x1 : 0000000000000000
> x0 : ffffff8001fa4c80
> [ 9.480798][ T121] Call trace:
> [ 9.480801][ T121] hi3660_thermal_probe+0x6c/0x74 [hisi_thermal]
> [ 9.480813][ T121] hisi_thermal_probe+0xbc/0x284 [hisi_thermal]
Taking a look here, it looks pretty obvious:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/hisi_thermal.c#n414
data->nr_sensors = 1;
data->sensor = devm_kzalloc(dev, sizeof(*data->sensor) *
data->nr_sensors, GFP_KERNEL);
Here as nr_sensors=1, we allocate only one structure for the array.
But then below that, we modify two entries, writing past the valid
array, and corrupting data when writing the second sensor values.
data->sensor[0].id = HI3660_BIG_SENSOR;
data->sensor[0].irq_name = "tsensor_a73";
data->sensor[0].data = data;
data->sensor[1].id = HI3660_LITTLE_SENSOR;
data->sensor[1].irq_name = "tsensor_a53";
data->sensor[1].data = data;
I suspect nr_sensors needs to be set to 2.
Nice work, Kees!
thanks
-john
On Wed, Feb 1, 2023 at 12:11 AM John Stultz <[email protected]> wrote:
> On Tue, Jan 31, 2023 at 11:36 PM Yongqin Liu <[email protected]> wrote:
> >
> > Hi, Kees
> >
> > This change causes "Kernel panic - not syncing: BRK handler: Fatal exception"
> > for the android-mainline based hikey960 build, with this commit reverted,
> > there is no problem for the build to boot to the homescreen.
> > Not sure if you have any idea about it and give some suggestions.
> >
> > Here is part of the kernel panic log:
> >
> > [ 9.479878][ T122] ueventd: Loading module
> > /vendor/lib/modules/spi-pl022.ko with args ''
> > [ 9.480276][ T115] apexd-bootstrap: Pre-allocated loop device 29
> > [ 9.480517][ T123] ueventd: LoadWithAliases was unable to load
> > of:Nhi3660_i2sT(null)Chisilicon,hi3660-i2s-1.0
> > [ 9.480632][ T121] Unexpected kernel BRK exception at EL1
> > [ 9.480637][ T121] Internal error: BRK handler:
> > 00000000f2000001 [#1] PREEMPT SMP
> > [ 9.480644][ T121] Modules linked in: cpufreq_dt(E+)
> > hisi_thermal(E+) phy_hi3660_usb3(E) btqca(E) hi6421_pmic_core(E)
> > btbcm(E) spi_pl022(E) hi3660_mailbox(E) i2c_designware_platform(E)
> > mali_kbase(OE) dw_mmc_k3(E) bluetooth(E) dw_mmc_pltfm(E) dw_mmc(E)
> > kirin_drm(E) rfkill(E) kirin_dsi(E) i2c_designware_core(E) k3dma(E)
> > drm_dma_helper(E) cma_heap(E) system_heap(E)
> > [ 9.480688][ T121] CPU: 4 PID: 121 Comm: ueventd Tainted: G
> > OE 6.2.0-rc6-mainline-14196-g1d9f94ec75b9 #1
> > [ 9.480694][ T121] Hardware name: HiKey960 (DT)
> > [ 9.480697][ T121] pstate: 20400005 (nzCv daif +PAN -UAO -TCO
> > -DIT -SSBS BTYPE=--)
> > [ 9.480703][ T121] pc : hi3660_thermal_probe+0x6c/0x74 [hisi_thermal]
> > [ 9.480722][ T121] lr : hi3660_thermal_probe+0x38/0x74 [hisi_thermal]
> > [ 9.480733][ T121] sp : ffffffc00aa13700
> > [ 9.480735][ T121] x29: ffffffc00aa13700 x28: 0000007ff8ae8531
> > x27: 00000000000008c0
> > [ 9.480743][ T121] x26: ffffffc00aa2a300 x25: ffffffc00aa2ab40
> > x24: 000000000000001d
> > [ 9.480749][ T121] x23: ffffffc00a29d000 x22: 0000000000000000
> > x21: ffffff8001fa4a80
> > [ 9.480755][ T121] x20: 0000000000000001 x19: ffffff8001fa4a80
> > x18: ffffffc00a8810b0
> > [ 9.480761][ T121] x17: 000000007ab542f2 x16: 000000007ab542f2
> > x15: ffffffc00aa01000
> > [ 9.480767][ T121] x14: ffffffc00966f250 x13: ffffffc0b58f9000
> > x12: ffffffc00a055f10
> > [ 9.480771][ T123] ueventd: LoadWithAliases was unable to load
> > cpu:type:aarch64:feature:,0000,0001,0002,0003,0004,0005,0006,0007,000B
> > [ 9.480773][ T121]
> > [ 9.480774][ T121] x11: 0000000000000000 x10: 0000000000000001
> > x9 : 0000000100000000
> > [ 9.480780][ T123] ueventd:
> > [ 9.480780][ T121] x8 : ffffffc0044154cb x7 : 0000000000000000
> > x6 : 000000000000003f
> > [ 9.480786][ T121] x5 : 0000000000000020 x4 : ffffffc0098db323
> > x3 : ffffff801aeb62c0
> > [ 9.480792][ T121] x2 : ffffff801aeb62c0 x1 : 0000000000000000
> > x0 : ffffff8001fa4c80
> > [ 9.480798][ T121] Call trace:
> > [ 9.480801][ T121] hi3660_thermal_probe+0x6c/0x74 [hisi_thermal]
> > [ 9.480813][ T121] hisi_thermal_probe+0xbc/0x284 [hisi_thermal]
>
>
> Taking a look here, it looks pretty obvious:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/hisi_thermal.c#n414
>
> data->nr_sensors = 1;
> data->sensor = devm_kzalloc(dev, sizeof(*data->sensor) *
> data->nr_sensors, GFP_KERNEL);
>
> Here as nr_sensors=1, we allocate only one structure for the array.
> But then below that, we modify two entries, writing past the valid
> array, and corrupting data when writing the second sensor values.
>
> data->sensor[0].id = HI3660_BIG_SENSOR;
> data->sensor[0].irq_name = "tsensor_a73";
> data->sensor[0].data = data;
>
> data->sensor[1].id = HI3660_LITTLE_SENSOR;
> data->sensor[1].irq_name = "tsensor_a53";
> data->sensor[1].data = data;
>
> I suspect nr_sensors needs to be set to 2.
Looks like the bug was introduced here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d3a2a2bbadb4bf5856ed394ba09b8fbb7a80460
But that change seems to imply the dual zones weren't fully supported
at the time. I'm not sure if that's changed in the meantime, so
removing the second sensor writes may potentially be a better fix.
thanks
-john
On Wed, Feb 01, 2023 at 12:11:41AM -0800, John Stultz wrote:
> On Tue, Jan 31, 2023 at 11:36 PM Yongqin Liu <[email protected]> wrote:
...
> data->nr_sensors = 1;
> data->sensor = devm_kzalloc(dev, sizeof(*data->sensor) *
> data->nr_sensors, GFP_KERNEL);
Side note: This should use devm_kcalloc().
--
With Best Regards,
Andy Shevchenko
On Wed, Feb 01, 2023 at 12:11:41AM -0800, John Stultz wrote:
> On Tue, Jan 31, 2023 at 11:36 PM Yongqin Liu <[email protected]> wrote:
> >
> > Hi, Kees
> >
> > This change causes "Kernel panic - not syncing: BRK handler: Fatal exception"
> > for the android-mainline based hikey960 build, with this commit reverted,
> > there is no problem for the build to boot to the homescreen.
> > Not sure if you have any idea about it and give some suggestions.
> >
> > Here is part of the kernel panic log:
> >
> > [ 9.479878][ T122] ueventd: Loading module
> > /vendor/lib/modules/spi-pl022.ko with args ''
> > [ 9.480276][ T115] apexd-bootstrap: Pre-allocated loop device 29
> > [ 9.480517][ T123] ueventd: LoadWithAliases was unable to load
> > of:Nhi3660_i2sT(null)Chisilicon,hi3660-i2s-1.0
> > [ 9.480632][ T121] Unexpected kernel BRK exception at EL1
> > [ 9.480637][ T121] Internal error: BRK handler:
> > 00000000f2000001 [#1] PREEMPT SMP
> > [ 9.480644][ T121] Modules linked in: cpufreq_dt(E+)
> > hisi_thermal(E+) phy_hi3660_usb3(E) btqca(E) hi6421_pmic_core(E)
> > btbcm(E) spi_pl022(E) hi3660_mailbox(E) i2c_designware_platform(E)
> > mali_kbase(OE) dw_mmc_k3(E) bluetooth(E) dw_mmc_pltfm(E) dw_mmc(E)
> > kirin_drm(E) rfkill(E) kirin_dsi(E) i2c_designware_core(E) k3dma(E)
> > drm_dma_helper(E) cma_heap(E) system_heap(E)
> > [ 9.480688][ T121] CPU: 4 PID: 121 Comm: ueventd Tainted: G
> > OE 6.2.0-rc6-mainline-14196-g1d9f94ec75b9 #1
> > [ 9.480694][ T121] Hardware name: HiKey960 (DT)
> > [ 9.480697][ T121] pstate: 20400005 (nzCv daif +PAN -UAO -TCO
> > -DIT -SSBS BTYPE=--)
> > [ 9.480703][ T121] pc : hi3660_thermal_probe+0x6c/0x74 [hisi_thermal]
> > [ 9.480722][ T121] lr : hi3660_thermal_probe+0x38/0x74 [hisi_thermal]
> > [ 9.480733][ T121] sp : ffffffc00aa13700
> > [ 9.480735][ T121] x29: ffffffc00aa13700 x28: 0000007ff8ae8531
> > x27: 00000000000008c0
> > [ 9.480743][ T121] x26: ffffffc00aa2a300 x25: ffffffc00aa2ab40
> > x24: 000000000000001d
> > [ 9.480749][ T121] x23: ffffffc00a29d000 x22: 0000000000000000
> > x21: ffffff8001fa4a80
> > [ 9.480755][ T121] x20: 0000000000000001 x19: ffffff8001fa4a80
> > x18: ffffffc00a8810b0
> > [ 9.480761][ T121] x17: 000000007ab542f2 x16: 000000007ab542f2
> > x15: ffffffc00aa01000
> > [ 9.480767][ T121] x14: ffffffc00966f250 x13: ffffffc0b58f9000
> > x12: ffffffc00a055f10
> > [ 9.480771][ T123] ueventd: LoadWithAliases was unable to load
> > cpu:type:aarch64:feature:,0000,0001,0002,0003,0004,0005,0006,0007,000B
> > [ 9.480773][ T121]
> > [ 9.480774][ T121] x11: 0000000000000000 x10: 0000000000000001
> > x9 : 0000000100000000
> > [ 9.480780][ T123] ueventd:
> > [ 9.480780][ T121] x8 : ffffffc0044154cb x7 : 0000000000000000
> > x6 : 000000000000003f
> > [ 9.480786][ T121] x5 : 0000000000000020 x4 : ffffffc0098db323
> > x3 : ffffff801aeb62c0
> > [ 9.480792][ T121] x2 : ffffff801aeb62c0 x1 : 0000000000000000
> > x0 : ffffff8001fa4c80
> > [ 9.480798][ T121] Call trace:
> > [ 9.480801][ T121] hi3660_thermal_probe+0x6c/0x74 [hisi_thermal]
> > [ 9.480813][ T121] hisi_thermal_probe+0xbc/0x284 [hisi_thermal]
>
>
> Taking a look here, it looks pretty obvious:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/hisi_thermal.c#n414
>
> data->nr_sensors = 1;
> data->sensor = devm_kzalloc(dev, sizeof(*data->sensor) *
> data->nr_sensors, GFP_KERNEL);
>
> Here as nr_sensors=1, we allocate only one structure for the array.
> But then below that, we modify two entries, writing past the valid
> array, and corrupting data when writing the second sensor values.
>
> data->sensor[0].id = HI3660_BIG_SENSOR;
> data->sensor[0].irq_name = "tsensor_a73";
> data->sensor[0].data = data;
>
> data->sensor[1].id = HI3660_LITTLE_SENSOR;
> data->sensor[1].irq_name = "tsensor_a53";
> data->sensor[1].data = data;
>
> I suspect nr_sensors needs to be set to 2.
>
> Nice work, Kees!
Yay for compilers! :)
Was a patch sent to fix this driver?
--
Kees Cook
On Thu, Feb 2, 2023 at 9:18 AM Kees Cook <[email protected]> wrote:
> On Wed, Feb 01, 2023 at 12:11:41AM -0800, John Stultz wrote:
> > On Tue, Jan 31, 2023 at 11:36 PM Yongqin Liu <[email protected]> wrote:
> > > This change causes "Kernel panic - not syncing: BRK handler: Fatal exception"
> > > for the android-mainline based hikey960 build, with this commit reverted,
> > > there is no problem for the build to boot to the homescreen.
> > > Not sure if you have any idea about it and give some suggestions.
> > >
> > > Here is part of the kernel panic log:
...
> > Here as nr_sensors=1, we allocate only one structure for the array.
> > But then below that, we modify two entries, writing past the valid
> > array, and corrupting data when writing the second sensor values.
> >
> > data->sensor[0].id = HI3660_BIG_SENSOR;
> > data->sensor[0].irq_name = "tsensor_a73";
> > data->sensor[0].data = data;
> >
> > data->sensor[1].id = HI3660_LITTLE_SENSOR;
> > data->sensor[1].irq_name = "tsensor_a53";
> > data->sensor[1].data = data;
> >
> > I suspect nr_sensors needs to be set to 2.
> >
> > Nice work, Kees!
>
> Yay for compilers! :)
Well, I know it's not trivial to make the compilers catch these
things, so yay for you and others putting in all the effort on this as
well.
That said, making sense of the error message isn't completely trivial
either. I've been seeing a few cases recently of some of the new
compiler tooling (I pinged you earlier on a CFI one) causing errors
that developers aren't really sure how to address. I know sometimes
it's not easy to surface the errors with context to what was wrong,
but at the risk of intense bike shedding, is there some way to provide
something like "Likely array bounds error" instead of just "BRK
handler: Fatal exception"?
> Was a patch sent to fix this driver?
I think YongQin is looking into it (either setting the nr_sensors
value to 2 or dropping the second sensor access).
thanks
-john
On Thu, Feb 02, 2023 at 10:56:29AM -0800, John Stultz wrote:
> That said, making sense of the error message isn't completely trivial
> either. I've been seeing a few cases recently of some of the new
> compiler tooling (I pinged you earlier on a CFI one) causing errors
> that developers aren't really sure how to address. I know sometimes
> it's not easy to surface the errors with context to what was wrong,
> but at the risk of intense bike shedding, is there some way to provide
> something like "Likely array bounds error" instead of just "BRK
> handler: Fatal exception"?
Yeah, this is a result of the size trade-off that resulted in config
CONFIG_UBSAN_TRAP -- there ends up being no message about what went
wrong. I'd really like to have cleaner handling of this -- perhaps what
was done for KCFI could be applied to UBSAN as well, though this is an
area I don't know well myself. (i.e. encoding "this was a UBSAN trap"
in the trap itself.)
Sami or Ard, is this something that could be improved for arm64?
--
Kees Cook
On Thu, 2 Feb 2023 at 20:10, Kees Cook <[email protected]> wrote:
>
> On Thu, Feb 02, 2023 at 10:56:29AM -0800, John Stultz wrote:
> > That said, making sense of the error message isn't completely trivial
> > either. I've been seeing a few cases recently of some of the new
> > compiler tooling (I pinged you earlier on a CFI one) causing errors
> > that developers aren't really sure how to address. I know sometimes
> > it's not easy to surface the errors with context to what was wrong,
> > but at the risk of intense bike shedding, is there some way to provide
> > something like "Likely array bounds error" instead of just "BRK
> > handler: Fatal exception"?
>
> Yeah, this is a result of the size trade-off that resulted in config
> CONFIG_UBSAN_TRAP -- there ends up being no message about what went
> wrong. I'd really like to have cleaner handling of this -- perhaps what
> was done for KCFI could be applied to UBSAN as well, though this is an
> area I don't know well myself. (i.e. encoding "this was a UBSAN trap"
> in the trap itself.)
>
> Sami or Ard, is this something that could be improved for arm64?
>
-ENOCONTEXT, so I am going to assume this is about runtime
instrumentation that needs some kind of 'panic' function which it will
invoke if some condition is met that should never occur?
We already use brk with different immediate values in the opcode, so
the arch layer already has what we need. Is this a limitation in the
compiler, perhaps, where it always emits the same brk opcode?
On Thu, Feb 2, 2023 at 11:20 AM Ard Biesheuvel <[email protected]> wrote:
>
> On Thu, 2 Feb 2023 at 20:10, Kees Cook <[email protected]> wrote:
> >
> > On Thu, Feb 02, 2023 at 10:56:29AM -0800, John Stultz wrote:
> > > That said, making sense of the error message isn't completely trivial
> > > either. I've been seeing a few cases recently of some of the new
> > > compiler tooling (I pinged you earlier on a CFI one) causing errors
> > > that developers aren't really sure how to address. I know sometimes
> > > it's not easy to surface the errors with context to what was wrong,
> > > but at the risk of intense bike shedding, is there some way to provide
> > > something like "Likely array bounds error" instead of just "BRK
> > > handler: Fatal exception"?
> >
> > Yeah, this is a result of the size trade-off that resulted in config
> > CONFIG_UBSAN_TRAP -- there ends up being no message about what went
> > wrong. I'd really like to have cleaner handling of this -- perhaps what
> > was done for KCFI could be applied to UBSAN as well, though this is an
> > area I don't know well myself. (i.e. encoding "this was a UBSAN trap"
> > in the trap itself.)
> >
> > Sami or Ard, is this something that could be improved for arm64?
> >
>
> -ENOCONTEXT, so I am going to assume this is about runtime
> instrumentation that needs some kind of 'panic' function which it will
> invoke if some condition is met that should never occur?
>
> We already use brk with different immediate values in the opcode, so
> the arch layer already has what we need. Is this a limitation in the
> compiler, perhaps, where it always emits the same brk opcode?
Yeah, we'd need to update both the compiler to produce the encoding,
and the kernel to recognize the encoding and do something special.
--
Thanks,
~Nick Desaulniers
On Thu, Feb 2, 2023 at 11:31 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Thu, Feb 2, 2023 at 11:20 AM Ard Biesheuvel <[email protected]> wrote:
> >
> > On Thu, 2 Feb 2023 at 20:10, Kees Cook <[email protected]> wrote:
> > >
> > > On Thu, Feb 02, 2023 at 10:56:29AM -0800, John Stultz wrote:
> > > > That said, making sense of the error message isn't completely trivial
> > > > either. I've been seeing a few cases recently of some of the new
> > > > compiler tooling (I pinged you earlier on a CFI one) causing errors
> > > > that developers aren't really sure how to address. I know sometimes
> > > > it's not easy to surface the errors with context to what was wrong,
> > > > but at the risk of intense bike shedding, is there some way to provide
> > > > something like "Likely array bounds error" instead of just "BRK
> > > > handler: Fatal exception"?
> > >
> > > Yeah, this is a result of the size trade-off that resulted in config
> > > CONFIG_UBSAN_TRAP -- there ends up being no message about what went
> > > wrong. I'd really like to have cleaner handling of this -- perhaps what
> > > was done for KCFI could be applied to UBSAN as well, though this is an
> > > area I don't know well myself. (i.e. encoding "this was a UBSAN trap"
> > > in the trap itself.)
> > >
> > > Sami or Ard, is this something that could be improved for arm64?
> > >
> >
> > -ENOCONTEXT, so I am going to assume this is about runtime
> > instrumentation that needs some kind of 'panic' function which it will
> > invoke if some condition is met that should never occur?
> >
> > We already use brk with different immediate values in the opcode, so
> > the arch layer already has what we need. Is this a limitation in the
> > compiler, perhaps, where it always emits the same brk opcode?
>
> Yeah, we'd need to update both the compiler to produce the encoding,
> and the kernel to recognize the encoding and do something special.
A quick look at Clang's source code suggests that Intrinsic::ubsantrap
already accepts the handler ID (from the SanitizerHandler enum) as an
argument and the arm64 LLVM back-end appears to encode the value as an
immediate for the brk instruction. I didn't confirm that this actually
works, but perhaps we just need to teach the kernel about the possible
values?
Sami
On Thu, Feb 02, 2023 at 11:49:42AM -0800, Sami Tolvanen wrote:
> On Thu, Feb 2, 2023 at 11:31 AM Nick Desaulniers
> <[email protected]> wrote:
> >
> > On Thu, Feb 2, 2023 at 11:20 AM Ard Biesheuvel <[email protected]> wrote:
> > >
> > > On Thu, 2 Feb 2023 at 20:10, Kees Cook <[email protected]> wrote:
> > > >
> > > > On Thu, Feb 02, 2023 at 10:56:29AM -0800, John Stultz wrote:
> > > > > That said, making sense of the error message isn't completely trivial
> > > > > either. I've been seeing a few cases recently of some of the new
> > > > > compiler tooling (I pinged you earlier on a CFI one) causing errors
> > > > > that developers aren't really sure how to address. I know sometimes
> > > > > it's not easy to surface the errors with context to what was wrong,
> > > > > but at the risk of intense bike shedding, is there some way to provide
> > > > > something like "Likely array bounds error" instead of just "BRK
> > > > > handler: Fatal exception"?
> > > >
> > > > Yeah, this is a result of the size trade-off that resulted in config
> > > > CONFIG_UBSAN_TRAP -- there ends up being no message about what went
> > > > wrong. I'd really like to have cleaner handling of this -- perhaps what
> > > > was done for KCFI could be applied to UBSAN as well, though this is an
> > > > area I don't know well myself. (i.e. encoding "this was a UBSAN trap"
> > > > in the trap itself.)
> > > >
> > > > Sami or Ard, is this something that could be improved for arm64?
> > > >
> > >
> > > -ENOCONTEXT, so I am going to assume this is about runtime
> > > instrumentation that needs some kind of 'panic' function which it will
> > > invoke if some condition is met that should never occur?
> > >
> > > We already use brk with different immediate values in the opcode, so
> > > the arch layer already has what we need. Is this a limitation in the
> > > compiler, perhaps, where it always emits the same brk opcode?
> >
> > Yeah, we'd need to update both the compiler to produce the encoding,
> > and the kernel to recognize the encoding and do something special.
>
> A quick look at Clang's source code suggests that Intrinsic::ubsantrap
> already accepts the handler ID (from the SanitizerHandler enum) as an
> argument and the arm64 LLVM back-end appears to encode the value as an
> immediate for the brk instruction. I didn't confirm that this actually
> works, but perhaps we just need to teach the kernel about the possible
> values?
Oh excellent. Yeah, if that's all that's needed here that would be
great. What are the values?
--
Kees Cook
On Thu, Feb 2, 2023 at 11:53 AM Kees Cook <[email protected]> wrote:
>
> On Thu, Feb 02, 2023 at 11:49:42AM -0800, Sami Tolvanen wrote:
> > A quick look at Clang's source code suggests that Intrinsic::ubsantrap
> > already accepts the handler ID (from the SanitizerHandler enum) as an
> > argument and the arm64 LLVM back-end appears to encode the value as an
> > immediate for the brk instruction. I didn't confirm that this actually
> > works, but perhaps we just need to teach the kernel about the possible
> > values?
>
> Oh excellent. Yeah, if that's all that's needed here that would be
> great. What are the values?
The arm64 brk immediate encoding seems to be "ubsantrap arg | 'U' << 8":
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AArch64/AArch64InstrInfo.td#L7571
The argument values come from the SanitizerHandler enum, which is
populated from this list:
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.h#L113
Therefore, according to the tests, for ubsantrap(12) we'll get brk
#0x550c, for example:
https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AArch64/ubsantrap.ll
Sami
On Thu, Feb 02, 2023 at 12:11:47PM -0800, Sami Tolvanen wrote:
> On Thu, Feb 2, 2023 at 11:53 AM Kees Cook <[email protected]> wrote:
> >
> > On Thu, Feb 02, 2023 at 11:49:42AM -0800, Sami Tolvanen wrote:
> > > A quick look at Clang's source code suggests that Intrinsic::ubsantrap
> > > already accepts the handler ID (from the SanitizerHandler enum) as an
> > > argument and the arm64 LLVM back-end appears to encode the value as an
> > > immediate for the brk instruction. I didn't confirm that this actually
> > > works, but perhaps we just need to teach the kernel about the possible
> > > values?
> >
> > Oh excellent. Yeah, if that's all that's needed here that would be
> > great. What are the values?
>
> The arm64 brk immediate encoding seems to be "ubsantrap arg | 'U' << 8":
>
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AArch64/AArch64InstrInfo.td#L7571
>
> The argument values come from the SanitizerHandler enum, which is
> populated from this list:
>
> https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.h#L113
>
> Therefore, according to the tests, for ubsantrap(12) we'll get brk
> #0x550c, for example:
>
> https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AArch64/ubsantrap.ll
So the absolute minimal handler would look like this:
diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h
index 6e000113e508..3f0f0d03268b 100644
--- a/arch/arm64/include/asm/brk-imm.h
+++ b/arch/arm64/include/asm/brk-imm.h
@@ -28,6 +28,8 @@
#define BUG_BRK_IMM 0x800
#define KASAN_BRK_IMM 0x900
#define KASAN_BRK_MASK 0x0ff
+#define UBSAN_BRK_IMM 0x5500
+#define UBSAN_BRK_MASK 0x00ff
#define CFI_BRK_IMM_TARGET GENMASK(4, 0)
#define CFI_BRK_IMM_TYPE GENMASK(9, 5)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 4c0caa589e12..36b917d8fa5f 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -1074,6 +1074,18 @@ static struct break_hook kasan_break_hook = {
};
#endif
+#ifdef CONFIG_UBSAN_TRAP
+static int ubsan_handler(struct pt_regs *regs, unsigned long esr)
+{
+ die("Oops - UBSAN", regs, esr);
+}
+
+static struct break_hook ubsan_break_hook = {
+ .fn = ubsan_handler,
+ .imm = UBSAN_BRK_IMM,
+ .mask = UBSAN_BRK_MASK,
+};
+#endif
#define esr_comment(esr) ((esr) & ESR_ELx_BRK64_ISS_COMMENT_MASK)
@@ -1091,6 +1103,10 @@ int __init early_brk64(unsigned long addr, unsigned long esr,
#ifdef CONFIG_KASAN_SW_TAGS
if ((esr_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
+#endif
+#ifdef CONFIG_UBSAN_TRAP
+ if ((esr_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
+ return ubsan_handler(regs, esr) != DBG_HOOK_HANDLED;
#endif
return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
}
@@ -1104,6 +1120,9 @@ void __init trap_init(void)
register_kernel_break_hook(&fault_break_hook);
#ifdef CONFIG_KASAN_SW_TAGS
register_kernel_break_hook(&kasan_break_hook);
+#endif
+#ifdef CONFIG_UBSAN_TRAP
+ register_kernel_break_hook(&ubsan_break_hook);
#endif
debug_traps_init();
}
But we could expand ubsan_handler() to extract the SanitizerHandler enum
value and report which UBSAN check was hit...
--
Kees Cook