Re: [PATCH] x86/boot: add prototype for __fortify_panic()
On 31.05.24 г. 19:28 ч., Kees Cook wrote:
On Thu, May 30, 2024 at 09:23:36AM -0700, Jeff Johnson wrote:
On 5/30/2024 8:42 AM, Nikolay Borisov wrote:
On 29.05.24 г. 21:09 ч., Jeff Johnson wrote:
As discussed in [1] add a prototype for __fortify_panic() to fix the
'make W=1 C=1' warning:
arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was
not declared. Should it be static?
Actually doesn't it make sense to have this defined under ../string.h ?
Actually given that we don't have any string fortification under the
boot/ why have the fortify _* functions at all ?
I'll let Kees answer these questions since I just took guidance from him :)
Ah-ha, I see what's happening. When not built with
CONFIG_FORTIFY_SOURCE, fortify-string.h isn't included. But since misc.c
has the function definition, we get a warning that the function
declaration was never seen. This is likely the better solution:
fortify-strings.h is used in include/linux/string.h. However all the
files in the decompressor are using a local copy of string.h and not the
kernel-wide. When pre-processing misc.c with FORTIFY_SOURCE enabled
here's the status:
$ grep -i fortify arch/x86/boot/compressed/misc.i
void __fortify_panic(const u8 reason, size_t avail, size_t size)
It seems the decompressor is not using fortify-string at all because
it's not using the global string.h ?
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index b70e4a21c15f..3f21a5e218f8 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -532,7 +532,9 @@ asmlinkage __visible void *extract_kernel(void *rmode,
unsigned char *output)
return output + entry_offset;
}
+#ifdef CONFIG_FORTIFY_SOURCE
void __fortify_panic(const u8 reason, size_t avail, size_t size)
{
error("detected buffer overflow");
}
+#endif
Jeff, can you test this? (I still haven't been able to reproduce the
warning.)
Re: [PATCH v2] wifi: brcm80211: use sizeof(*pointer) instead of sizeof(type)
Erick Archer wrote: > It is preferred to use sizeof(*pointer) instead of sizeof(type) > due to the type of the variable can change and one needs not > change the former (unlike the latter). This patch has no effect > on runtime behavior. > > At the same time remove some redundant NULL initializations. > > Acked-by: Arend van Spriel > Signed-off-by: Erick Archer Patch applied to wireless-next.git, thanks. dcb77f854ae0 wifi: brcm80211: use sizeof(*pointer) instead of sizeof(type) -- https://patchwork.kernel.org/project/linux-wireless/patch/as8pr02mb7237ff1c2e880d1231684d708b...@as8pr02mb7237.eurprd02.prod.outlook.com/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] sctp: annotate struct sctp_assoc_ids with __counted_by()
Hi,
On Wed, May 01, 2024 at 07:01:22PM +0200, Erick Archer wrote:
> Prepare for the coming implementation by GCC and Clang of the
> __counted_by attribute. Flexible array members annotated with
> __counted_by can have their accesses bounds-checked at run-time via
> CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> (for strcpy/memcpy-family functions).
>
> Suggested-by: Kees Cook
> Signed-off-by: Erick Archer
> ---
> include/uapi/linux/sctp.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index b7d91d4cf0db..836173e73401 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -1007,7 +1007,7 @@ enum sctp_sstat_state {
> */
> struct sctp_assoc_ids {
> __u32 gaids_number_of_ids;
> - sctp_assoc_tgaids_assoc_id[];
> + sctp_assoc_tgaids_assoc_id[] __counted_by(gaids_number_of_ids);
> };
>
> /*
Friendly ping: who can take this, please?
Regards,
Erick
Re: [PATCH v2] x86/traps: Enable UBSAN traps on x86
On Sat, Jun 01, 2024 at 03:10:05AM +, Gatlin Newhouse wrote:
> +void handle_ubsan_failure(struct pt_regs *regs, int insn)
> +{
> + u32 type = 0;
> +
> + if (insn == INSN_ASOP) {
> + type = (*(u16 *)(regs->ip + LEN_ASOP + LEN_UD1));
> + if ((type & 0xFF) == 0x40)
> + type = (type >> 8) & 0xFF;
> + } else {
> + type = (*(u16 *)(regs->ip + LEN_UD1));
> + if ((type & 0xFF) == 0x40)
> + type = (type >> 8) & 0xFF;
> + }
The if/else code is repeated, but the only difference is the offset to
read from. Also, if the 0x40 is absent, we likely don't want to report
anything. So, perhaps:
u16 offset = LEN_UD1;
u32 type;
if (insn == INSN_ASOP)
offset += INSN_ASOP;
type = *(u16 *)(regs->ip + offset);
if ((type & 0xFF) != 0x40)
return;
type = (type >> 8) & 0xFF;
pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void
*)regs->ip);
--
Kees Cook
[PATCH v4 0/3] Hardening perf subsystem
Hi everyone,
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
In the first patch, the "struct amd_uncore_ctx" can be refactored to
use a flex array for the "events" member. This way, the allocation/
freeing of the memory can be simplified. Then, the struct_size()
helper can be used to do the arithmetic calculation for the memory
to be allocated.
In the second patch, as the "struct intel_uncore_box" ends in a
flexible array, the preferred way in the kernel is to use the
struct_size() helper to do the arithmetic instead of the calculation
"size + count * size" in the kzalloc_node() function.
In the third patch, as the "struct perf_buffer" also ends in a
flexible array, the preferred way in the kernel is to use the
struct_size() helper to do the arithmetic instead of the calculation
"size + count * size" in the kzalloc_node() functions. At the same
time, prepare for the coming implementation by GCC and Clang of the
__counted_by attribute.
This time, I have decided to send these three patches in the same serie
since all of them has been rejected by the maintainers. I have used
the v4 tag since it is the latest iteration in one of the patches.
The reason these patches were rejected is that Peter Zijlstra detest
the struct_size() helper [3][4]. However, Kees Cook and I consider that
the use of this helper improves readability. But we can all say that it
is a matter of preference.
Anyway, leaving aside personal preferences, these patches has the
following pros:
- Code robustness improvement (__counted_by coverage).
- Code robustness improvement (use of struct_size() to do calculations
on memory allocator functions).
- Fewer lines of code.
- Follow the recommendations made in "Deprecated Interfaces, Language
Features, Attributes, and Conventions" [5] as the preferred method
of doing things in the kernel.
- Widely used in the kernel.
- Widely accepted in the kernel.
There are also patches in this subsystem that use the struct_size()
helper:
6566f907bf31 ("Convert intel uncore to struct_size") by Matthew Wilcox
dfbc411e0a5e ("perf/x86/rapl: Prefer struct_size() over open coded arithmetic")
by me
Therefore, I would like these patches to be applied this time.
Best regards,
Erick
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Link:
https://lore.kernel.org/linux-hardening/20240430091833.gd40...@noisy.programming.kicks-ass.net
[3]
Link:
https://lore.kernel.org/linux-hardening/20240430091504.gc40...@noisy.programming.kicks-ass.net
[4]
Link: https://docs.kernel.org/process/deprecated.html [5]
---
Changes in v4:
- Add the "Reviewed-by:" tag to the three patches.
- Rebase against linux next (tag next-20240531).
Previous versions:
perf/x86/amd/uncore: Add flex array to struct amd_uncore_ctx
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237e4848b44a5226bd3551c8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237f4d39bf6aa0ff40e91638b...@as8pr02mb7237.eurprd02.prod.outlook.com/
perf/ring_buffer: Prefer struct_size over open coded arithmetic
v3 ->
https://lore.kernel.org/linux-hardening/as8pr02mb72379b4807f3951a1b926ba58b...@as8pr02mb7237.eurprd02.prod.outlook.com/
v2 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237569e4fbe0b26f62fdfdb8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb72372ab065ea8340d960ccc48b...@as8pr02mb7237.eurprd02.prod.outlook.com/
Changes in v3:
- Refactor the logic, compared to the previous version, of the second
rb_alloc() function to gain __counted_by() coverage (Kees Cook and
Christophe JAILLET).
Changes in v2:
- Annotate "struct perf_buffer" with __counted_by() attribute (Kees Cook).
- Refactor the logic to gain __counted_by() coverage (Kees Cook).
---
Erick Archer (3):
perf/x86/amd/uncore: Add flex array to struct amd_uncore_ctx
perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
perf/ring_buffer: Prefer struct_size over open coded arithmetic
arch/x86/events/amd/uncore.c | 18 +-
arch/x86/events/intel/uncore.c | 7 +++
kernel/events/internal.h | 2 +-
kernel/events/ring_buffer.c| 15 ---
4 files changed, 13 insertions(+), 29 deletions(-)
--
2.25.1
[PATCH v4 1/3] perf/x86/amd/uncore: Add flex array to struct amd_uncore_ctx
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
The "struct amd_uncore_ctx" can be refactored to use a flex array for
the "events" member. This way, the allocation/freeing of the memory can
be simplified.
Specifically, as the "curr" variable is a pointer to the amd_uncore_ctx
structure and it now ends up in a flexible array:
struct amd_uncore_ctx {
[...]
struct perf_event *events[];
};
the two-step allocation can be simplifief by using just one kzalloc_node
function and the struct_size() helper to do the arithmetic calculation
for the memory to be allocated.
This way, the code is more readable and safer.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Suggested-by: Christophe JAILLET
Reviewed-by: Kees Cook
Signed-off-by: Erick Archer
---
arch/x86/events/amd/uncore.c | 18 +-
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 0fafe233bba4..9d43048bfdab 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -37,8 +37,8 @@ static int pmu_version;
struct amd_uncore_ctx {
int refcnt;
int cpu;
- struct perf_event **events;
struct hlist_node node;
+ struct perf_event *events[];
};
struct amd_uncore_pmu {
@@ -430,10 +430,8 @@ static void amd_uncore_ctx_free(struct amd_uncore *uncore,
unsigned int cpu)
if (cpu == ctx->cpu)
cpumask_clear_cpu(cpu, &pmu->active_mask);
- if (!--ctx->refcnt) {
- kfree(ctx->events);
+ if (!--ctx->refcnt)
kfree(ctx);
- }
*per_cpu_ptr(pmu->ctx, cpu) = NULL;
}
@@ -478,19 +476,13 @@ static int amd_uncore_ctx_init(struct amd_uncore *uncore,
unsigned int cpu)
/* Allocate context if sibling does not exist */
if (!curr) {
node = cpu_to_node(cpu);
- curr = kzalloc_node(sizeof(*curr), GFP_KERNEL, node);
+ curr = kzalloc_node(struct_size(curr, events,
+ pmu->num_counters),
+ GFP_KERNEL, node);
if (!curr)
goto fail;
curr->cpu = cpu;
- curr->events = kzalloc_node(sizeof(*curr->events) *
- pmu->num_counters,
- GFP_KERNEL, node);
- if (!curr->events) {
- kfree(curr);
- goto fail;
- }
-
cpumask_set_cpu(cpu, &pmu->active_mask);
}
--
2.25.1
[PATCH v4 2/3] perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "box" variable is a pointer to "struct intel_uncore_box" and
this structure ends in a flexible array:
struct intel_uncore_box {
[...]
struct intel_uncore_extra_reg shared_regs[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc_node() function.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Reviewed-by: Kees Cook
Signed-off-by: Erick Archer
---
arch/x86/events/intel/uncore.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 419c517b8594..b0d518d752aa 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -350,12 +350,11 @@ static void uncore_pmu_init_hrtimer(struct
intel_uncore_box *box)
static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type
*type,
int node)
{
- int i, size, numshared = type->num_shared_regs ;
+ int i, numshared = type->num_shared_regs;
struct intel_uncore_box *box;
- size = sizeof(*box) + numshared * sizeof(struct intel_uncore_extra_reg);
-
- box = kzalloc_node(size, GFP_KERNEL, node);
+ box = kzalloc_node(struct_size(box, shared_regs, numshared), GFP_KERNEL,
+ node);
if (!box)
return NULL;
--
2.25.1
[PATCH v4 3/3] perf/ring_buffer: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "rb" variable is a pointer to "struct perf_buffer" and this
structure ends in a flexible array:
struct perf_buffer {
[...]
void*data_pages[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc_node() functions.
In the "rb_alloc" function defined in the else branch of the macro
#ifndef CONFIG_PERF_USE_VMALLOC
the count in the struct_size helper is the literal "1" since only one
pointer to void is allocated. Also, remove the "size" variable as it
is no longer needed.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions). In this case, it is important to note
that the logic needs a little refactoring to ensure that the "nr_pages"
member is initialized before the first access to the flex array.
In one case, it is only necessary to move the "nr_pages" assignment
before the array-writing loop while in the other case the access to the
flex array needs to be moved inside the if branch and after the
"nr_pages" assignment.
This way, the code is more safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Reviewed-by: Kees Cook
Signed-off-by: Erick Archer
---
kernel/events/internal.h| 2 +-
kernel/events/ring_buffer.c | 15 ---
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/kernel/events/internal.h b/kernel/events/internal.h
index 5150d5f84c03..dc8d39b01adb 100644
--- a/kernel/events/internal.h
+++ b/kernel/events/internal.h
@@ -55,7 +55,7 @@ struct perf_buffer {
void*aux_priv;
struct perf_event_mmap_page *user_page;
- void*data_pages[];
+ void*data_pages[] __counted_by(nr_pages);
};
extern void rb_free(struct perf_buffer *rb);
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 4013408ce012..d123fa2096cf 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
unsigned long size;
int i, node;
- size = sizeof(struct perf_buffer);
- size += nr_pages * sizeof(void *);
-
+ size = struct_size(rb, data_pages, nr_pages);
if (order_base_2(size) > PAGE_SHIFT+MAX_PAGE_ORDER)
goto fail;
@@ -833,6 +831,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
if (!rb)
goto fail;
+ rb->nr_pages = nr_pages;
rb->user_page = perf_mmap_alloc_page(cpu);
if (!rb->user_page)
goto fail_user_page;
@@ -843,8 +842,6 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
goto fail_data_pages;
}
- rb->nr_pages = nr_pages;
-
ring_buffer_init(rb, watermark, flags);
return rb;
@@ -916,15 +913,11 @@ void rb_free(struct perf_buffer *rb)
struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
{
struct perf_buffer *rb;
- unsigned long size;
void *all_buf;
int node;
- size = sizeof(struct perf_buffer);
- size += sizeof(void *);
-
node = (cpu == -1) ? cpu : cpu_to_node(cpu);
- rb = kzalloc_node(size, GFP_KERNEL, node);
+ rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node);
if (!rb)
goto fail;
@@ -935,9 +928,9 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
goto fail_all_buf;
rb->user_page = all_buf;
- rb->data_pages[0] = all_buf + PAGE_SIZE;
if (nr_pages) {
rb->nr_pages = 1;
+ rb->data_pages[0] = all_buf + PAGE_SIZE;
rb->page_order = ilog2(nr_pages);
}
--
2.25.1
Re: [PATCH] kunit/fortify: Remove __kmalloc_node() test
On 5/31/24 8:57 PM, Kees Cook wrote: > __kmalloc_node() is considered an "internal" function to the Slab, so > drop it from explicit testing. So is __kmalloc() and so I have the removal of both as part of the cleanup here: https://lore.kernel.org/all/20240527090127.21979-2-vba...@suse.cz/ which reminds me I should put it to -next at this point. Review still welcome :) > Signed-off-by: Kees Cook > --- > Cc: Vlastimil Babka > Cc: linux...@kvack.org > Cc: linux-hardening@vger.kernel.org > --- > lib/fortify_kunit.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c > index 39da5b3bc649..f9cc467334ce 100644 > --- a/lib/fortify_kunit.c > +++ b/lib/fortify_kunit.c > @@ -235,9 +235,6 @@ static void > fortify_test_alloc_size_##allocator##_dynamic(struct kunit *test) \ > 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);\
