Re: [PATCH] uapi: stddef.h: Provide UAPI macros for __counted_by_{le, be}
Hi Alexander,
On Tue, May 07, 2024 at 02:58:15PM +0200, Alexander Lobakin wrote:
> From: Erick Archer
> Date: Mon, 6 May 2024 19:42:08 +0200
>
> > Provide UAPI macros for UAPI structs that will gain annotations for
> > __counted_by_{le, be} attributes.
>
> Pls add me to Cc next time.
Ok.
> Why is this change needed? __counted_by_{le, be}() aren't used anywhere
> in the uAPI headers.
The goal of this commit is to be able to accept this another one [1].
In the "batman-adv" we annotate "struct batadv_tvlv_tt_data" with the
"__counted_by_be()" attribute.
[1]
https://lore.kernel.org/linux-hardening/as8pr02mb72371f89d188b047410b755e8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
Thanks,
Erick
>
> >
> > Signed-off-by: Erick Archer
> > ---
> > include/uapi/linux/stddef.h | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h
> > index 2ec6f35cda32..58154117d9b0 100644
> > --- a/include/uapi/linux/stddef.h
> > +++ b/include/uapi/linux/stddef.h
> > @@ -55,4 +55,12 @@
> > #define __counted_by(m)
> > #endif
> >
> > +#ifndef __counted_by_le
> > +#define __counted_by_le(m)
> > +#endif
> > +
> > +#ifndef __counted_by_be
> > +#define __counted_by_be(m)
> > +#endif
> > +
> > #endif /* _UAPI_LINUX_STDDEF_H */
>
> Thanks,
> Olek
Re: [PATCH v3] scsi: csiostor: Use kcalloc() instead of kzalloc()
Hi Martin, Kees and Finn, On Sat, Mar 30, 2024 at 05:17:53PM +0100, Erick Archer wrote: > Use 2-factor multiplication argument form kcalloc() instead > of kzalloc(). > > Also, 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). > > Link: https://github.com/KSPP/linux/issues/162 > Reviewed-by: Gustavo A. R. Silva > Signed-off-by: Erick Archer > --- > Thank you very much for the reviews and comments. Regards, Erick
[PATCH v3] 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]
Signed-off-by: Erick Archer
---
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).
Previous versions:
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/
---
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_page
Re: [PATCH] perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
Hi everyone,
On Sat, Mar 30, 2024 at 03:32:59PM +0100, Erick Archer wrote:
> 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]
> Signed-off-by: Erick Archer
How could this patch be accepted?
Thanks,
Erick
[PATCH] 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
Signed-off-by: Erick Archer
---
Hi,
This patch can be considered v4 of this other one [1]. However, since
the patch has completely changed due to the addition of the flex array,
I have decided to make a new series and remove the "Reviewed-by:" tag
by Gustavo A. R. Silva and Kees cook.
[1]
https://lore.kernel.org/linux-hardening/paxpr02mb7248f46defa47e79677481b18b...@paxpr02mb7248.eurprd02.prod.outlook.com/
Thanks,
Erick
---
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 4ccb8fa483e6..86755d16a3b2 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 {
@@ -426,10 +426,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;
}
@@ -474,19 +472,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
Re: [PATCH v3] scsi: csiostor: Use kcalloc() instead of kzalloc()
Hi James, On Sat, May 11, 2024 at 01:18:46PM +0200, Erick Archer wrote: > Hi Martin, Kees and Finn, > > On Sat, Mar 30, 2024 at 05:17:53PM +0100, Erick Archer wrote: > > Use 2-factor multiplication argument form kcalloc() instead > > of kzalloc(). > > > > Also, 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). > > > > Link: https://github.com/KSPP/linux/issues/162 > > Reviewed-by: Gustavo A. R. Silva > > Signed-off-by: Erick Archer > > --- > > > Thank you very much for the reviews and comments. Also, thanks for the comments and clarifications. Regards, Erick
Re: [RFC] Mitigating unexpected arithmetic overflow
I'm pretty sure we've tried using a sanitizer before and it had too many
false positives. So your solution is to annotate all the false
positives?
There are two main issue which make integer overflows complicated from
a static analysis perspective. 1) Places where it's intentional or
we don't care. 2) Places where the integer overflow is harmless for
one reason or another. Btw, integer overflows are a lot more common on
32 bit CPUs because obviously it's easier to overflow 4 billion than
whatever number U64_MAX is.
Here are is a sample of ten integer overflows that the user can trigger.
Maybe pick a couple and walk us through how they would be annotated?
drivers/usb/dwc3/gadget.c:3064 dwc3_gadget_vbus_draw() warn: potential integer
overflow from user '1000 * mA'
3052 static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA)
3053 {
3054 struct dwc3 *dwc = gadget_to_dwc(g);
3055 union power_supply_propval val = {0};
3056 int ret;
3057
3058 if (dwc->usb2_phy)
3059 return usb_phy_set_power(dwc->usb2_phy, mA);
3060
3061 if (!dwc->usb_psy)
3062 return -EOPNOTSUPP;
3063
3064 val.intval = 1000 * mA;
^^
mA comes from the user and we only know that it's a multiple of 2.
So here we would annotate that val.intval can store integer overflows?
Or we'd annotate mA?
3065 ret = power_supply_set_property(dwc->usb_psy,
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val);
3066
3067 return ret;
3068 }
drivers/usb/class/usbtmc.c:852 usbtmc_generic_read() warn: potential integer
overflow from user 'max_transfer_size + 1'
842 * wMaxPacketSize – 1) to avoid sending a zero-length
843 * packet
844 */
845 remaining = transfer_size;
846 if ((max_transfer_size % data->wMaxPacketSize) == 0)
847 max_transfer_size += (data->wMaxPacketSize - 1);
848 } else {
849 /* round down to bufsize to avoid truncated data left */
850 if (max_transfer_size > bufsize) {
851 max_transfer_size =
852 roundup(max_transfer_size + 1 - bufsize,
^
This can overflow. We should make it a rule that all size variables
have to be unsigned long. That would have made this safe on 64 bit
systems.
853 bufsize);
854 }
855 remaining = max_transfer_size;
drivers/usb/misc/usbtest.c:1994 iso_alloc_urb() warn: potential integer
overflow from user 'bytes + maxp'
1974 static struct urb *iso_alloc_urb(
1975 struct usb_device *udev,
1976 int pipe,
1977 struct usb_endpoint_descriptor *desc,
1978 longbytes,
1979 unsigned offset
1980 )
1981 {
1982 struct urb *urb;
1983 unsignedi, maxp, packets;
1984
1985 if (bytes < 0 || !desc)
1986 return NULL;
1987
1988 maxp = usb_endpoint_maxp(desc);
1989 if (udev->speed >= USB_SPEED_SUPER)
1990 maxp *= ss_isoc_get_packet_num(udev, pipe);
1991 else
1992 maxp *= usb_endpoint_maxp_mult(desc);
1993
1994 packets = DIV_ROUND_UP(bytes, maxp);
^
The issue here is on a 32 bit system when bytes is INT_MAX.
1995
1996 urb = usb_alloc_urb(packets, GFP_KERNEL);
1997 if (!urb)
1998 return urb;
drivers/usb/storage/uas.c:324 uas_stat_cmplt() warn: potential integer overflow
from user 'idx + 1'
322 idx = be16_to_cpup(&iu->tag) - 1;
The "- 1" makes this UINT_MAX
323 if (idx >= MAX_CMNDS || !devinfo->cmnd[idx]) {
324 dev_err(&urb->dev->dev,
325 "stat urb: no pending cmd for uas-tag %d\n",
idx + 1);
^^^
Harmless integer overflow in printk.
326 goto out;
327 }
drivers/mtd/parsers/tplink_safeloader.c:101
mtd_parser_tplink_safeloader_parse() warn: potential integer overflow from user
'bytes + 1'
97 for (idx = 0, offset = TPLINK_SAFELOADER_DATA_OFFSET;
98 idx < TPLINK_SAFELOADER_MAX_PARTS &&
99 sscanf(buf + offset, "partition %64s base 0x%llx size
0x%llx%zn\n",
100 name, &parts[idx].offset, &parts[idx].size, &bytes)
== 3;
I think this buffer comes from the partition table?
101
[GIT PULL] hardening updates for 6.10-rc1
Hi Linus,
Please pull these hardening updates for 6.10-rc1. The bulk of the changes
here are related to refactoring and expanding the KUnit tests for string
helper and fortify behavior. Some trivial strncpy replacements in fs/
were carried in my tree. Also some fixes to SCSI string handling were
carried in my tree since the helper for those was introduce here. Beyond
that, just little fixes all around: objtool getting confused about
LKDTM+KCFI, preparing for future refactors (constification of sysctl
tables, additional __counted_by annotations), a Clang UBSAN+i386 crash
fix, and adding more options in the hardening.config Kconfig fragment.
Thanks!
-Kees
The following changes since commit 39cd87c4eb2b893354f3b850f916353f2658ae6f:
Linux 6.9-rc2 (2024-03-31 14:32:39 -0700)
are available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git
tags/hardening-6.10-rc1
for you to fetch changes up to 6d305cbef1aa01b9714e01e35f3d5c28544cf04d:
uapi: stddef.h: Provide UAPI macros for __counted_by_{le, be} (2024-05-08
00:42:25 -0700)
hardening updates for 6.10-rc1
- selftests: Add str*cmp tests (Ivan Orlov)
- __counted_by: provide UAPI for _le/_be variants (Erick Archer)
- Various strncpy deprecation refactors (Justin Stitt)
- stackleak: Use a copy of soon-to-be-const sysctl table (Thomas Weißschuh)
- UBSAN: Work around i386 -regparm=3 bug with Clang prior to version 19
- Provide helper to deal with non-NUL-terminated string copying
- SCSI: Fix older string copying bugs (with new helper)
- selftests: Consolidate string helper behavioral tests
- selftests: add memcpy() fortify tests
- string: Add additional __realloc_size() annotations for "dup" helpers
- LKDTM: Fix KCFI+rodata+objtool confusion
- hardening.config: Enable KCFI
Erick Archer (1):
uapi: stddef.h: Provide UAPI macros for __counted_by_{le, be}
Ivan Orlov (1):
string_kunit: Add test cases for str*cmp functions
Justin Stitt (5):
virt: acrn: replace deprecated strncpy with strscpy
reiserfs: replace deprecated strncpy with scnprintf
hfsplus: refactor copy_name to not use strncpy
fs: ecryptfs: replace deprecated strncpy with strscpy
init: replace deprecated strncpy with strscpy_pad
Kees Cook (21):
string: Prepare to merge strscpy_kunit.c into string_kunit.c
string: Merge strscpy KUnit tests into string_kunit.c
string: Prepare to merge strcat KUnit tests into string_kunit.c
string: Merge strcat KUnit tests into string_kunit.c
string: Convert KUnit test names to standard convention
string.h: Introduce memtostr() and memtostr_pad()
string_kunit: Move strtomem KUnit test to string_kunit.c
MAINTAINERS: Add ubsan.h to the UBSAN section
ubsan: Remove 1-element array usage in debug reporting
ubsan: Avoid i386 UBSAN handler crashes with Clang
scsi: mptfusion: Avoid possible run-time warning with long manufacturer
strings
scsi: mpi3mr: Avoid possible run-time warning with long manufacturer
strings
scsi: qla2xxx: Avoid possible run-time warning with long model_num
kunit/fortify: Fix mismatched kvalloc()/vfree() usage
kunit/fortify: Rename tests to use recommended conventions
kunit/fortify: Do not spam logs with fortify WARNs
kunit/fortify: Add memcpy() tests
lkdtm: Disable CFI checking for perms functions
hardening: Enable KCFI and some other options
kunit/fortify: Fix replaced failure path to unbreak __alloc_size
string: Add additional __realloc_size() annotations for "dup" helpers
Thomas Weißschuh (1):
stackleak: Use a copy of the ctl_table argument
MAINTAINERS| 3 +-
arch/arm64/configs/hardening.config| 1 +
arch/x86/configs/hardening.config | 3 +
drivers/message/fusion/mptsas.c| 14 +-
drivers/misc/lkdtm/Makefile| 2 +-
drivers/misc/lkdtm/perms.c | 2 +-
drivers/scsi/mpi3mr/mpi3mr_transport.c | 14 +-
drivers/scsi/qla2xxx/qla_mr.c | 6 +-
drivers/virt/acrn/ioreq.c | 2 +-
fs/ecryptfs/crypto.c | 4 +-
fs/ecryptfs/main.c | 26 +-
fs/hfsplus/xattr.c | 22 +-
fs/reiserfs/item_ops.c | 13 +-
include/linux/fortify-string.h | 9 +-
include/linux/string.h | 62 -
include/uapi/linux/stddef.h| 8 +
init/do_mounts.c | 3 +-
kernel/configs/hardening.config| 8 +
kernel/stackleak.c | 6 +-
lib/Kconfig.debug | 10 -
lib/Makefile | 2 -
lib/fortify_kunit.c| 222
lib/memcpy_kunit.c | 53
lib/strcat_kunit.c
Re: [PATCH] perf/x86/amd/uncore: Add flex array to struct amd_uncore_ctx
On Sat, May 11, 2024 at 04:51:54PM +0200, Erick Archer wrote:
> 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
> Signed-off-by: Erick Archer
> ---
> Hi,
>
> This patch can be considered v4 of this other one [1]. However, since
> the patch has completely changed due to the addition of the flex array,
> I have decided to make a new series and remove the "Reviewed-by:" tag
> by Gustavo A. R. Silva and Kees cook.
>
> [1]
> https://lore.kernel.org/linux-hardening/paxpr02mb7248f46defa47e79677481b18b...@paxpr02mb7248.eurprd02.prod.outlook.com/
>
> Thanks,
> Erick
> ---
> arch/x86/events/amd/uncore.c | 18 +-
> 1 file changed, 5 insertions(+), 13 deletions(-)
My favorite kind of patch: fewer lines, clearer code.
Reviewed-by: Kees Cook
-Kees
--
Kees Cook
