| Message ID | AS8PR02MB7237569E4FBE0B26F62FDFDB8B1D2@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] perf/ring_buffer: Prefer struct_size over open coded arithmetic | expand |
Le 05/05/2024 à 16:15, Erick Archer a écrit : > 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 assignment before the > array-writing loop while in the other case the assignment needs to be > added. > > 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] > Signed-off-by: Erick Archer <erick.archer@outlook.com> > --- > Changes in v2: > - Annotate "struct perf_buffer" with __counted_by() attribute (Kees Cook). > - Refactor the logic to gain __counted_by() coverage (Kees Cook). > > Previous versions: > v1 -> https://lore.kernel.org/linux-hardening/AS8PR02MB72372AB065EA8340D960CCC48B1B2@AS8PR02MB7237.eurprd02.prod.outlook.com/ > > Hi Peter, > > I know that you detest the struct_size() helper, however, as Kees > explained in v1, this change improves the robustness of the code. > Also, we will gain __counted_by() coverage. > > I hope this patch can be applied this time. > > Regards, > Erick > --- > kernel/events/internal.h | 2 +- > kernel/events/ring_buffer.c | 14 ++++---------- > 2 files changed, 5 insertions(+), 11 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..080537eff69f 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; Hi, Should size be size_t? > 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,18 +913,15 @@ 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; > > + rb->nr_pages = nr_pages; I don't think this is correct. There is already a logic in place about it a few lines below: all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE); if (!all_buf) goto fail_all_buf; rb->user_page = all_buf; rb->data_pages[0] = all_buf + PAGE_SIZE; if (nr_pages) { <--- here rb->nr_pages = 1; <--- rb->page_order = ilog2(nr_pages); } I think that what is needed is to move this block just 2 lines above, (before rb->data_pages[0] = ...) I'm also wondering what should be done if nr_pages = 0. CJ > INIT_WORK(&rb->work, rb_free_work); > > all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
On Sun, May 05, 2024 at 05:24:55PM +0200, Christophe JAILLET wrote: > Le 05/05/2024 à 16:15, Erick Archer a écrit : > > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c > > index 4013408ce012..080537eff69f 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; > > Hi, > > Should size be size_t? I'm sorry, but I don't have enough knowledge to answer this question. The "size" variable is used as a return value by struct_size and as a parameter to the order_base_2() and kzalloc_node() functions. The size type for the kzalloc_node function is "size_t" but for the order_base_2() macro it is necessary an unsigned type (since this is expanded to "__ilog2_u32(u32 n)" or "__ilog2_u64(u64 n)"). So, I don't know if it is correct to change the type to size_t. Maybe someone can help with this. > > > 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,18 +913,15 @@ 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; > > + rb->nr_pages = nr_pages; > > I don't think this is correct. I think you are right. My bad :( > There is already a logic in place about it a few lines below: > > all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE); > if (!all_buf) > goto fail_all_buf; > > rb->user_page = all_buf; > rb->data_pages[0] = all_buf + PAGE_SIZE; > if (nr_pages) { <--- here > rb->nr_pages = 1; <--- > rb->page_order = ilog2(nr_pages); > } > > I think that what is needed is to move this block just 2 lines above, > (before rb->data_pages[0] = ...) > > > I'm also wondering what should be done if nr_pages = 0. Perhaps this is enough since we only allocate memory for one member of the array. @@ -916,18 +913,15 @@ 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; + rb->nr_pages = 1; INIT_WORK(&rb->work, rb_free_work); all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE); I think that we don't need to deal with the "nr_pages = 0" case since the flex array will always have a length of one. Kees, can you help us with this? Regards, Erick > CJ > > > INIT_WORK(&rb->work, rb_free_work); > > all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE); >
On Sun, May 05, 2024 at 07:31:24PM +0200, Erick Archer wrote: > On Sun, May 05, 2024 at 05:24:55PM +0200, Christophe JAILLET wrote: > > Le 05/05/2024 à 16:15, Erick Archer a écrit : > > > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c > > > index 4013408ce012..080537eff69f 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; > > > > Hi, > > > > Should size be size_t? > > I'm sorry, but I don't have enough knowledge to answer this question. > The "size" variable is used as a return value by struct_size and as > a parameter to the order_base_2() and kzalloc_node() functions. For Linux, size_t and unsigned long are the same (currently). Pedantically, yes, this should be size_t, but it's the same. > [...] > > all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE); > > if (!all_buf) > > goto fail_all_buf; > > > > rb->user_page = all_buf; > > rb->data_pages[0] = all_buf + PAGE_SIZE; > > if (nr_pages) { <--- here > > rb->nr_pages = 1; <--- > > rb->page_order = ilog2(nr_pages); > > } > [...] > I think that we don't need to deal with the "nr_pages = 0" case > since the flex array will always have a length of one. > > Kees, can you help us with this? Agh, this code hurt my head for a while. all_buf contains "nr_pages + 1" pages. all_buf gets attached to rb->user_page, and then rb->data_pages[0] points to the second page in all_buf... which means, I guess, that rb->data_pages does only have 1 entry. However, the nr_pages == 0 case is weird. Currently, data_pages[0] will still get set (which points ... off the end of all_buf). If we unconditionally set rb->nr_pages to 1, we're changing the behavior. If we _don't_ set rb->data_pages[0], we're changing the behavior, but I think it's an invalid pointer anyway, so this is the safer change to make. I suspect the right replacement is: diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c index 4013408ce012..7d638ce76799 100644 --- a/kernel/events/ring_buffer.c +++ b/kernel/events/ring_buffer.c @@ -916,15 +916,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, nr_pages, 1), GFP_KERNEL, node); if (!rb) goto fail; @@ -935,9 +931,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); } Also, why does rb_alloc() take an "int" nr_pages? The only caller has an unsigned long argument for nr_pages. Nothing checks for >INT_MAX that I can find.
Le 06/05/2024 à 18:23, Kees Cook a écrit : > On Sun, May 05, 2024 at 07:31:24PM +0200, Erick Archer wrote: >> On Sun, May 05, 2024 at 05:24:55PM +0200, Christophe JAILLET wrote: >>> Le 05/05/2024 à 16:15, Erick Archer a écrit : >>>> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c >>>> index 4013408ce012..080537eff69f 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; >>> >>> Hi, >>> >>> Should size be size_t? >> >> I'm sorry, but I don't have enough knowledge to answer this question. >> The "size" variable is used as a return value by struct_size and as >> a parameter to the order_base_2() and kzalloc_node() functions. > > For Linux, size_t and unsigned long are the same (currently). > Pedantically, yes, this should be size_t, but it's the same. > >> [...] >>> all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE); >>> if (!all_buf) >>> goto fail_all_buf; >>> >>> rb->user_page = all_buf; >>> rb->data_pages[0] = all_buf + PAGE_SIZE; >>> if (nr_pages) { <--- here >>> rb->nr_pages = 1; <--- >>> rb->page_order = ilog2(nr_pages); >>> } >> [...] >> I think that we don't need to deal with the "nr_pages = 0" case >> since the flex array will always have a length of one. >> >> Kees, can you help us with this? > > Agh, this code hurt my head for a while. > > all_buf contains "nr_pages + 1" pages. all_buf gets attached to > rb->user_page, and then rb->data_pages[0] points to the second page in > all_buf... which means, I guess, that rb->data_pages does only have 1 > entry. > > However, the nr_pages == 0 case is weird. Currently, data_pages[0] will > still get set (which points ... off the end of all_buf). If we > unconditionally set rb->nr_pages to 1, we're changing the behavior. If > we _don't_ set rb->data_pages[0], we're changing the behavior, but I > think it's an invalid pointer anyway, so this is the safer change to > make. I suspect the right replacement is: > > > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c > index 4013408ce012..7d638ce76799 100644 > --- a/kernel/events/ring_buffer.c > +++ b/kernel/events/ring_buffer.c > @@ -916,15 +916,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, nr_pages, 1), GFP_KERNEL, node); > if (!rb) > goto fail; > > @@ -935,9 +931,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); > } This is also what make the most sense to me. CJ > > > > Also, why does rb_alloc() take an "int" nr_pages? The only caller has an > unsigned long argument for nr_pages. Nothing checks for >INT_MAX that I > can find. >
Hi Kees and Christophe, First of all, thanks for the reviews, comments and advices. On Mon, May 06, 2024 at 09:23:15AM -0700, Kees Cook wrote: > On Sun, May 05, 2024 at 07:31:24PM +0200, Erick Archer wrote: > > On Sun, May 05, 2024 at 05:24:55PM +0200, Christophe JAILLET wrote: > > > Le 05/05/2024 à 16:15, Erick Archer a écrit : > > > > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c > > > > index 4013408ce012..080537eff69f 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; > > > > > > Hi, > > > > > > Should size be size_t? > > > > I'm sorry, but I don't have enough knowledge to answer this question. > > The "size" variable is used as a return value by struct_size and as > > a parameter to the order_base_2() and kzalloc_node() functions. > > For Linux, size_t and unsigned long are the same (currently). > Pedantically, yes, this should be size_t, but it's the same. Thanks for this clarification. I will change the type for the next version. > > > [...] > > > all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE); > > > if (!all_buf) > > > goto fail_all_buf; > > > > > > rb->user_page = all_buf; > > > rb->data_pages[0] = all_buf + PAGE_SIZE; > > > if (nr_pages) { <--- here > > > rb->nr_pages = 1; <--- > > > rb->page_order = ilog2(nr_pages); > > > } > > [...] > > I think that we don't need to deal with the "nr_pages = 0" case > > since the flex array will always have a length of one. > > > > Kees, can you help us with this? > > Agh, this code hurt my head for a while. > > all_buf contains "nr_pages + 1" pages. all_buf gets attached to > rb->user_page, and then rb->data_pages[0] points to the second page in > all_buf... which means, I guess, that rb->data_pages does only have 1 > entry. > > However, the nr_pages == 0 case is weird. Currently, data_pages[0] will > still get set (which points ... off the end of all_buf). If we > unconditionally set rb->nr_pages to 1, we're changing the behavior. If > we _don't_ set rb->data_pages[0], we're changing the behavior, but I > think it's an invalid pointer anyway, so this is the safer change to > make. Thanks for explain things well. > I suspect the right replacement is: > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c > index 4013408ce012..7d638ce76799 100644 > --- a/kernel/events/ring_buffer.c > +++ b/kernel/events/ring_buffer.c > @@ -916,15 +916,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, nr_pages, 1), GFP_KERNEL, node); > if (!rb) > goto fail; > > @@ -935,9 +931,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); > } > Ok, I'll do it like this for the next version. > > > Also, why does rb_alloc() take an "int" nr_pages? The only caller has an > unsigned long argument for nr_pages. Nothing checks for >INT_MAX that I > can find. Thanks for letting me know. I will take a look. > > -- Again, thanks, Erick > Kees Cook
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..080537eff69f 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,18 +913,15 @@ 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; + rb->nr_pages = nr_pages; INIT_WORK(&rb->work, rb_free_work); all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
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 assignment before the array-writing loop while in the other case the assignment needs to be added. 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] Signed-off-by: Erick Archer <erick.archer@outlook.com> --- Changes in v2: - Annotate "struct perf_buffer" with __counted_by() attribute (Kees Cook). - Refactor the logic to gain __counted_by() coverage (Kees Cook). Previous versions: v1 -> https://lore.kernel.org/linux-hardening/AS8PR02MB72372AB065EA8340D960CCC48B1B2@AS8PR02MB7237.eurprd02.prod.outlook.com/ Hi Peter, I know that you detest the struct_size() helper, however, as Kees explained in v1, this change improves the robustness of the code. Also, we will gain __counted_by() coverage. I hope this patch can be applied this time. Regards, Erick --- kernel/events/internal.h | 2 +- kernel/events/ring_buffer.c | 14 ++++---------- 2 files changed, 5 insertions(+), 11 deletions(-)