Re: [PATCH] mm/util: Swap kmemdup_array() arguments
On Thu, Jun 06, 2024 at 08:48:37PM +0300, Andy Shevchenko wrote: > On Thu, Jun 6, 2024 at 8:46 PM Kees Cook wrote: > > > > On Thu, Jun 06, 2024 at 08:35:13PM +0300, Andy Shevchenko wrote: > > > On Thu, Jun 6, 2024 at 6:56 PM Kees Cook wrote: > > > > On Thu, 06 Jun 2024 15:46:09 +0100, Jean-Philippe Brucker wrote: > > > > > > [...] > > > > > > > Applied to for-next/hardening, thanks! > > > > > > Btw, is it possible to get this for v6.10, so we may start enabling it > > > for others? > > > > Which others do you mean? > > There are a lot of users of kmemdup(x*y) which I want to convert > sooner than later to kmemdup_array(x,y). Ah-ha, I see what you mean. Well, I'm not sure we can do v6.10 for this because rc2 is behind us, and that's what most subsystems merge to. I can land the patch for rc3 so there will be no warnings in Linus's tree/-next, but conversions in subsystem trees will gain warnings, I think... -- Kees Cook
Re: [PATCH] mm/util: Swap kmemdup_array() arguments
On Thu, Jun 06, 2024 at 08:35:13PM +0300, Andy Shevchenko wrote: > On Thu, Jun 6, 2024 at 6:56 PM Kees Cook wrote: > > On Thu, 06 Jun 2024 15:46:09 +0100, Jean-Philippe Brucker wrote: > > [...] > > > Applied to for-next/hardening, thanks! > > Btw, is it possible to get this for v6.10, so we may start enabling it > for others? Which others do you mean? -- Kees Cook
Re: [PATCH v2] pstore/ram: Replace of_node_put with __free() for automatic cleanup
On Wed, Jun 05, 2024 at 09:49:44PM +, Abhinav Jain wrote:
> Add __free(device_node) to the parent_node struct declaration
> Add changes to incorporate the review comments from v1
>
> Signed-off-by: Abhinav Jain
>
> PATCH v1 link:
> https://lore.kernel.org/all/20240415161409.8375-1-jain.abhinav...@gmail.com/
>
> Changes since v1:
> - Moved the variable definition back to the top of the function body
The history and version links should be below the "---" line.
But more importantly, please base your patch on the upstream tree,
rather than on your v1 patch. :) (i.e. squash your v1 and v2 together).
-Kees
> ---
> fs/pstore/ram.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 14f2f4864e48..f8258e4567c3 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -644,6 +644,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> struct ramoops_platform_data *pdata)
> {
> struct device_node *of_node = pdev->dev.of_node;
> + struct device_node *parent_node __free(device_node) =
> of_node_parent(of_node);
> struct resource *res;
> u32 value;
> int ret;
> @@ -703,7 +704,6 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>* we're not a child of "reserved-memory" and mimicking the
>* expected behavior.
>*/
> - struct device_node *parent_node __free(device_node) =
> of_node_parent(of_node);
> if (!of_node_name_eq(parent_node, "reserved-memory") &&
> !pdata->console_size && !pdata->ftrace_size &&
> !pdata->pmsg_size && !pdata->ecc_info.ecc_size) {
> --
> 2.34.1
>
--
Kees Cook
Re: [PATCH] mm/util: Swap kmemdup_array() arguments
On Thu, 06 Jun 2024 15:46:09 +0100, Jean-Philippe Brucker wrote: > GCC 14.1 complains about the argument usage of kmemdup_array(): > > drivers/soc/tegra/fuse/fuse-tegra.c:130:65: error: 'kmemdup_array' sizes > specified with 'sizeof' in the earlier argument and not in the later argument > [-Werror=calloc-transposed-args] > 130 | fuse->lookups = kmemdup_array(fuse->soc->lookups, > sizeof(*fuse->lookups), > | ^ > drivers/soc/tegra/fuse/fuse-tegra.c:130:65: note: earlier argument should > specify number of elements, later size of each element > > [...] Applied to for-next/hardening, thanks! [1/1] mm/util: Swap kmemdup_array() arguments https://git.kernel.org/kees/c/0ee14725471c Take care, -- Kees Cook
Re: [PATCH v4 4/6] mm/slab: Introduce kmem_buckets_create() and family
On Tue, Jun 04, 2024 at 04:13:32PM -0600, Tycho Andersen wrote:
> On Tue, Jun 04, 2024 at 04:02:28PM +0100, Simon Horman wrote:
> > On Fri, May 31, 2024 at 12:14:56PM -0700, Kees Cook wrote:
> > > + for (idx = 0; idx < ARRAY_SIZE(kmalloc_caches[KMALLOC_NORMAL]); idx++) {
> > > + char *short_size, *cache_name;
> > > + unsigned int cache_useroffset, cache_usersize;
> > > + unsigned int size;
> > > +
> > > + if (!kmalloc_caches[KMALLOC_NORMAL][idx])
> > > + continue;
> > > +
> > > + size = kmalloc_caches[KMALLOC_NORMAL][idx]->object_size;
> > > + if (!size)
> > > + continue;
> > > +
> > > + short_size = strchr(kmalloc_caches[KMALLOC_NORMAL][idx]->name,
> > > '-');
> > > + if (WARN_ON(!short_size))
> > > + goto fail;
> > > +
> > > + cache_name = kasprintf(GFP_KERNEL, "%s-%s", name, short_size +
> > > 1);
> > > + if (WARN_ON(!cache_name))
> > > + goto fail;
> > > +
> > > + if (useroffset >= size) {
> > > + cache_useroffset = 0;
> > > + cache_usersize = 0;
> > > + } else {
> > > + cache_useroffset = useroffset;
> > > + cache_usersize = min(size - cache_useroffset, usersize);
> > > + }
> > > + (*b)[idx] = kmem_cache_create_usercopy(cache_name, size,
> > > + align, flags, cache_useroffset,
> > > + cache_usersize, ctor);
> > > + kfree(cache_name);
> > > + if (WARN_ON(!(*b)[idx]))
> > > + goto fail;
> > > + }
> > > +
> > > + return b;
> > > +
> > > +fail:
> > > + for (idx = 0; idx < ARRAY_SIZE(kmalloc_caches[KMALLOC_NORMAL]); idx++) {
> > > + if ((*b)[idx])
> > > + kmem_cache_destroy((*b)[idx]);
> >
> > nit: I don't think it is necessary to guard this with a check for NULL.
>
> Isn't it? What if a kasprintf() fails halfway through the loop?
He means that kmem_cache_destroy() already checks for NULL. Quite right!
void kmem_cache_destroy(struct kmem_cache *s)
{
int err = -EBUSY;
bool rcu_set;
if (unlikely(!s) || !kasan_check_byte(s))
return;
--
Kees Cook
Re: [PATCH] HID: usbhid: fix recurrent out-of-bounds bug in usbhid_parse()
On Tue, Jun 04, 2024 at 10:09:43AM -0700, Nikita Zhandarovich wrote:
> Hi,
>
> On 6/4/24 07:15, Jiri Kosina wrote:
> > On Tue, 4 Jun 2024, Kees Cook wrote:
> >
> >> This isn't the right solution. The problem is that hid_class_descriptor
> >> is a flexible array but was sized as a single element fake flexible
> >> array:
> >>
> >> struct hid_descriptor {
> >> __u8 bLength;
> >> __u8 bDescriptorType;
> >> __le16 bcdHID;
> >> __u8 bCountryCode;
> >> __u8 bNumDescriptors;
> >>
> >> struct hid_class_descriptor desc[1];
> >> } __attribute__ ((packed));
> >>
> >> This likely needs to be:
> >>
> >> struct hid_class_descriptor desc[] __counted_by(bNumDescriptors);
> >>
> >> And then check for any sizeof() uses of the struct that might have changed.
> >
> > Ah, you are of course right, not sure what I was thinking. Thanks a lot
> > for catching my brainfart.
> >
> > I am dropping the patch for now; Nikita, will you please send a refreshed
> > one?
> >
>
> Thanks for catching my mistake.
>
> I'll gladly send a revised version, hoping to do it very soon.
I spent a little more time looking at this, and I'm not sure I
understand where the actual space for the descriptors comes from?
There's interface->extra that is being parsed, and effectively
hid_descriptor is being mapped into it, but it uses "sizeof(struct
hid_descriptor)" for the limit. Is more than 1 descriptor expected to
work correctly? Or is the limit being ignored? I'm a bit confused by
this code...
--
Kees Cook
Re: [PATCH] HID: usbhid: fix recurrent out-of-bounds bug in usbhid_parse()
On June 4, 2024 1:15:35 AM PDT, Jiri Kosina wrote:
>On Fri, 24 May 2024, Nikita Zhandarovich wrote:
>
>> Syzbot reports [1] a reemerging out-of-bounds bug regarding hid
>> descriptors possibly having incorrect bNumDescriptors values in
>> usbhid_parse().
>>
>> Build on the previous fix in "HID: usbhid: fix out-of-bounds bug"
>> and run a sanity-check ensuring that number of descriptors doesn't
>> exceed the size of desc[] in struct hid_descriptor.
>>
>> [1] Syzbot report:
>> Link: https://syzkaller.appspot.com/bug?extid=c52569baf0c843f35495
>>
>> UBSAN: array-index-out-of-bounds in drivers/hid/usbhid/hid-core.c:1024:7
>> index 1 is out of range for type 'struct hid_class_descriptor[1]'
>> CPU: 0 PID: 8 Comm: kworker/0:1 Not tainted
>> 6.9.0-rc6-syzkaller-00290-gb9158815de52 #0
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
>> Google 03/27/2024
>> Workqueue: usb_hub_wq hub_event
>> Call Trace:
>>
>> __dump_stack lib/dump_stack.c:88 [inline]
>> dump_stack_lvl+0x241/0x360 lib/dump_stack.c:114
>> ubsan_epilogue lib/ubsan.c:231 [inline]
>> __ubsan_handle_out_of_bounds+0x121/0x150 lib/ubsan.c:429
>> usbhid_parse+0x5a7/0xc80 drivers/hid/usbhid/hid-core.c:1024
>> hid_add_device+0x132/0x520 drivers/hid/hid-core.c:2790
>> usbhid_probe+0xb38/0xea0 drivers/hid/usbhid/hid-core.c:1429
>> usb_probe_interface+0x645/0xbb0 drivers/usb/core/driver.c:399
>> really_probe+0x2b8/0xad0 drivers/base/dd.c:656
>> __driver_probe_device+0x1a2/0x390 drivers/base/dd.c:798
>> driver_probe_device+0x50/0x430 drivers/base/dd.c:828
>> __device_attach_driver+0x2d6/0x530 drivers/base/dd.c:956
>> bus_for_each_drv+0x24e/0x2e0 drivers/base/bus.c:457
>> __device_attach+0x333/0x520 drivers/base/dd.c:1028
>> bus_probe_device+0x189/0x260 drivers/base/bus.c:532
>> device_add+0x8ff/0xca0 drivers/base/core.c:3720
>> usb_set_configuration+0x1976/0x1fb0 drivers/usb/core/message.c:2210
>> usb_generic_driver_probe+0x88/0x140 drivers/usb/core/generic.c:254
>> usb_probe_device+0x1b8/0x380 drivers/usb/core/driver.c:294
>>
>> Reported-and-tested-by: syzbot+c52569baf0c843f35...@syzkaller.appspotmail.com
>> Fixes: f043bfc98c19 ("HID: usbhid: fix out-of-bounds bug")
>> Signed-off-by: Nikita Zhandarovich
>
>Applied, thanks.
This isn't the right solution. The problem is that hid_class_descriptor is a
flexible array but was sized as a single element fake flexible array:
struct hid_descriptor {
__u8 bLength;
__u8 bDescriptorType;
__le16 bcdHID;
__u8 bCountryCode;
__u8 bNumDescriptors;
struct hid_class_descriptor desc[1];
} __attribute__ ((packed));
This likely needs to be:
struct hid_class_descriptor desc[] __counted_by(bNumDescriptors);
And then check for any sizeof() uses of the struct that might have changed.
--
Kees Cook
Re: [PATCH 1/2] mm/memblock: Add "reserve_mem" to reserved named memory at boot up
On June 3, 2024 4:33:31 PM PDT, Steven Rostedt wrote:
>From: "Steven Rostedt (Google)"
>
>In order to allow for requesting a memory region that can be used for
>things like pstore on multiple machines where the memory layout is not the
>same, add a new option to the kernel command line called "reserve_mem".
>
>The format is: reserve_mem=nn:align:name
>
>Where it will find nn amount of memory at the given alignment of align.
>The name field is to allow another subsystem to retrieve where the memory
>was found. For example:
>
> reserve_mem=12M:4096:oops ramoops.mem_name=oops
How does this interact with KASLR? It has chosen its physical location before
this parsing happens, so I'd expect this to fail once in a while, unless the
size/alignment is lucky enough that KASLR never uses that portion of the
physical memory...
-Kees
>
>Where ramoops.mem_name will tell ramoops that memory was reserved for it
>via the reserve_mem option and it can find it by calling:
>
> if (reserve_mem_find_by_name("oops", , )) {
> // start holds the start address and size holds the size given
>
>Link: https://lore.kernel.org/all/zjjvnzux3nzig...@kernel.org/
>
>Suggested-by: Mike Rapoport
>Signed-off-by: Steven Rostedt (Google)
>---
> include/linux/mm.h | 2 +
> mm/memblock.c | 97 ++
> 2 files changed, 99 insertions(+)
>
>diff --git a/include/linux/mm.h b/include/linux/mm.h
>index 9849dfda44d4..b4455cc02f2c 100644
>--- a/include/linux/mm.h
>+++ b/include/linux/mm.h
>@@ -4263,4 +4263,6 @@ static inline bool pfn_is_unaccepted_memory(unsigned
>long pfn)
> void vma_pgtable_walk_begin(struct vm_area_struct *vma);
> void vma_pgtable_walk_end(struct vm_area_struct *vma);
>
>+int reserve_mem_find_by_name(const char *name, unsigned long *start, unsigned
>long *size);
>+
> #endif /* _LINUX_MM_H */
>diff --git a/mm/memblock.c b/mm/memblock.c
>index d09136e040d3..a8bf0ee9e2b4 100644
>--- a/mm/memblock.c
>+++ b/mm/memblock.c
>@@ -2244,6 +2244,103 @@ void __init memblock_free_all(void)
> totalram_pages_add(pages);
> }
>
>+/* Keep a table to reserve named memory */
>+#define RESERVE_MEM_MAX_ENTRIES 8
>+#define RESERVE_MEM_NAME_SIZE 16
>+struct reserve_mem_table {
>+ charname[RESERVE_MEM_NAME_SIZE];
>+ unsigned long start;
>+ unsigned long size;
>+};
>+static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
>+static int reserved_mem_count;
>+
>+/* Add wildcard region with a lookup name */
>+static int __init reserved_mem_add(unsigned long start, unsigned long size,
>+ const char *name)
>+{
>+ struct reserve_mem_table *map;
>+
>+ if (!name || !name[0] || strlen(name) >= RESERVE_MEM_NAME_SIZE)
>+ return -EINVAL;
>+
>+ if (reserved_mem_count >= RESERVE_MEM_MAX_ENTRIES)
>+ return -1;
>+
>+ map = _mem_table[reserved_mem_count++];
>+ map->start = start;
>+ map->size = size;
>+ strscpy(map->name, name);
>+ return 0;
>+}
>+
>+/**
>+ * reserve_mem_find_by_name - Find reserved memory region with a given name
>+ * @name: The name that is attached to a reserved memory region
>+ * @start: If found, holds the start address
>+ * @size: If found, holds the size of the address.
>+ *
>+ * Returns: 1 if found or 0 if not found.
>+ */
>+int reserve_mem_find_by_name(const char *name, unsigned long *start, unsigned
>long *size)
>+{
>+ struct reserve_mem_table *map;
>+ int i;
>+
>+ for (i = 0; i < reserved_mem_count; i++) {
>+ map = _mem_table[i];
>+ if (!map->size)
>+ continue;
>+ if (strcmp(name, map->name) == 0) {
>+ *start = map->start;
>+ *size = map->size;
>+ return 1;
>+ }
>+ }
>+ return 0;
>+}
>+
>+/*
>+ * Parse early_reserve_mem=nn:align:name
>+ */
>+static int __init reserve_mem(char *p)
>+{
>+ phys_addr_t start, size, align;
>+ char *oldp;
>+ int err;
>+
>+ if (!p)
>+ return -EINVAL;
>+
>+ oldp = p;
>+ size = memparse(p, );
>+ if (p == oldp)
>+ return -EINVAL;
>+
>+ if (*p != ':')
>+ return -EINVAL;
>+
>+ align = memparse(p+1, );
>+ if (*p != ':')
>+ return -EINVAL;
>+
>+ start = memblock_phys_alloc(size, align);
>+ if (!start)
>+ return -ENOMEM;
>+
>+ p++;
>+ err = reserved_mem_add(start, size, p);
>+ if (err) {
>+ memblock_phys_free(start, size);
>+ return err;
>+ }
>+
>+ p += strlen(p);
>+
>+ return *p == '\0' ? 0: -EINVAL;
>+}
>+__setup("reserve_mem=", reserve_mem);
>+
> #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK)
> static const char * const flagname[] = {
> [ilog2(MEMBLOCK_HOTPLUG)] = "HOTPLUG",
--
Kees Cook
Re: [PATCH v4 2/6] mm/slab: Plumb kmem_buckets into __do_kmalloc_node()
On Mon, Jun 03, 2024 at 07:06:15PM +0200, Vlastimil Babka wrote:
> On 5/31/24 9:14 PM, Kees Cook wrote:
> > Introduce CONFIG_SLAB_BUCKETS which provides the infrastructure to
> > support separated kmalloc buckets (in the follow kmem_buckets_create()
> > patches and future codetag-based separation). Since this will provide
> > a mitigation for a very common case of exploits, enable it by default.
>
> Are you sure? I thought there was a policy that nobody is special enough
> to have stuff enabled by default. Is it worth risking Linus shouting? :)
I think it's important to have this enabled given how common the
exploitation methodology is and how cheap this solution is. Regardless,
if you want it "default n", I can change it.
> I found this too verbose and tried a different approach, in the end rewrote
> everything to verify the idea works. So I'll just link to the result in git:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/linux.git/log/?h=slab-buckets-v4-rewrite
>
> It's also rebased on slab.git:slab/for-6.11/cleanups with some alloc_hooks()
> cleanups that would cause conflicts otherwkse.
>
> But the crux of that approach is:
>
> /*
> * These macros allow declaring a kmem_buckets * parameter alongside size,
> which
> * can be compiled out with CONFIG_SLAB_BUCKETS=n so that a large number of
> call
> * sites don't have to pass NULL.
> */
> #ifdef CONFIG_SLAB_BUCKETS
> #define DECL_BUCKET_PARAMS(_size, _b) size_t (_size), kmem_buckets *(_b)
> #define PASS_BUCKET_PARAMS(_size, _b) (_size), (_b)
> #define PASS_BUCKET_PARAM(_b) (_b)
> #else
> #define DECL_BUCKET_PARAMS(_size, _b) size_t (_size)
> #define PASS_BUCKET_PARAMS(_size, _b) (_size)
> #define PASS_BUCKET_PARAM(_b) NULL
> #endif
>
> Then we have declaration e.g.
>
> void *__kmalloc_node_noprof(DECL_BUCKET_PARAMS(size, b), gfp_t flags, int
> node)
> __assume_kmalloc_alignment __alloc_size(1);
>
> and the function is called like (from code not using buckets)
> return __kmalloc_node_noprof(PASS_BUCKET_PARAMS(size, NULL), flags, node);
>
> or (from code using buckets)
> #define kmem_buckets_alloc(_b, _size, _flags) \
> alloc_hooks(__kmalloc_node_noprof(PASS_BUCKET_PARAMS(_size, _b),
> _flags, NUMA_NO_NODE))
>
> And implementation looks like:
>
> void *__kmalloc_node_noprof(DECL_BUCKET_PARAMS(size, b), gfp_t flags, int
> node)
> {
> return __do_kmalloc_node(size, PASS_BUCKET_PARAM(b), flags, node,
> _RET_IP_);
> }
>
> The size param is always the first, so the __alloc_size(1) doesn't need
> tweaking.
> size is also used in the macros even if it's never mangled, because it's easy
> to pass one param instead of two, but not zero params instead of one, if we
> want
> the ending comma not be part of the macro (which would look awkward).
>
> Does it look ok to you? Of course names of the macros could be tweaked.
> Anyway feel
> free to use the branch for the followup. Hopefully this way is also
> compatible with
> the planned codetag based followup.
This looks really nice, thank you! This is well aligned with the codetag
followup, which also needs to have "size" be very easy to find (to the
macros can check for compile-time-constant or not).
I will go work from your branch...
Thanks!
-Kees
--
Kees Cook
Re: [GIT PULL] bcachefs updates fro 6.10-rc1
On Sat, Jun 01, 2024 at 12:33:31PM +0100, Mark Brown wrote: > On Mon, May 20, 2024 at 12:10:31PM -0400, James Bottomley wrote: > > On Sun, 2024-05-19 at 23:52 -0400, Kent Overstreet wrote: > > > > I also do (try to) post patches to the list that are doing something > > > interesting and worth discussion; the vast majority this cycle has > > > been boring syzbot crap... > > > you still don't say what problem not posting most patches solves? You > > imply it would slow you down, but getting git-send-email to post to a > > mailing list can actually be automated through a pre-push commit hook > > with no slowdown in the awesome rate at which you apply patches to your > > own tree. > > > Linux kernel process exists because it's been found to work over time. > > That's not to say it can't be changed, but it usually requires at least > > some stab at a reason before that happens. > > Even if no meaningful review ever happens on the actual posts there's > still utility in having the patches on a list and findable in lore, > since everything is normally on the list people end up with workflows > that assume that they'll be able to find things there. For example it's > common for test people who identify which patch introduces an issue to > grab the patch from lore in order to review any discussion of the patch, > then report by replying to the patch to help with context for their > report and get some help with figuring out a CC list. Posting costs > very little and makes people's lives easier. Exactly. This is the standard workflow that everyone depends on. So, for example, for my -next trees, I only ever add patches to them via "b4 am lore-url-here...". If I've got patches to add to -next from some devel tree, I don't cherry-pick them to my -next tree: I send them to lore, and then pull them back down. But the point is: send your stuff to lore. :) -- Kees Cook
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
Re: [PATCH] x86/boot: add prototype for __fortify_panic()
On Fri, May 31, 2024 at 11:20:09PM +0200, Borislav Petkov wrote: > So I get an allergic reaction everytime we wag the dog - i.e., fix the > code because some tool or option can't handle it even if it is > a perfectly fine code. In that case it is an unused symbol. > > And frankly, I'd prefer the silly warning to denote that fortify doesn't > need to do any checking there vs shutting it up just because. If we want to declare that x86 boot will never perform string handling on strings with unknown lengths, we could just delete the boot/ implementation of __fortify_panic(), and make it a hard failure if such cases are introduced in the future. This hasn't been a particularly friendly solution in the past, though, as the fortify routines do tend to grow additional coverage over time, so there may be future cases that do trip the runtime checking... -- Kees Cook
Re: [PATCH] x86/boot: add prototype for __fortify_panic()
On Fri, May 31, 2024 at 10:49:47PM +0200, Borislav Petkov wrote:
> On Fri, May 31, 2024 at 01:46:37PM -0700, Kees Cook wrote:
> > Please do not do this. It still benefits from compile-time sanity
> > checking.
>
> Care to elaborate how exactly it benefits?
Because when new code gets added that accidentally does improper string
handling, fortify will yell about it at compile time. e.g, if someone
typos something like:
#define BUF_LEN_FOO 16
...
#define BUF_LEN_BAR 10
struct foo {
...
char buf[BUF_LEN_FOO];
...
};
...
void process_stuff(struct foo *p)
{
...
char local_copy[BUF_LEN_BAR];
...
strcpy(local_copy, p->buf);
...
}
or refactors and forgets to change some name, etc. It's all for catching
bugs before they happen, etc. And when source string lengths aren't
known, the runtime checking can kick in too. It happens x86 boot doesn't
have any of those (good!) so __fortify_panic() goes unused there. But
that's a larger topic covered by stuff like
CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, etc.
--
Kees Cook
Re: [PATCH v3 2/6] mm/slab: Plumb kmem_buckets into __do_kmalloc_node()
On Fri, May 31, 2024 at 12:51:29PM -0400, Kent Overstreet wrote: > On Fri, May 31, 2024 at 09:48:49AM -0700, Kees Cook wrote: > > On Fri, May 24, 2024 at 11:01:40AM -0400, Kent Overstreet wrote: > > > On Wed, Apr 24, 2024 at 02:40:59PM -0700, Kees Cook wrote: > > > > To be able to choose which buckets to allocate from, make the buckets > > > > available to the lower level kmalloc interfaces by adding them as the > > > > first argument. Where the bucket is not available, pass NULL, which > > > > means > > > > "use the default system kmalloc bucket set" (the prior existing > > > > behavior), > > > > as implemented in kmalloc_slab(). > > > > > > I thought the plan was to use codetags for this? That would obviate the > > > need for all this plumbing. > > > > > > Add fields to the alloc tag for: > > > - allocation size (or 0 if it's not a compile time constant) > > > - union of kmem_cache, kmem_buckets, depending on whether the > > >allocation size is constant or not > > > > I want to provide "simple" (low-hanging fruit) coverage that can live > > separately from the codetags-based coverage. The memory overhead for > > this patch series is negligible, but I suspect the codetags expansion, > > while not giant, will be more than some deployments will want. I want > > to avoid an all-or-nothing solution -- which is why I had intended this > > to be available "by default". > > technically there's no reason for your thing to depend on > CONFIG_CODETAGGING at all, that's the infrastructure for finding > codetags for e.g. /proc/allocinfo. you'd just be using the alloc_hoos() > macro and struct alloc_tag as a place to stash the kmem_buckets pointer. It's the overhead of separate kmem_cache and kmem_buckets for every allocation location that I meant. So I'd like the "simple" version for gaining coverage over the currently-being-regularly-exploited cases, and then allow for the "big hammer" solution too. However, I do think I'll still need the codetag infra because of the sections, etc. I think we'll need to pre-build the caches, but maybe that could be avoided by adding some kind of per-site READ_ONCE/lock thingy to create them on demand. We'll see! :) -- Kees Cook
Re: [PATCH] x86/boot: add prototype for __fortify_panic()
On Fri, May 31, 2024 at 09:08:16PM +0200, Borislav Petkov wrote: > On Fri, May 31, 2024 at 09:53:28AM -0700, Kees Cook wrote: > > Under CONFIG_FORTIFY_SOURCE, the boot code *does* still uses > > fortify-string.h. It lets us both catch mistakes we can discover at > > compile and will catch egregious runtime mistakes, though the reporting > > is much simpler in the boot code. > > From where I'm standing, we're not catching anything in the > decompressor: > > $ objdump -D arch/x86/boot/compressed/vmlinux | grep __fortify_panic > 01bec250 <__fortify_panic>: > $ > > Sure, in vmlinux proper (allmodconfig) we do: > > objdump -D vmlinux | grep __fortify_panic | wc -l > 1417 > > but not in the decompressor which is special anyway. > > So we can just as well disable CONFIG_FORTIFY_SOURCE in the decompressor > and not do silly prototypes. Please do not do this. It still benefits from compile-time sanity checking. -- Kees Cook
[PATCH v4 5/6] ipc, msg: Use dedicated slab buckets for alloc_msg()
The msg subsystem is a common target for exploiting[1][2][3][4][5][6][7]
use-after-free type confusion flaws in the kernel for both read and write
primitives. Avoid having a user-controlled dynamically-size allocation
share the global kmalloc cache by using a separate set of kmalloc buckets
via the kmem_buckets API.
Link:
https://blog.hacktivesecurity.com/index.php/2022/06/13/linux-kernel-exploit-development-1day-case-study/
[1]
Link: https://hardenedvault.net/blog/2022-11-13-msg_msg-recon-mitigation-ved/
[2]
Link:
https://www.willsroot.io/2021/08/corctf-2021-fire-of-salvation-writeup.html [3]
Link: https://a13xp0p0v.github.io/2021/02/09/CVE-2021-26708.html [4]
Link:
https://google.github.io/security-research/pocs/linux/cve-2021-22555/writeup.html
[5]
Link: https://zplin.me/papers/ELOISE.pdf [6]
Link: https://syst3mfailure.io/wall-of-perdition/ [7]
Signed-off-by: Kees Cook
---
Cc: "GONG, Ruiqi"
Cc: Xiu Jianfeng
Cc: Suren Baghdasaryan
Cc: Kent Overstreet
Cc: Jann Horn
Cc: Matteo Rizzo
Cc: jvoisin
---
ipc/msgutil.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index d0a0e877cadd..f392f30a057a 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -42,6 +42,17 @@ struct msg_msgseg {
#define DATALEN_MSG((size_t)PAGE_SIZE-sizeof(struct msg_msg))
#define DATALEN_SEG((size_t)PAGE_SIZE-sizeof(struct msg_msgseg))
+static kmem_buckets *msg_buckets __ro_after_init;
+
+static int __init init_msg_buckets(void)
+{
+ msg_buckets = kmem_buckets_create("msg_msg", 0, SLAB_ACCOUNT,
+ sizeof(struct msg_msg),
+ DATALEN_MSG, NULL);
+
+ return 0;
+}
+subsys_initcall(init_msg_buckets);
static struct msg_msg *alloc_msg(size_t len)
{
@@ -50,7 +61,7 @@ static struct msg_msg *alloc_msg(size_t len)
size_t alen;
alen = min(len, DATALEN_MSG);
- msg = kmalloc(sizeof(*msg) + alen, GFP_KERNEL_ACCOUNT);
+ msg = kmem_buckets_alloc(msg_buckets, sizeof(*msg) + alen, GFP_KERNEL);
if (msg == NULL)
return NULL;
--
2.34.1
[PATCH v4 6/6] mm/util: Use dedicated slab buckets for memdup_user()
Both memdup_user() and vmemdup_user() handle allocations that are
regularly used for exploiting use-after-free type confusion flaws in
the kernel (e.g. prctl() PR_SET_VMA_ANON_NAME[1] and setxattr[2][3][4]
respectively).
Since both are designed for contents coming from userspace, it allows
for userspace-controlled allocation sizes. Use a dedicated set of kmalloc
buckets so these allocations do not share caches with the global kmalloc
buckets.
After a fresh boot under Ubuntu 23.10, we can see the caches are already
in active use:
# grep ^memdup /proc/slabinfo
memdup_user-8k 4 4 819248 : ...
memdup_user-4k 8 8 409688 : ...
memdup_user-2k16 16 2048 168 : ...
memdup_user-1k 0 0 1024 164 : ...
memdup_user-5120 0512 162 : ...
memdup_user-2560 0256 161 : ...
memdup_user-1280 0128 321 : ...
memdup_user-64 256256 64 641 : ...
memdup_user-32 512512 32 1281 : ...
memdup_user-16 1024 1024 16 2561 : ...
memdup_user-8 2048 2048 8 5121 : ...
memdup_user-1920 0192 211 : ...
memdup_user-96 168168 96 421 : ...
Link:
https://starlabs.sg/blog/2023/07-prctl-anon_vma_name-an-amusing-heap-spray/ [1]
Link: https://duasynt.com/blog/linux-kernel-heap-spray [2]
Link: https://etenal.me/archives/1336 [3]
Link:
https://github.com/a13xp0p0v/kernel-hack-drill/blob/master/drill_exploit_uaf.c
[4]
Signed-off-by: Kees Cook
---
Cc: "GONG, Ruiqi"
Cc: Xiu Jianfeng
Cc: Suren Baghdasaryan
Cc: Kent Overstreet
Cc: Jann Horn
Cc: Matteo Rizzo
Cc: jvoisin
Cc: linux...@kvack.org
---
mm/util.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/mm/util.c b/mm/util.c
index 53f7fc5912bd..f30460c82641 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -198,6 +198,16 @@ char *kmemdup_nul(const char *s, size_t len, gfp_t gfp)
}
EXPORT_SYMBOL(kmemdup_nul);
+static kmem_buckets *user_buckets __ro_after_init;
+
+static int __init init_user_buckets(void)
+{
+ user_buckets = kmem_buckets_create("memdup_user", 0, 0, 0, INT_MAX,
NULL);
+
+ return 0;
+}
+subsys_initcall(init_user_buckets);
+
/**
* memdup_user - duplicate memory region from user space
*
@@ -211,7 +221,7 @@ void *memdup_user(const void __user *src, size_t len)
{
void *p;
- p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN);
+ p = kmem_buckets_alloc_track_caller(user_buckets, len, GFP_USER |
__GFP_NOWARN);
if (!p)
return ERR_PTR(-ENOMEM);
@@ -237,7 +247,7 @@ void *vmemdup_user(const void __user *src, size_t len)
{
void *p;
- p = kvmalloc(len, GFP_USER);
+ p = kmem_buckets_valloc(user_buckets, len, GFP_USER);
if (!p)
return ERR_PTR(-ENOMEM);
--
2.34.1
[PATCH v4 1/6] mm/slab: Introduce kmem_buckets typedef
Encapsulate the concept of a single set of kmem_caches that are used
for the kmalloc size buckets. Redefine kmalloc_caches as an array
of these buckets (for the different global cache buckets).
Signed-off-by: Kees Cook
---
Cc: Vlastimil Babka
Cc: Christoph Lameter
Cc: Pekka Enberg
Cc: David Rientjes
Cc: Joonsoo Kim
Cc: jvoisin
Cc: Andrew Morton
Cc: Roman Gushchin
Cc: Hyeonggon Yoo <42.hye...@gmail.com>
Cc: linux...@kvack.org
---
include/linux/slab.h | 5 +++--
mm/slab_common.c | 3 +--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 7247e217e21b..de2b7209cd05 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -426,8 +426,9 @@ enum kmalloc_cache_type {
NR_KMALLOC_TYPES
};
-extern struct kmem_cache *
-kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1];
+typedef struct kmem_cache * kmem_buckets[KMALLOC_SHIFT_HIGH + 1];
+
+extern kmem_buckets kmalloc_caches[NR_KMALLOC_TYPES];
/*
* Define gfp bits that should not be set for KMALLOC_NORMAL.
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 1560a1546bb1..e0b1c109bed2 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -653,8 +653,7 @@ static struct kmem_cache *__init create_kmalloc_cache(const
char *name,
return s;
}
-struct kmem_cache *
-kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1] __ro_after_init =
+kmem_buckets kmalloc_caches[NR_KMALLOC_TYPES] __ro_after_init =
{ /* initialization for https://llvm.org/pr42570 */ };
EXPORT_SYMBOL(kmalloc_caches);
--
2.34.1
[PATCH v4 4/6] mm/slab: Introduce kmem_buckets_create() and family
Dedicated caches are available for fixed size allocations via
kmem_cache_alloc(), but for dynamically sized allocations there is only
the global kmalloc API's set of buckets available. This means it isn't
possible to separate specific sets of dynamically sized allocations into
a separate collection of caches.
This leads to a use-after-free exploitation weakness in the Linux
kernel since many heap memory spraying/grooming attacks depend on using
userspace-controllable dynamically sized allocations to collide with
fixed size allocations that end up in same cache.
While CONFIG_RANDOM_KMALLOC_CACHES provides a probabilistic defense
against these kinds of "type confusion" attacks, including for fixed
same-size heap objects, we can create a complementary deterministic
defense for dynamically sized allocations that are directly user
controlled. Addressing these cases is limited in scope, so isolating these
kinds of interfaces will not become an unbounded game of whack-a-mole. For
example, many pass through memdup_user(), making isolation there very
effective.
In order to isolate user-controllable dynamically-sized
allocations from the common system kmalloc allocations, introduce
kmem_buckets_create(), which behaves like kmem_cache_create(). Introduce
kmem_buckets_alloc(), which behaves like kmem_cache_alloc(). Introduce
kmem_buckets_alloc_track_caller() for where caller tracking is
needed. Introduce kmem_buckets_valloc() for cases where vmalloc fallback
is needed.
This can also be used in the future to extend allocation profiling's use
of code tagging to implement per-caller allocation cache isolation[1]
even for dynamic allocations.
Memory allocation pinning[2] is still needed to plug the Use-After-Free
cross-allocator weakness, but that is an existing and separate issue
which is complementary to this improvement. Development continues for
that feature via the SLAB_VIRTUAL[3] series (which could also provide
guard pages -- another complementary improvement).
Link: https://lore.kernel.org/lkml/202402211449.401382D2AF@keescook [1]
Link:
https://googleprojectzero.blogspot.com/2021/10/how-simple-linux-kernel-memory.html
[2]
Link:
https://lore.kernel.org/lkml/20230915105933.495735-1-matteori...@google.com/ [3]
Signed-off-by: Kees Cook
---
Cc: Vlastimil Babka
Cc: Christoph Lameter
Cc: Pekka Enberg
Cc: David Rientjes
Cc: Joonsoo Kim
Cc: jvoisin
Cc: Andrew Morton
Cc: Roman Gushchin
Cc: Hyeonggon Yoo <42.hye...@gmail.com>
Cc: linux...@kvack.org
---
include/linux/slab.h | 12 +++
mm/slab_common.c | 80
2 files changed, 92 insertions(+)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8853c6eb20b4..b48c50d90aae 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -552,6 +552,11 @@ void *kmem_cache_alloc_lru_noprof(struct kmem_cache *s,
struct list_lru *lru,
void kmem_cache_free(struct kmem_cache *s, void *objp);
+kmem_buckets *kmem_buckets_create(const char *name, unsigned int align,
+ slab_flags_t flags,
+ unsigned int useroffset, unsigned int
usersize,
+ void (*ctor)(void *));
+
/*
* Bulk allocation and freeing operations. These are accelerated in an
* allocator specific way to avoid taking locks repeatedly or building
@@ -675,6 +680,12 @@ static __always_inline __alloc_size(1) void
*kmalloc_noprof(size_t size, gfp_t f
}
#define kmalloc(...)
alloc_hooks(kmalloc_noprof(__VA_ARGS__))
+#define kmem_buckets_alloc(_b, _size, _flags) \
+ alloc_hooks(__kmalloc_node_noprof(_b, _size, _flags, NUMA_NO_NODE))
+
+#define kmem_buckets_alloc_track_caller(_b, _size, _flags) \
+ alloc_hooks(kmalloc_node_track_caller_noprof(_b, _size, _flags,
NUMA_NO_NODE, _RET_IP_))
+
static __always_inline __alloc_size(1) void *kmalloc_node_noprof(size_t size,
gfp_t flags, int node)
{
if (__builtin_constant_p(size) && size) {
@@ -818,6 +829,7 @@ extern void *kvmalloc_buckets_node_noprof(size_t size,
gfp_t flags, int node)
#define kvzalloc(_size, _flags)kvmalloc(_size,
(_flags)|__GFP_ZERO)
#define kvzalloc_node(_size, _flags, _node)kvmalloc_node(_size,
(_flags)|__GFP_ZERO, _node)
+#define kmem_buckets_valloc(_b, _size, _flags) kvmalloc_buckets_node(_b,
_size, _flags, NUMA_NO_NODE)
static inline __alloc_size(1, 2) void *
kvmalloc_array_node_noprof(size_t n, size_t size, gfp_t flags, int node)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index b5c879fa66bc..f42a98d368a9 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -392,6 +392,82 @@ kmem_cache_create(const char *name, unsigned int size,
unsigned int align,
}
EXPORT_SYMBOL(kmem_cache_create);
+static struct kmem_cache *kmem_buckets_cache __ro_after_init;
+
+kmem_buckets *kmem_buckets_create(const char *name, unsigned int align,
+
[PATCH v4 3/6] mm/slab: Introduce kvmalloc_buckets_node() that can take kmem_buckets argument
Plumb kmem_buckets arguments through kvmalloc_node_noprof() so it is
possible to provide an API to perform kvmalloc-style allocations with
a particular set of buckets. Introduce kvmalloc_buckets_node() that takes a
kmem_buckets argument.
Signed-off-by: Kees Cook
---
Cc: Vlastimil Babka
Cc: Christoph Lameter
Cc: Pekka Enberg
Cc: David Rientjes
Cc: Joonsoo Kim
Cc: jvoisin
Cc: Andrew Morton
Cc: Roman Gushchin
Cc: Hyeonggon Yoo <42.hye...@gmail.com>
Cc: linux...@kvack.org
---
include/linux/slab.h | 19 +++
lib/rhashtable.c | 2 +-
mm/util.c| 13 +
3 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index b1165b22cc6f..8853c6eb20b4 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -799,11 +799,22 @@ static inline __alloc_size(1) void *kzalloc_noprof(size_t
size, gfp_t flags)
#define kzalloc(...)
alloc_hooks(kzalloc_noprof(__VA_ARGS__))
#define kzalloc_node(_size, _flags, _node) kmalloc_node(_size,
(_flags)|__GFP_ZERO, _node)
-extern void *kvmalloc_node_noprof(size_t size, gfp_t flags, int node)
__alloc_size(1);
-#define kvmalloc_node(...)
alloc_hooks(kvmalloc_node_noprof(__VA_ARGS__))
+#ifdef CONFIG_SLAB_BUCKETS
+extern void *kvmalloc_buckets_node_noprof(kmem_buckets *b, size_t size, gfp_t
flags, int node)
+ __alloc_size(2);
+# define kvmalloc_node_noprof(b, size, flags, node)\
+ kvmalloc_buckets_node_noprof(b, size, flags, node)
+#else
+extern void *kvmalloc_buckets_node_noprof(size_t size, gfp_t flags, int node)
+ __alloc_size(1);
+# define kvmalloc_node_noprof(b, size, flags, node)\
+ kvmalloc_buckets_node_noprof(size, flags, node)
+#endif
+#define kvmalloc_buckets_node(...)
alloc_hooks(kvmalloc_node_noprof(__VA_ARGS__))
+#define kvmalloc_node(...) kvmalloc_buckets_node(NULL,
__VA_ARGS__)
#define kvmalloc(_size, _flags)kvmalloc_node(_size,
_flags, NUMA_NO_NODE)
-#define kvmalloc_noprof(_size, _flags) kvmalloc_node_noprof(_size,
_flags, NUMA_NO_NODE)
+#define kvmalloc_noprof(_size, _flags) kvmalloc_node_noprof(NULL,
_size, _flags, NUMA_NO_NODE)
#define kvzalloc(_size, _flags)kvmalloc(_size,
(_flags)|__GFP_ZERO)
#define kvzalloc_node(_size, _flags, _node)kvmalloc_node(_size,
(_flags)|__GFP_ZERO, _node)
@@ -816,7 +827,7 @@ kvmalloc_array_node_noprof(size_t n, size_t size, gfp_t
flags, int node)
if (unlikely(check_mul_overflow(n, size, )))
return NULL;
- return kvmalloc_node_noprof(bytes, flags, node);
+ return kvmalloc_node_noprof(NULL, bytes, flags, node);
}
#define kvmalloc_array_noprof(...)
kvmalloc_array_node_noprof(__VA_ARGS__, NUMA_NO_NODE)
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index dbbed19f8fff..ef0f496e4aed 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -184,7 +184,7 @@ static struct bucket_table *bucket_table_alloc(struct
rhashtable *ht,
static struct lock_class_key __key;
tbl = alloc_hooks_tag(ht->alloc_tag,
- kvmalloc_node_noprof(struct_size(tbl, buckets,
nbuckets),
+ kvmalloc_node_noprof(NULL, struct_size(tbl, buckets,
nbuckets),
gfp|__GFP_ZERO, NUMA_NO_NODE));
size = nbuckets;
diff --git a/mm/util.c b/mm/util.c
index 80430e5ba981..53f7fc5912bd 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -593,9 +593,11 @@ unsigned long vm_mmap(struct file *file, unsigned long
addr,
}
EXPORT_SYMBOL(vm_mmap);
+#ifdef CONFIG_SLAB_BUCKETS
/**
- * kvmalloc_node - attempt to allocate physically contiguous memory, but upon
+ * kvmalloc_buckets_node_noprof - attempt to allocate physically contiguous
memory, but upon
* failure, fall back to non-contiguous (vmalloc) allocation.
+ * @b: which set of kmalloc buckets to allocate from.
* @size: size of the request.
* @flags: gfp mask for the allocation - must be compatible (superset) with
GFP_KERNEL.
* @node: numa node to allocate from
@@ -609,7 +611,10 @@ EXPORT_SYMBOL(vm_mmap);
*
* Return: pointer to the allocated memory of %NULL in case of failure
*/
-void *kvmalloc_node_noprof(size_t size, gfp_t flags, int node)
+void *kvmalloc_buckets_node_noprof(kmem_buckets *b, size_t size, gfp_t flags,
int node)
+#else
+void *kvmalloc_buckets_node_noprof(size_t size, gfp_t flags, int node)
+#endif
{
gfp_t kmalloc_flags = flags;
void *ret;
@@ -631,7 +636,7 @@ void *kvmalloc_node_noprof(size_t size, gfp_t flags, int
node)
kmalloc_flags &= ~__GFP_NOFAIL;
}
- ret = kmalloc_node_noprof(size, kmalloc_flags, node);
+ ret = __kmalloc_node_noprof(b, size, kmalloc_flags, node);
/*
* It doesn't real
[PATCH v4 0/6] slab: Introduce dedicated bucket allocator
Hi, v4: - Rebase to v6.10-rc1 - Add CONFIG_SLAB_BUCKETS to turn off the feature v3: https://lore.kernel.org/lkml/20240424213019.make.366-k...@kernel.org/ v2: https://lore.kernel.org/lkml/20240305100933.it.923-k...@kernel.org/ v1: https://lore.kernel.org/lkml/20240304184252.work.496-k...@kernel.org/ For the cover letter, I'm repeating commit log for patch 4 here, which has additional clarifications and rationale since v2: Dedicated caches are available for fixed size allocations via kmem_cache_alloc(), but for dynamically sized allocations there is only the global kmalloc API's set of buckets available. This means it isn't possible to separate specific sets of dynamically sized allocations into a separate collection of caches. This leads to a use-after-free exploitation weakness in the Linux kernel since many heap memory spraying/grooming attacks depend on using userspace-controllable dynamically sized allocations to collide with fixed size allocations that end up in same cache. While CONFIG_RANDOM_KMALLOC_CACHES provides a probabilistic defense against these kinds of "type confusion" attacks, including for fixed same-size heap objects, we can create a complementary deterministic defense for dynamically sized allocations that are directly user controlled. Addressing these cases is limited in scope, so isolation these kinds of interfaces will not become an unbounded game of whack-a-mole. For example, pass through memdup_user(), making isolation there very effective. In order to isolate user-controllable sized allocations from system allocations, introduce kmem_buckets_create(), which behaves like kmem_cache_create(). Introduce kmem_buckets_alloc(), which behaves like kmem_cache_alloc(). Introduce kmem_buckets_alloc_track_caller() for where caller tracking is needed. Introduce kmem_buckets_valloc() for cases where vmalloc callback is needed. Allows for confining allocations to a dedicated set of sized caches (which have the same layout as the kmalloc caches). This can also be used in the future to extend codetag allocation annotations to implement per-caller allocation cache isolation[1] even for dynamic allocations. Memory allocation pinning[2] is still needed to plug the Use-After-Free cross-allocator weakness, but that is an existing and separate issue which is complementary to this improvement. Development continues for that feature via the SLAB_VIRTUAL[3] series (which could also provide guard pages -- another complementary improvement). Link: https://lore.kernel.org/lkml/202402211449.401382D2AF@keescook [1] Link: https://googleprojectzero.blogspot.com/2021/10/how-simple-linux-kernel-memory.html [2] Link: https://lore.kernel.org/lkml/20230915105933.495735-1-matteori...@google.com/ [3] After the core implementation are 2 patches that cover the most heavily abused "repeat offenders" used in exploits. Repeating those details here: The msg subsystem is a common target for exploiting[1][2][3][4][5][6] use-after-free type confusion flaws in the kernel for both read and write primitives. Avoid having a user-controlled size cache share the global kmalloc allocator by using a separate set of kmalloc buckets. Link: https://blog.hacktivesecurity.com/index.php/2022/06/13/linux-kernel-exploit-development-1day-case-study/ [1] Link: https://hardenedvault.net/blog/2022-11-13-msg_msg-recon-mitigation-ved/ [2] Link: https://www.willsroot.io/2021/08/corctf-2021-fire-of-salvation-writeup.html [3] Link: https://a13xp0p0v.github.io/2021/02/09/CVE-2021-26708.html [4] Link: https://google.github.io/security-research/pocs/linux/cve-2021-22555/writeup.html [5] Link: https://zplin.me/papers/ELOISE.pdf [6] Link: https://syst3mfailure.io/wall-of-perdition/ [7] Both memdup_user() and vmemdup_user() handle allocations that are regularly used for exploiting use-after-free type confusion flaws in the kernel (e.g. prctl() PR_SET_VMA_ANON_NAME[1] and setxattr[2][3][4] respectively). Since both are designed for contents coming from userspace, it allows for userspace-controlled allocation sizes. Use a dedicated set of kmalloc buckets so these allocations do not share caches with the global kmalloc buckets. Link: https://starlabs.sg/blog/2023/07-prctl-anon_vma_name-an-amusing-heap-spray/ [1] Link: https://duasynt.com/blog/linux-kernel-heap-spray [2] Link: https://etenal.me/archives/1336 [3] Link: https://github.com/a13xp0p0v/kernel-hack-drill/blob/master/drill_exploit_uaf.c [4] Thanks! -Kees Kees Cook (6): mm/slab: Introduce kmem_buckets typedef mm/slab: Plumb kmem_buckets into __do_kmalloc_node() mm/slab: Introduce kvmalloc_buckets_node() that can take kmem_buckets argument mm/slab: Introduce
[PATCH v4 2/6] mm/slab: Plumb kmem_buckets into __do_kmalloc_node()
Introduce CONFIG_SLAB_BUCKETS which provides the infrastructure to support separated kmalloc buckets (in the follow kmem_buckets_create() patches and future codetag-based separation). Since this will provide a mitigation for a very common case of exploits, enable it by default. To be able to choose which buckets to allocate from, make the buckets available to the internal kmalloc interfaces by adding them as the first argument, rather than depending on the buckets being chosen from the fixed set of global buckets. Where the bucket is not available, pass NULL, which means "use the default system kmalloc bucket set" (the prior existing behavior), as implemented in kmalloc_slab(). To avoid adding the extra argument when !CONFIG_SLAB_BUCKETS, only the top-level macros and static inlines use the buckets argument (where they are stripped out and compiled out respectively). The actual extern functions can then been built without the argument, and the internals fall back to the global kmalloc buckets unconditionally. Signed-off-by: Kees Cook --- Cc: Vlastimil Babka Cc: Christoph Lameter Cc: Pekka Enberg Cc: David Rientjes Cc: Joonsoo Kim Cc: jvoisin Cc: Andrew Morton Cc: Roman Gushchin Cc: Hyeonggon Yoo <42.hye...@gmail.com> Cc: linux...@kvack.org Cc: linux-hardening@vger.kernel.org --- include/linux/slab.h | 34 ++ mm/Kconfig | 15 +++ mm/slab.h| 6 -- mm/slab_common.c | 4 ++-- mm/slub.c| 34 -- mm/util.c| 2 +- 6 files changed, 72 insertions(+), 23 deletions(-) diff --git a/include/linux/slab.h b/include/linux/slab.h index de2b7209cd05..b1165b22cc6f 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -569,8 +569,17 @@ static __always_inline void kfree_bulk(size_t size, void **p) kmem_cache_free_bulk(NULL, size, p); } -void *__kmalloc_node_noprof(size_t size, gfp_t flags, int node) __assume_kmalloc_alignment -__alloc_size(1); +#ifdef CONFIG_SLAB_BUCKETS +void *__kmalloc_buckets_node_noprof(kmem_buckets *b, size_t size, gfp_t flags, int node) + __assume_kmalloc_alignment __alloc_size(2); +# define __kmalloc_node_noprof(b, size, flags, node) \ + __kmalloc_buckets_node_noprof(b, size, flags, node) +#else +void *__kmalloc_buckets_node_noprof(size_t size, gfp_t flags, int node) + __assume_kmalloc_alignment __alloc_size(1); +# define __kmalloc_node_noprof(b, size, flags, node) \ + __kmalloc_buckets_node_noprof(size, flags, node) +#endif #define __kmalloc_node(...) alloc_hooks(__kmalloc_node_noprof(__VA_ARGS__)) void *kmem_cache_alloc_node_noprof(struct kmem_cache *s, gfp_t flags, @@ -679,7 +688,7 @@ static __always_inline __alloc_size(1) void *kmalloc_node_noprof(size_t size, gf kmalloc_caches[kmalloc_type(flags, _RET_IP_)][index], flags, node, size); } - return __kmalloc_node_noprof(size, flags, node); + return __kmalloc_node_noprof(NULL, size, flags, node); } #define kmalloc_node(...) alloc_hooks(kmalloc_node_noprof(__VA_ARGS__)) @@ -730,10 +739,19 @@ static inline __realloc_size(2, 3) void * __must_check krealloc_array_noprof(voi */ #define kcalloc(n, size, flags)kmalloc_array(n, size, (flags) | __GFP_ZERO) -void *kmalloc_node_track_caller_noprof(size_t size, gfp_t flags, int node, - unsigned long caller) __alloc_size(1); +#ifdef CONFIG_SLAB_BUCKETS +void *__kmalloc_node_track_caller_noprof(kmem_buckets *b, size_t size, gfp_t flags, int node, +unsigned long caller) __alloc_size(2); +# define kmalloc_node_track_caller_noprof(b, size, flags, node, caller) \ + __kmalloc_node_track_caller_noprof(b, size, flags, node, caller) +#else +void *__kmalloc_node_track_caller_noprof(size_t size, gfp_t flags, int node, +unsigned long caller) __alloc_size(1); +# define kmalloc_node_track_caller_noprof(b, size, flags, node, caller) \ + __kmalloc_node_track_caller_noprof(size, flags, node, caller) +#endif #define kmalloc_node_track_caller(...) \ - alloc_hooks(kmalloc_node_track_caller_noprof(__VA_ARGS__, _RET_IP_)) + alloc_hooks(kmalloc_node_track_caller_noprof(NULL, __VA_ARGS__, _RET_IP_)) /* * kmalloc_track_caller is a special version of kmalloc that records the @@ -746,7 +764,7 @@ void *kmalloc_node_track_caller_noprof(size_t size, gfp_t flags, int node, #define kmalloc_track_caller(...) kmalloc_node_track_caller(__VA_ARGS__, NUMA_NO_NODE) #define kmalloc_track_caller_noprof(...) \ - kmalloc_node_track_caller_noprof(__VA_ARGS__, NUMA_NO
[PATCH] kunit/fortify: Remove __kmalloc_node() test
__kmalloc_node() is considered an "internal" function to the Slab, so drop it from explicit testing. 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);\ -- 2.34.1
Re: [PATCH] x86/boot: add prototype for __fortify_panic()
On Thu, May 30, 2024 at 06:46:39PM +0200, Borislav Petkov wrote: > On May 30, 2024 6:23:36 PM GMT+02:00, 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 :) > > The more important question is how does the decompressor build even know of > this symbol? And then make it forget it again instead of adding silly > prototypes... Under CONFIG_FORTIFY_SOURCE, the boot code *does* still uses fortify-string.h. It lets us both catch mistakes we can discover at compile and will catch egregious runtime mistakes, though the reporting is much simpler in the boot code. -- Kees Cook
Re: [PATCH v3 2/6] mm/slab: Plumb kmem_buckets into __do_kmalloc_node()
On Fri, May 24, 2024 at 11:01:40AM -0400, Kent Overstreet wrote: > On Wed, Apr 24, 2024 at 02:40:59PM -0700, Kees Cook wrote: > > To be able to choose which buckets to allocate from, make the buckets > > available to the lower level kmalloc interfaces by adding them as the > > first argument. Where the bucket is not available, pass NULL, which means > > "use the default system kmalloc bucket set" (the prior existing behavior), > > as implemented in kmalloc_slab(). > > I thought the plan was to use codetags for this? That would obviate the > need for all this plumbing. > > Add fields to the alloc tag for: > - allocation size (or 0 if it's not a compile time constant) > - union of kmem_cache, kmem_buckets, depending on whether the >allocation size is constant or not I want to provide "simple" (low-hanging fruit) coverage that can live separately from the codetags-based coverage. The memory overhead for this patch series is negligible, but I suspect the codetags expansion, while not giant, will be more than some deployments will want. I want to avoid an all-or-nothing solution -- which is why I had intended this to be available "by default". But I will respin this with kmem_buckets under a Kconfig. -- Kees Cook
Re: [PATCH v3 2/6] mm/slab: Plumb kmem_buckets into __do_kmalloc_node()
On Fri, May 24, 2024 at 03:38:58PM +0200, Vlastimil Babka wrote: > On 4/24/24 11:40 PM, Kees Cook wrote: > > To be able to choose which buckets to allocate from, make the buckets > > available to the lower level kmalloc interfaces by adding them as the > > first argument. Where the bucket is not available, pass NULL, which means > > "use the default system kmalloc bucket set" (the prior existing behavior), > > as implemented in kmalloc_slab(). > > > > Signed-off-by: Kees Cook > > --- > > Cc: Vlastimil Babka > > Cc: Christoph Lameter > > Cc: Pekka Enberg > > Cc: David Rientjes > > Cc: Joonsoo Kim > > Cc: Andrew Morton > > Cc: Roman Gushchin > > Cc: Hyeonggon Yoo <42.hye...@gmail.com> > > Cc: linux...@kvack.org > > Cc: linux-hardening@vger.kernel.org > > --- > > include/linux/slab.h | 16 > > lib/fortify_kunit.c | 2 +- > > mm/slab.h| 6 -- > > mm/slab_common.c | 4 ++-- > > mm/slub.c| 14 +++--- > > mm/util.c| 2 +- > > 6 files changed, 23 insertions(+), 21 deletions(-) > > > > diff --git a/include/linux/slab.h b/include/linux/slab.h > > index c8164d5db420..07373b680894 100644 > > --- a/include/linux/slab.h > > +++ b/include/linux/slab.h > > @@ -569,8 +569,8 @@ static __always_inline void kfree_bulk(size_t size, > > void **p) > > kmem_cache_free_bulk(NULL, size, p); > > } > > > > -void *__kmalloc_node_noprof(size_t size, gfp_t flags, int node) > > __assume_kmalloc_alignment > > -__alloc_size(1); > > +void *__kmalloc_node_noprof(kmem_buckets *b, size_t size, gfp_t flags, int > > node) > > + __assume_kmalloc_alignment __alloc_size(2); > > #define __kmalloc_node(...) > > alloc_hooks(__kmalloc_node_noprof(__VA_ARGS__)) > > > > void *kmem_cache_alloc_node_noprof(struct kmem_cache *s, gfp_t flags, > > @@ -679,7 +679,7 @@ static __always_inline __alloc_size(1) void > > *kmalloc_node_noprof(size_t size, gf > > kmalloc_caches[kmalloc_type(flags, > > _RET_IP_)][index], > > flags, node, size); > > } > > - return __kmalloc_node_noprof(size, flags, node); > > + return __kmalloc_node_noprof(NULL, size, flags, node); > > This is not ideal as now every kmalloc_node() callsite will now have to add > the NULL parameter even if this is not enabled. Could the new parameter be > only added depending on the respective config? I felt like it was much simpler to add an argument to the existing call path than to create a duplicate API that had 1 extra argument. However, if you want this behind a Kconfig option, I can redefine the argument list based on that? -- Kees Cook
Re: [PATCH v3 0/6] slab: Introduce dedicated bucket allocator
On Fri, May 24, 2024 at 10:54:58AM -0400, Kent Overstreet wrote: > On Wed, Apr 24, 2024 at 02:40:57PM -0700, Kees Cook wrote: > > Hi, > > > > Series change history: > > > > v3: > > - clarify rationale and purpose in commit log > > - rebase to -next (CONFIG_CODE_TAGGING) > > - simplify calling styles and split out bucket plumbing more cleanly > > - consolidate kmem_buckets_*() family introduction patches > > v2: https://lore.kernel.org/lkml/20240305100933.it.923-k...@kernel.org/ > > v1: https://lore.kernel.org/lkml/20240304184252.work.496-k...@kernel.org/ > > > > For the cover letter, I'm repeating commit log for patch 4 here, which has > > additional clarifications and rationale since v2: > > > > Dedicated caches are available for fixed size allocations via > > kmem_cache_alloc(), but for dynamically sized allocations there is only > > the global kmalloc API's set of buckets available. This means it isn't > > possible to separate specific sets of dynamically sized allocations into > > a separate collection of caches. > > > > This leads to a use-after-free exploitation weakness in the Linux > > kernel since many heap memory spraying/grooming attacks depend on using > > userspace-controllable dynamically sized allocations to collide with > > fixed size allocations that end up in same cache. > > This is going to increase internal fragmentation in the slab allocator, > so we're going to need better, more visible numbers on the amount of > memory stranded thusly, so users can easily see the effect this has. Yes, but not significantly. It's less than the 16-buckets randomized kmalloc implementation. The numbers will be visible in /proc/slabinfo just like any other. > Please also document this effect and point users in the documentation > where to check, so that we devs can get feedback on this. Okay, sure. In the commit log, or did you have somewhere else in mind? -- Kees Cook
Re: [PATCH v3 4/6] mm/slab: Introduce kmem_buckets_create() and family
On Fri, May 24, 2024 at 03:43:33PM +0200, Vlastimil Babka wrote: > On 4/24/24 11:41 PM, Kees Cook wrote: > > Dedicated caches are available for fixed size allocations via > > kmem_cache_alloc(), but for dynamically sized allocations there is only > > the global kmalloc API's set of buckets available. This means it isn't > > possible to separate specific sets of dynamically sized allocations into > > a separate collection of caches. > > > > This leads to a use-after-free exploitation weakness in the Linux > > kernel since many heap memory spraying/grooming attacks depend on using > > userspace-controllable dynamically sized allocations to collide with > > fixed size allocations that end up in same cache. > > > > While CONFIG_RANDOM_KMALLOC_CACHES provides a probabilistic defense > > against these kinds of "type confusion" attacks, including for fixed > > same-size heap objects, we can create a complementary deterministic > > defense for dynamically sized allocations that are directly user > > controlled. Addressing these cases is limited in scope, so isolation these > > kinds of interfaces will not become an unbounded game of whack-a-mole. For > > example, pass through memdup_user(), making isolation there very > > effective. > > > > In order to isolate user-controllable sized allocations from system > > allocations, introduce kmem_buckets_create(), which behaves like > > kmem_cache_create(). Introduce kmem_buckets_alloc(), which behaves like > > kmem_cache_alloc(). Introduce kmem_buckets_alloc_track_caller() for > > where caller tracking is needed. Introduce kmem_buckets_valloc() for > > cases where vmalloc callback is needed. > > > > Allows for confining allocations to a dedicated set of sized caches > > (which have the same layout as the kmalloc caches). > > > > This can also be used in the future to extend codetag allocation > > annotations to implement per-caller allocation cache isolation[1] even > > for dynamic allocations. > > > > Memory allocation pinning[2] is still needed to plug the Use-After-Free > > cross-allocator weakness, but that is an existing and separate issue > > which is complementary to this improvement. Development continues for > > that feature via the SLAB_VIRTUAL[3] series (which could also provide > > guard pages -- another complementary improvement). > > > > Link: https://lore.kernel.org/lkml/202402211449.401382D2AF@keescook [1] > > Link: > > https://googleprojectzero.blogspot.com/2021/10/how-simple-linux-kernel-memory.html > > [2] > > Link: > > https://lore.kernel.org/lkml/20230915105933.495735-1-matteori...@google.com/ > > [3] > > Signed-off-by: Kees Cook > > So this seems like it's all unconditional and not depending on a config > option? I'd rather see one, as has been done for all similar functionality > before, as not everyone will want this trade-off. Okay, sure, I can do that. Since it was changing some of the core APIs to pass in the target buckets (instead of using the global buckets), it seemed less complex to me to just make the simple changes unconditional. > Also AFAIU every new user (patches 5, 6) will add new bunch of lines to > /proc/slabinfo? And when you integrate alloc tagging, do you expect every Yes, that's true, but that's the goal: they want to live in a separate bucket collection. At present, it's only two, and arguable almost everything that the manual isolation provides should be using the mem_from_user() interface anyway. > callsite will do that as well? Any idea how many there would be and what > kind of memory overhead it will have in the end? I haven't measured the per-codetag-site overhead, but I don't expect it to have giant overhead. If the buckets are created on demand, it should be small. But one step at a time. This provides the infrastructure needed to immediately solve the low-hanging exploitable fruit and to pave the way for the per-codetag-site automation. -Kees -- Kees Cook
Re: [PATCH] x86/boot: add prototype for __fortify_panic()
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:
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.)
--
Kees Cook
Re: [PATCH] x86/traps: Enable UBSAN traps on x86
On Wed, May 29, 2024 at 01:36:39PM -0700, Gatlin Newhouse wrote:
> On Wed, May 29, 2024 at 08:30:20PM UTC, Marco Elver wrote:
> > On Wed, 29 May 2024 at 20:17, Gatlin Newhouse
> > wrote:
> > >
> > > On Wed, May 29, 2024 at 09:25:21AM UTC, Marco Elver wrote:
> > > > On Wed, 29 May 2024 at 04:20, Gatlin Newhouse
> > > > wrote:
> > > > [...]
> > > > > if (regs->flags & X86_EFLAGS_IF)
> > > > > raw_local_irq_enable();
> > > > > - if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
> > > > > - handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
> > > > > - regs->ip += LEN_UD2;
> > > > > - handled = true;
> > > > > +
> > > > > + if (insn == INSN_UD2) {
> > > > > + if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN
> > > > > ||
> > > > > + handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
> > > > > + regs->ip += LEN_UD2;
> > > > > + handled = true;
> > > > > + }
> > > > > + } else {
> > > > > + if (handle_ubsan_failure(regs, insn) ==
> > > > > BUG_TRAP_TYPE_WARN) {
> > > >
> > > > handle_ubsan_failure currently only returns BUG_TRAP_TYPE_NONE?
> > > >
> > > > > + if (insn == INSN_REX)
> > > > > + regs->ip += LEN_REX;
> > > > > + regs->ip += LEN_UD1;
> > > > > + handled = true;
> > > > > + }
> > > > > }
> > > > > if (regs->flags & X86_EFLAGS_IF)
> > > > > raw_local_irq_disable();
> > > > > diff --git a/arch/x86/kernel/ubsan.c b/arch/x86/kernel/ubsan.c
> > > > > new file mode 100644
> > > > > index ..6cae11f4fe23
> > > > > --- /dev/null
> > > > > +++ b/arch/x86/kernel/ubsan.c
> > > > > @@ -0,0 +1,32 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Clang Undefined Behavior Sanitizer trap mode support.
> > > > > + */
> > > > > +#include
> > > > > +#include
> > > > > +#include
> > > > > +#include
> > > > > +#include
> > > > > +#include
> > > > > +
> > > > > +/*
> > > > > + * Checks for the information embedded in the UD1 trap instruction
> > > > > + * for the UB Sanitizer in order to pass along debugging output.
> > > > > + */
> > > > > +enum bug_trap_type handle_ubsan_failure(struct pt_regs *regs, int
> > > > > insn)
> > > > > +{
> > > > > + u32 type = 0;
> > > > > +
> > > > > + if (insn == INSN_REX) {
> > > > > + type = (*(u16 *)(regs->ip + LEN_REX + 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;
> > > > > + }
> > > > > + pr_crit("%s at %pS\n", report_ubsan_failure(regs, type),
> > > > > (void *)regs->ip);
> > > > > +
> > > > > + return BUG_TRAP_TYPE_NONE;
> > > > > +}
> > > >
> > > > Shouldn't this return BUG_TRAP_TYPE_WARN?
> > >
> > > So as far as I understand, UBSAN trap mode never warns. Perhaps it does on
> > > arm64, although it calls die() so I am unsure. Maybe the condition in
> > > handle_bug() should be rewritten in the case of UBSAN ud1s? Do you have
> > > any
> > > suggestions?
> >
> > AFAIK on arm64 it's basically a kernel OOPS.
> >
> > The main thing I just wanted to point out though is that your newly added
> > branch
> >
> > > if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
> >
> > will never be taken, because I don't see where handle_ubsan_failure()
> > returns BUG_TRAP_TYPE_WARN.
> >
>
> Initially I wrote this with some symmetry to the KCFI checks nearby, but I
> was unsure if this would be considered handled or not.
Yeah, that seemed like the right "style" to me too. Perhaps, since it
can never warn, we could just rewrite it so it's a void function avoid
the checking, etc.
--
Kees Cook
Re: __fortify_panic() question
On Wed, May 29, 2024 at 10:09:45AM -0700, Jeff Johnson wrote: > On 5/29/2024 9:55 AM, Kees Cook wrote: > > On Wed, May 29, 2024 at 07:36:25AM -0700, Jeff Johnson wrote: > >> 'make W=1 C=1' on x86 gives the warning: > >> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' > >> was not declared. Should it be static? > > > > Hm, I can't reproduce this currently (but yes, it looks like arm vs x86 > > is mismatched). What tree is this? > > e0cce98fe279 (linus/master, linux-master) Merge tag 'tpmdd-next-6.10-rc2' of > git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd > > > > >> Looking at this I see for ARM there is a prototype for __fortify_panic() in > >> arch/arm/boot/compressed/misc.h > >> And there is a matching implementation in arch/arm/boot/compressed/misc.c > >> > >> But for x86 there is only the implementation in > >> arch/x86/boot/compressed/misc.c > >> There is not a prototype in arch/x86/boot/compressed/misc.h. > >> > >> The easy fix for this would be to add a prototype to > >> arch/x86/boot/compressed/misc.h. > > > > Yeah, I think this is the right solution. > > You want to do this, or should I? Please feel free! I'd appreciate it. :) -- Kees Cook
Re: __fortify_panic() question
On Wed, May 29, 2024 at 07:36:25AM -0700, Jeff Johnson wrote: > 'make W=1 C=1' on x86 gives the warning: > arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was > not declared. Should it be static? Hm, I can't reproduce this currently (but yes, it looks like arm vs x86 is mismatched). What tree is this? > Looking at this I see for ARM there is a prototype for __fortify_panic() in > arch/arm/boot/compressed/misc.h > And there is a matching implementation in arch/arm/boot/compressed/misc.c > > But for x86 there is only the implementation in > arch/x86/boot/compressed/misc.c > There is not a prototype in arch/x86/boot/compressed/misc.h. > > The easy fix for this would be to add a prototype to > arch/x86/boot/compressed/misc.h. Yeah, I think this is the right solution. > But it seems strange to me to add a prototype to a header file that is only > for the benefit of the callee and is not the prototype/header used by the > caller, in this case the one in include/linux/fortify-string.h The stuff in boot/ doesn't tend to include fortify-string.h (since it's sort of "outside" the kernel), hence the need for additional prototypes. -- Kees Cook
Re: [PATCH] drm/i915: 2 GiB of relocations ought to be enough for anybody*
On Tue, May 21, 2024 at 11:12:01AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> Kernel test robot reports i915 can hit a warn in kvmalloc_node which has
> a purpose of dissalowing crazy size kernel allocations. This was added in
> 7661809d493b ("mm: don't allow oversized kvmalloc() calls"):
>
>/* Don't even allow crazy sizes */
>if (WARN_ON_ONCE(size > INT_MAX))
>return NULL;
>
> This would be kind of okay since i915 at one point dropped the need for
> making a shadow copy of the relocation list, but then it got re-added in
> fd1500fcd442 ("Revert "drm/i915/gem: Drop relocation slowpath".") a year
> after Linus added the above warning.
>
> It is plausible that the issue was not seen until now because to trigger
> gem_exec_reloc test requires a combination of an relatively older
> generation hardware but with at least 8GiB of RAM installed. Probably even
> more depending on runtime checks.
>
> Lets cap what we allow userspace to pass in using the matching limit.
> There should be no issue for real userspace since we are talking about
> "crazy" number of relocations which have no practical purpose.
>
> *) Well IGT tests might get upset but they can be easily adjusted.
>
> Signed-off-by: Tvrtko Ursulin
Thanks for fixing this!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH] drm/i915: 2 GiB of relocations ought to be enough for anybody*
On Tue, May 21, 2024 at 11:12:01AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> Kernel test robot reports i915 can hit a warn in kvmalloc_node which has
> a purpose of dissalowing crazy size kernel allocations. This was added in
> 7661809d493b ("mm: don't allow oversized kvmalloc() calls"):
>
>/* Don't even allow crazy sizes */
>if (WARN_ON_ONCE(size > INT_MAX))
>return NULL;
>
> This would be kind of okay since i915 at one point dropped the need for
> making a shadow copy of the relocation list, but then it got re-added in
> fd1500fcd442 ("Revert "drm/i915/gem: Drop relocation slowpath".") a year
> after Linus added the above warning.
>
> It is plausible that the issue was not seen until now because to trigger
> gem_exec_reloc test requires a combination of an relatively older
> generation hardware but with at least 8GiB of RAM installed. Probably even
> more depending on runtime checks.
>
> Lets cap what we allow userspace to pass in using the matching limit.
> There should be no issue for real userspace since we are talking about
> "crazy" number of relocations which have no practical purpose.
>
> *) Well IGT tests might get upset but they can be easily adjusted.
>
> Signed-off-by: Tvrtko Ursulin
Thanks for fixing this!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v10 0/5] Introduce mseal
On Tue, May 14, 2024 at 12:52:13PM -0700, Kees Cook wrote: > On Tue, May 14, 2024 at 10:46:46AM -0700, Andrew Morton wrote: > > On Mon, 15 Apr 2024 16:35:19 + jef...@chromium.org wrote: > > > > > This patchset proposes a new mseal() syscall for the Linux kernel. > > > > I have not moved this into mm-stable for a 6.10 merge. Mainly because > > of the total lack of Reviewed-by:s and Acked-by:s. > > Oh, I thought I had already reviewed it. FWIW, please consider it: > > Reviewed-by: Kees Cook > > > The code appears to be stable enough for a merge. > > Agreed. > > > It's awkward that we're in conference this week, but I ask people to > > give consideration to the desirability of moving mseal() into mainline > > sometime over the next week, please. > > Yes please. :) Is the plan still to land this for 6.10? With the testing it's had in -next and Liam's review, I think we're good to go? Thanks! -Kees -- Kees Cook
[PATCH] ext4: Use memtostr_pad() for s_volume_name
As with the other strings in struct ext4_super_block, s_volume_name is
not NUL terminated. The other strings were marked in commit 072ebb3bffe6
("ext4: add nonstring annotations to ext4.h"). Using strscpy() isn't
the right replacement for strncpy(); it should use memtostr_pad()
instead.
Reported-by: syzbot+50835f73143cc2905...@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/19f4c00619192...@google.com/
Fixes: 744a56389f73 ("ext4: replace deprecated strncpy with alternatives")
Signed-off-by: Kees Cook
---
Cc: "Theodore Ts'o"
Cc: Justin Stitt
Cc: Andreas Dilger
Cc: linux-e...@vger.kernel.org
---
fs/ext4/ext4.h | 2 +-
fs/ext4/ioctl.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 983dad8c07ec..efed7f09876d 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1347,7 +1347,7 @@ struct ext4_super_block {
/*60*/ __le32 s_feature_incompat; /* incompatible feature set */
__le32 s_feature_ro_compat;/* readonly-compatible feature set */
/*68*/ __u8s_uuid[16]; /* 128-bit uuid for volume */
-/*78*/ chars_volume_name[EXT4_LABEL_MAX]; /* volume name */
+/*78*/ chars_volume_name[EXT4_LABEL_MAX] __nonstring; /* volume name */
/*88*/ chars_last_mounted[64] __nonstring; /* directory where last mounted
*/
/*C8*/ __le32 s_algorithm_usage_bitmap; /* For compression */
/*
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index dab7acd49709..e8bf5972dd47 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1151,7 +1151,7 @@ static int ext4_ioctl_getlabel(struct ext4_sb_info *sbi,
char __user *user_label
BUILD_BUG_ON(EXT4_LABEL_MAX >= FSLABEL_MAX);
lock_buffer(sbi->s_sbh);
- strscpy_pad(label, sbi->s_es->s_volume_name);
+ memtostr_pad(label, sbi->s_es->s_volume_name);
unlock_buffer(sbi->s_sbh);
if (copy_to_user(user_label, label, sizeof(label)))
--
2.34.1
Re: [linux-next:master] [mm/slab] 7bd230a266: WARNING:at_mm/util.c:#kvmalloc_node_noprof
On Sun, May 19, 2024 at 07:06:45PM -0400, Kent Overstreet wrote:
> this looks like an i915 bug
Yeah, agreed.
> On Wed, May 15, 2024 at 10:41:19AM +0800, kernel test robot wrote:
[...]
> > [test failed on linux-next/master 6ba6c795dc73c22ce2c86006f17c4aa802db2a60]
[...]
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new
> > version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot
> > | Closes:
> > https://lore.kernel.org/oe-lkp/202405151008.6ddd1aaf-oliver.s...@intel.com
> >
> >
> > [ 940.101700][ T5353] [ cut here ]
> > [ 940.107107][ T5353] WARNING: CPU: 1 PID: 5353 at mm/util.c:649
> > kvmalloc_node_noprof (mm/util.c:649 (discriminator 1))
This is:
/* Don't even allow crazy sizes */
if (unlikely(size > INT_MAX)) {
WARN_ON_ONCE(!(flags & __GFP_NOWARN));
> > [ 940.307791][ T5353] Call Trace:
[...]
> > [ 940.351795][ T5353] eb_copy_relocations
> > (drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1685) i915
And this is:
const unsigned int nreloc = eb->exec[i].relocation_count;
...
size = nreloc * sizeof(*relocs);
relocs = kvmalloc_array(1, size, GFP_KERNEL);
So something isn't checking the "relocation_count" size that I assume is
coming in from the ioctl?
-Kees
--
Kees Cook
Re: [linux-next:master] [mm/slab] 7bd230a266: WARNING:at_mm/util.c:#kvmalloc_node_noprof
On Sun, May 19, 2024 at 07:06:45PM -0400, Kent Overstreet wrote:
> this looks like an i915 bug
Yeah, agreed.
> On Wed, May 15, 2024 at 10:41:19AM +0800, kernel test robot wrote:
[...]
> > [test failed on linux-next/master 6ba6c795dc73c22ce2c86006f17c4aa802db2a60]
[...]
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new
> > version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot
> > | Closes:
> > https://lore.kernel.org/oe-lkp/202405151008.6ddd1aaf-oliver.s...@intel.com
> >
> >
> > [ 940.101700][ T5353] [ cut here ]
> > [ 940.107107][ T5353] WARNING: CPU: 1 PID: 5353 at mm/util.c:649
> > kvmalloc_node_noprof (mm/util.c:649 (discriminator 1))
This is:
/* Don't even allow crazy sizes */
if (unlikely(size > INT_MAX)) {
WARN_ON_ONCE(!(flags & __GFP_NOWARN));
> > [ 940.307791][ T5353] Call Trace:
[...]
> > [ 940.351795][ T5353] eb_copy_relocations
> > (drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1685) i915
And this is:
const unsigned int nreloc = eb->exec[i].relocation_count;
...
size = nreloc * sizeof(*relocs);
relocs = kvmalloc_array(1, size, GFP_KERNEL);
So something isn't checking the "relocation_count" size that I assume is
coming in from the ioctl?
-Kees
--
Kees Cook
Re: [GIT PULL] bcachefs updates fro 6.10-rc1
On Sun, May 19, 2024 at 12:14:34PM -0400, Kent Overstreet wrote: > [...] > bcachefs changes for 6.10-rc1 > [...] > bcachefs: bch2_btree_path_to_text() Hi Kent, I've asked after this before[1], but there continues to be a lot of bcachefs development going on that is only visible when it appears in -next or during the merge window. I cannot find the above commit on any mailing list on lore.kernel.org[2]. The rules for -next are clear: patches _must_ appear on a list _somewhere_ before they land in -next (much less Linus's tree). The point is to get additional reviews, and to serve as a focal point for any discussions that pop up over a given change. Please adjust the bcachefs development workflow to address this. Anyway, in reference to the above commit, please scrub bcachefs of its %px format string uses. Neither it nor %p should be used[3][4] in new code. (Which is, again, something checkpatch.pl will warn about.) Thanks! -Kees [1] https://lore.kernel.org/all/202401101525.112E8234@keescook/ [2] https://lore.kernel.org/linux-bcachefs/?q=bch2_btree_path_to_text [3] https://docs.kernel.org/process/deprecated.html#p-format-specifier [4] https://lore.kernel.org/lkml/ca+55afwqed_d40g4mucssvrzzrfpujt74vc6pppb675hynx...@mail.gmail.com/ -- Kees Cook
[PATCH 0/2] exec: Add KUnit test for bprm_stack_limits()
Hi, This adds a first KUnit test to the core exec code. With the ability to manipulate userspace memory from KUnit coming[1], I wanted to at least get the KUnit framework in place in exec.c. Most of the coming tests will likely be to binfmt_elf.c, but still, this serves as a reasonable first step. -Kees [1] https://lore.kernel.org/linux-hardening/20240519190422.work.715-k...@kernel.org/ Kees Cook (2): exec: Add KUnit test for bprm_stack_limits() exec: Avoid pathological argc, envc, and bprm->p values MAINTAINERS | 2 + fs/Kconfig.binfmt | 8 +++ fs/exec.c | 24 +++- fs/exec_test.c| 137 ++ 4 files changed, 170 insertions(+), 1 deletion(-) create mode 100644 fs/exec_test.c -- 2.34.1
[PATCH 2/2] exec: Avoid pathological argc, envc, and bprm->p values
Make sure nothing goes wrong with the string counters or the bprm's
belief about the stack pointer. Add checks and matching self-tests.
For 32-bit validation, this was run under 32-bit UML:
$ tools/testing/kunit/kunit.py run --make_options SUBARCH=i386 exec
Signed-off-by: Kees Cook
---
Cc: Eric Biederman
Cc: Justin Stitt
Cc: Alexander Viro
Cc: Christian Brauner
Cc: Jan Kara
Cc: linux-fsde...@vger.kernel.org
Cc: linux...@kvack.org
---
fs/exec.c | 11 ++-
fs/exec_test.c | 26 +-
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 1d45e1a2d620..5dcdd115739e 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -503,6 +503,9 @@ static int bprm_stack_limits(struct linux_binprm *bprm)
* of argument strings even with small stacks
*/
limit = max_t(unsigned long, limit, ARG_MAX);
+ /* Reject totally pathological counts. */
+ if (bprm->argc < 0 || bprm->envc < 0)
+ return -E2BIG;
/*
* We must account for the size of all the argv and envp pointers to
* the argv and envp strings, since they will also take up space in
@@ -516,11 +519,17 @@ static int bprm_stack_limits(struct linux_binprm *bprm)
* argc can never be 0, to keep them from walking envp by accident.
* See do_execveat_common().
*/
- ptr_size = (max(bprm->argc, 1) + bprm->envc) * sizeof(void *);
+ if (check_add_overflow(max(bprm->argc, 1), bprm->envc, _size) ||
+ check_mul_overflow(ptr_size, sizeof(void *), _size))
+ return -E2BIG;
if (limit <= ptr_size)
return -E2BIG;
limit -= ptr_size;
+ /* Avoid a pathological bprm->p. */
+ if (bprm->p < limit)
+ return -E2BIG;
+
bprm->argmin = bprm->p - limit;
return 0;
}
diff --git a/fs/exec_test.c b/fs/exec_test.c
index 32a90c6f47e7..f2d4a80c861d 100644
--- a/fs/exec_test.c
+++ b/fs/exec_test.c
@@ -8,9 +8,32 @@ struct bprm_stack_limits_result {
};
static const struct bprm_stack_limits_result bprm_stack_limits_results[] = {
- /* Giant values produce -E2BIG */
+ /* Negative argc/envc counts produce -E2BIG */
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+ .argc = INT_MIN, .envc = INT_MIN }, .expected_rc = -E2BIG },
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+ .argc = 5, .envc = -1 }, .expected_rc = -E2BIG },
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+ .argc = -1, .envc = 10 }, .expected_rc = -E2BIG },
+ /* The max value of argc or envc is MAX_ARG_STRINGS. */
{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
.argc = INT_MAX, .envc = INT_MAX }, .expected_rc = -E2BIG },
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+ .argc = MAX_ARG_STRINGS, .envc = MAX_ARG_STRINGS }, .expected_rc =
-E2BIG },
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+ .argc = 0, .envc = MAX_ARG_STRINGS }, .expected_rc = -E2BIG },
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+ .argc = MAX_ARG_STRINGS, .envc = 0 }, .expected_rc = -E2BIG },
+ /*
+* On 32-bit system these argc and envc counts, while likely impossible
+* to represent within the associated TASK_SIZE, could overflow the
+* limit calculation, and bypass the ptr_size <= limit check.
+*/
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+ .argc = 0x2001, .envc = 0x2001 }, .expected_rc = -E2BIG },
+ /* Make sure a pathological bprm->p doesn't cause an overflow. */
+ { { .p = sizeof(void *), .rlim_stack.rlim_cur = ULONG_MAX,
+ .argc = 10, .envc = 10 }, .expected_rc = -E2BIG },
/*
* 0 rlim_stack will get raised to ARG_MAX. With 1 string pointer,
* we should see p - ARG_MAX + sizeof(void *).
@@ -88,6 +111,7 @@ static void exec_test_bprm_stack_limits(struct kunit *test)
/* Double-check the constants. */
KUNIT_EXPECT_EQ(test, _STK_LIM, SZ_8M);
KUNIT_EXPECT_EQ(test, ARG_MAX, 32 * SZ_4K);
+ KUNIT_EXPECT_EQ(test, MAX_ARG_STRINGS, 0x7FFF);
for (int i = 0; i < ARRAY_SIZE(bprm_stack_limits_results); i++) {
const struct bprm_stack_limits_result *result =
_stack_limits_results[i];
--
2.34.1
[PATCH 1/2] exec: Add KUnit test for bprm_stack_limits()
Since bprm_stack_limits() operates with very limited side-effects, add
it as the first exec.c KUnit test. Add to Kconfig and adjust MAINTAINERS
file to include it.
Tested on 64-bit UML:
$ tools/testing/kunit/kunit.py run exec
Signed-off-by: Kees Cook
---
Cc: Eric Biederman
Cc: Justin Stitt
Cc: Alexander Viro
Cc: Christian Brauner
Cc: Jan Kara
Cc: linux-fsde...@vger.kernel.org
Cc: linux...@kvack.org
---
MAINTAINERS | 2 +
fs/Kconfig.binfmt | 8
fs/exec.c | 13 ++
fs/exec_test.c| 113 ++
4 files changed, 136 insertions(+)
create mode 100644 fs/exec_test.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7c121493f43d..845165dbb756 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8039,7 +8039,9 @@ S:Supported
T: git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git
for-next/execve
F: Documentation/userspace-api/ELF.rst
F: fs/*binfmt_*.c
+F: fs/Kconfig.binfmt
F: fs/exec.c
+F: fs/exec_test.c
F: include/linux/binfmts.h
F: include/linux/elf.h
F: include/uapi/linux/binfmts.h
diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
index f5693164ca9a..58657f2d9719 100644
--- a/fs/Kconfig.binfmt
+++ b/fs/Kconfig.binfmt
@@ -176,4 +176,12 @@ config COREDUMP
certainly want to say Y here. Not necessary on systems that never
need debugging or only ever run flawless code.
+config EXEC_KUNIT_TEST
+ bool "Build execve tests" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ This builds the exec KUnit tests, which tests boundary conditions
+ of various aspects of the exec internals.
+
endmenu
diff --git a/fs/exec.c b/fs/exec.c
index b3c40fbb325f..1d45e1a2d620 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -475,6 +475,15 @@ static int count_strings_kernel(const char *const *argv)
return i;
}
+/*
+ * Calculate bprm->argmin from:
+ * - _STK_LIM
+ * - ARG_MAX
+ * - bprm->rlim_stack.rlim_cur
+ * - bprm->argc
+ * - bprm->envc
+ * - bprm->p
+ */
static int bprm_stack_limits(struct linux_binprm *bprm)
{
unsigned long limit, ptr_size;
@@ -2200,3 +2209,7 @@ static int __init init_fs_exec_sysctls(void)
fs_initcall(init_fs_exec_sysctls);
#endif /* CONFIG_SYSCTL */
+
+#ifdef CONFIG_EXEC_KUNIT_TEST
+#include "exec_test.c"
+#endif
diff --git a/fs/exec_test.c b/fs/exec_test.c
new file mode 100644
index ..32a90c6f47e7
--- /dev/null
+++ b/fs/exec_test.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include
+
+struct bprm_stack_limits_result {
+ struct linux_binprm bprm;
+ int expected_rc;
+ unsigned long expected_argmin;
+};
+
+static const struct bprm_stack_limits_result bprm_stack_limits_results[] = {
+ /* Giant values produce -E2BIG */
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+ .argc = INT_MAX, .envc = INT_MAX }, .expected_rc = -E2BIG },
+ /*
+* 0 rlim_stack will get raised to ARG_MAX. With 1 string pointer,
+* we should see p - ARG_MAX + sizeof(void *).
+*/
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+ .argc = 1, .envc = 0 }, .expected_argmin = ULONG_MAX - ARG_MAX +
sizeof(void *)},
+ /* Validate that argc is always raised to a minimum of 1. */
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+ .argc = 0, .envc = 0 }, .expected_argmin = ULONG_MAX - ARG_MAX +
sizeof(void *)},
+ /*
+* 0 rlim_stack will get raised to ARG_MAX. With pointers filling
ARG_MAX,
+* we should see -E2BIG. (Note argc is always raised to at least 1.)
+*/
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+ .argc = ARG_MAX / sizeof(void *), .envc = 0 }, .expected_rc =
-E2BIG },
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+ .argc = 0, .envc = ARG_MAX / sizeof(void *) - 1 }, .expected_rc =
-E2BIG },
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+ .argc = ARG_MAX / sizeof(void *) + 1, .envc = 0 }, .expected_rc =
-E2BIG },
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+ .argc = 0, .envc = ARG_MAX / sizeof(void *) }, .expected_rc =
-E2BIG },
+ /* And with one less, we see space for exactly 1 pointer. */
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+ .argc = (ARG_MAX / sizeof(void *)) - 1, .envc = 0 },
+ .expected_argmin = ULONG_MAX - sizeof(void *) },
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+ .argc = 0, .envc = (ARG_MAX / sizeof(void *)) - 2, },
+ .expected_argmin = ULONG_MAX - sizeof(void *) },
+ /* If we raise rlim_stack / 4 to exactly ARG_MAX, nothing changes. */
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = ARG_MAX * 4,
+ .argc = ARG_MAX / sizeof(void *), .envc = 0 }, .expected_rc =
-E2BIG },
+ { { .p = ULONG_MAX, .rlim_stack.rlim_cur = ARG_MA
[PATCH 2/2] usercopy: Convert test_user_copy to KUnit test
Convert the runtime tests of hardened usercopy to standard KUnit tests.
Co-developed-by: Vitor Massaru Iha
Signed-off-by: Vitor Massaru Iha
Link: https://lore.kernel.org/r/20200721174654.72132-1-vi...@massaru.org
Signed-off-by: Kees Cook
---
MAINTAINERS| 1 +
lib/Kconfig.debug | 21 +-
lib/Makefile | 2 +-
lib/{test_user_copy.c => usercopy_kunit.c} | 252 ++---
4 files changed, 133 insertions(+), 143 deletions(-)
rename lib/{test_user_copy.c => usercopy_kunit.c} (52%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 7c121493f43d..73995b807e5a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11761,6 +11761,7 @@ F: arch/*/configs/hardening.config
F: include/linux/overflow.h
F: include/linux/randomize_kstack.h
F: kernel/configs/hardening.config
+F: lib/usercopy_kunit.c
F: mm/usercopy.c
K: \b(add|choose)_random_kstack_offset\b
K: \b__check_(object_size|heap_object)\b
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c63a5fbf1f1c..fd974480aa45 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2460,18 +2460,6 @@ config TEST_VMALLOC
If unsure, say N.
-config TEST_USER_COPY
- tristate "Test user/kernel boundary protections"
- depends on m
- help
- This builds the "test_user_copy" module that runs sanity checks
- on the copy_to/from_user infrastructure, making sure basic
- user/kernel boundary testing is working. If it fails to load,
- a regression has been detected in the user/kernel memory boundary
- protections.
-
- If unsure, say N.
-
config TEST_BPF
tristate "Test BPF filter functionality"
depends on m && NET
@@ -2779,6 +2767,15 @@ config SIPHASH_KUNIT_TEST
This is intended to help people writing architecture-specific
optimized versions. If unsure, say N.
+config USERCOPY_KUNIT_TEST
+ tristate "KUnit Test for user/kernel boundary protections"
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ This builds the "usercopy_kunit" module that runs sanity checks
+ on the copy_to/from_user infrastructure, making sure basic
+ user/kernel boundary testing is working.
+
config TEST_UDELAY
tristate "udelay test driver"
help
diff --git a/lib/Makefile b/lib/Makefile
index ffc6b2341b45..6287bd6be5d7 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_LKM) += test_module.o
obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
obj-$(CONFIG_TEST_SORT) += test_sort.o
-obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
obj-$(CONFIG_TEST_DYNAMIC_DEBUG) += test_dynamic_debug.o
@@ -406,6 +405,7 @@ obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o
obj-$(CONFIG_STRCAT_KUNIT_TEST) += strcat_kunit.o
obj-$(CONFIG_STRSCPY_KUNIT_TEST) += strscpy_kunit.o
obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o
+obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o
obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
diff --git a/lib/test_user_copy.c b/lib/usercopy_kunit.c
similarity index 52%
rename from lib/test_user_copy.c
rename to lib/usercopy_kunit.c
index 5ff04d8fe971..515df08b3190 100644
--- a/lib/test_user_copy.c
+++ b/lib/usercopy_kunit.c
@@ -15,7 +15,7 @@
#include
#include
#include
-#include
+#include
/*
* Several 32-bit architectures support 64-bit {get,put}_user() calls.
@@ -31,11 +31,17 @@
# define TEST_U64
#endif
+struct usercopy_test_priv {
+ char *kmem;
+ char __user *umem;
+ size_t size;
+};
+
#define test(condition, msg, ...) \
({ \
int cond = (condition); \
if (cond) \
- pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
+ KUNIT_EXPECT_FALSE_MSG(test, cond, msg, ##__VA_ARGS__); \
cond; \
})
@@ -44,13 +50,16 @@ static bool is_zeroed(void *from, size_t size)
return memchr_inv(from, 0x0, size) == NULL;
}
-static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
+/* Test usage of check_nonzero_user(). */
+static void usercopy_test_check_nonzero_user(struct kunit *test)
{
- int ret = 0;
size_t start, end, i, zero_start, zero_end;
+ struct usercopy_test_priv *priv = test->priv;
+ char __user *umem = priv->umem;
+ char *kmem = priv->kmem;
+ size_t size = p
[PATCH 0/2] usercopy: Convert test_user_copy to KUnit test
Hi,
This builds on the proposal[1] from Mark and lets me convert the
existing usercopy selftest to KUnit. Besides adding this basic test to
the KUnit collection, it also opens the door for execve testing (which
depends on having a functional current->mm), and should provide the
basic infrastructure for adding Mark's much more complete usercopy tests.
-Kees
[1] https://lore.kernel.org/lkml/20230321122514.1743889-2-mark.rutl...@arm.com/
Kees Cook (2):
kunit: test: Add vm_mmap() allocation resource manager
usercopy: Convert test_user_copy to KUnit test
MAINTAINERS| 1 +
include/kunit/test.h | 17 ++
lib/Kconfig.debug | 21 +-
lib/Makefile | 2 +-
lib/kunit/test.c | 139 +++-
lib/{test_user_copy.c => usercopy_kunit.c} | 252 ++---
6 files changed, 288 insertions(+), 144 deletions(-)
rename lib/{test_user_copy.c => usercopy_kunit.c} (52%)
--
2.34.1
[PATCH 1/2] kunit: test: Add vm_mmap() allocation resource manager
For tests that need to allocate using vm_mmap() (e.g. usercopy and
execve), provide the interface to have the allocation tracked by KUnit
itself. This requires bringing up a placeholder userspace mm.
This combines my earlier attempt at this with Mark Rutland's version[1].
Link:
https://lore.kernel.org/lkml/20230321122514.1743889-2-mark.rutl...@arm.com/ [1]
Co-developed-by: Mark Rutland
Signed-off-by: Mark Rutland
Signed-off-by: Kees Cook
---
include/kunit/test.h | 17 ++
lib/kunit/test.c | 139 ++-
2 files changed, 155 insertions(+), 1 deletion(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 61637ef32302..8c3835a6f282 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -478,6 +478,23 @@ static inline void *kunit_kcalloc(struct kunit *test,
size_t n, size_t size, gfp
return kunit_kmalloc_array(test, n, size, gfp | __GFP_ZERO);
}
+/**
+ * kunit_vm_mmap() - Allocate KUnit-tracked vm_mmap() area
+ * @test: The test context object.
+ * @file: struct file pointer to map from, if any
+ * @addr: desired address, if any
+ * @len: how many bytes to allocate
+ * @prot: mmap PROT_* bits
+ * @flag: mmap flags
+ * @offset: offset into @file to start mapping from.
+ *
+ * See vm_mmap() for more information.
+ */
+unsigned long kunit_vm_mmap(struct kunit *test, struct file *file,
+ unsigned long addr, unsigned long len,
+ unsigned long prot, unsigned long flag,
+ unsigned long offset);
+
void kunit_cleanup(struct kunit *test);
void __printf(2, 3) kunit_log_append(struct string_stream *log, const char
*fmt, ...);
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 1d1475578515..09194dbffb63 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -11,13 +11,14 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
#include
#include
#include
-#include
#include "debugfs.h"
#include "device-impl.h"
@@ -871,6 +872,142 @@ void kunit_kfree(struct kunit *test, const void *ptr)
}
EXPORT_SYMBOL_GPL(kunit_kfree);
+struct kunit_vm_mmap_resource {
+ unsigned long addr;
+ size_t size;
+};
+
+/* vm_mmap() arguments */
+struct kunit_vm_mmap_params {
+ struct file *file;
+ unsigned long addr;
+ unsigned long len;
+ unsigned long prot;
+ unsigned long flag;
+ unsigned long offset;
+};
+
+/*
+ * Arbitrarily chosen user address for the base allocation.
+ */
+#define UBUF_ADDR_BASE SZ_2M
+
+/* Create and attach a new mm if it doesn't already exist. */
+static int kunit_attach_mm(void)
+{
+ struct vm_area_struct *vma;
+ struct mm_struct *mm;
+
+ if (current->mm)
+ return 0;
+
+ mm = mm_alloc();
+ if (!mm)
+ return -ENOMEM;
+
+ if (mmap_write_lock_killable(mm))
+ goto out_free;
+
+ /* Define the task size. */
+ mm->task_size = TASK_SIZE;
+
+ /* Prepare the base VMA. */
+ vma = vm_area_alloc(mm);
+ if (!vma)
+ goto out_unlock;
+
+ vma_set_anonymous(vma);
+ vma->vm_start = UBUF_ADDR_BASE;
+ vma->vm_end = UBUF_ADDR_BASE + PAGE_SIZE;
+ vm_flags_init(vma, VM_READ | VM_MAYREAD | VM_WRITE | VM_MAYWRITE);
+ vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
+
+ if (insert_vm_struct(mm, vma))
+ goto out_free_vma;
+
+ mmap_write_unlock(mm);
+
+ /* Make sure we can allocate new VMAs. */
+ arch_pick_mmap_layout(mm, >signal->rlim[RLIMIT_STACK]);
+
+ /* Attach the mm. It will be cleaned up when the process dies. */
+ kthread_use_mm(mm);
+
+ return 0;
+
+out_free_vma:
+ vm_area_free(vma);
+out_unlock:
+ mmap_write_unlock(mm);
+out_free:
+ mmput(mm);
+ return -ENOMEM;
+}
+
+static int kunit_vm_mmap_init(struct kunit_resource *res, void *context)
+{
+ struct kunit_vm_mmap_params *p = context;
+ struct kunit_vm_mmap_resource vres;
+ int ret;
+
+ ret = kunit_attach_mm();
+ if (ret)
+ return ret;
+
+ vres.size = p->len;
+ vres.addr = vm_mmap(p->file, p->addr, p->len, p->prot, p->flag,
p->offset);
+ if (!vres.addr)
+ return -ENOMEM;
+ res->data = kmemdup(, sizeof(vres), GFP_KERNEL);
+ if (!res->data) {
+ vm_munmap(vres.addr, vres.size);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void kunit_vm_mmap_free(struct kunit_resource *res)
+{
+ struct kunit_vm_mmap_resource *vres = res->data;
+
+ /*
+* Since this is executed from the test monitoring process,
+* the test's mm has already been torn down. We don't need
+* to run vm_munmap(vres->addr, vres->size), only clean up
+* the vres.
+*/
+
+
Re: [PATCH] efi: pstore: Return proper errors on UEFI failures
On Sun, May 19, 2024 at 01:33:53PM -0300, Guilherme G. Piccoli wrote: > Right now efi-pstore either returns 0 (success) or -EIO; but we > do have a function to convert UEFI errors in different standard > error codes, helping to narrow down potential issues more accurately. > > So, let's use this helper here. > > Signed-off-by: Guilherme G. Piccoli Ah yeah, good idea! Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] selftests: rtc: rtctest: Do not open-code TEST_HARNESS_MAIN
On Sat, May 18, 2024 at 10:23:54PM +0200, Alexandre Belloni wrote: > On 17/05/2024 17:16:58-0700, Kees Cook wrote: > > Argument processing is specific to the test harness code. Any optional > > information needs to be passed via environment variables. Move alternate > > path to the RTC_DEV environment variable. Also do not open-code > > TEST_HARNESS_MAIN because its definition may change. > > Th main issue doing that is that this breaks the main use case of > rtctest as /dev/rtc1 is usually the main target for those tests. Having > the RTC_DEV environment variable only documented n this commit message > is definitively not enough, I'm going to have to handle zillion of > complaints that this is not working anymore. Hm, maybe switch the default to /dev/rtc1? Maybe there's a better way to integrate arguments into a test runner. Right now the core harness code is doing the argument parsing... -- Kees Cook
Re: [PATCH v2] drm/nouveau/nvif: Avoid build error due to potential integer overflows
On Sat, May 18, 2024 at 11:29:23AM -0700, Guenter Roeck wrote:
> Trying to build parisc:allmodconfig with gcc 12.x or later results
> in the following build error.
>
> drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_mthd':
> drivers/gpu/drm/nouveau/nvif/object.c:161:9: error:
> 'memcpy' accessing 4294967264 or more bytes at offsets 0 and 32
> overlaps 6442450881 bytes at offset -2147483617 [-Werror=restrict]
> 161 | memcpy(data, args->mthd.data, size);
> | ^~~
> drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_ctor':
> drivers/gpu/drm/nouveau/nvif/object.c:298:17: error:
> 'memcpy' accessing 4294967240 or more bytes at offsets 0 and 56
> overlaps 6442450833 bytes at offset -2147483593 [-Werror=restrict]
> 298 | memcpy(data, args->new.data, size);
>
> gcc assumes that 'sizeof(*args) + size' can overflow, which would result
> in the problem.
>
> The problem is not new, only it is now no longer a warning but an error
> since W=1 has been enabled for the drm subsystem and since Werror is
> enabled for test builds.
>
> Rearrange arithmetic and use check_add_overflow() for validating the
> allocation size to avoid the overflow.
>
> Fixes: a61ddb4393ad ("drm: enable (most) W=1 warnings by default across the
> subsystem")
> Cc: Javier Martinez Canillas
> Cc: Jani Nikula
> Cc: Thomas Zimmermann
> Cc: Danilo Krummrich
> Cc: Maxime Ripard
> Cc: Kees Cook
> Cc: Christophe JAILLET
> Signed-off-by: Guenter Roeck
Yeah, looks good to me. Thanks!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v2] drm/nouveau/nvif: Avoid build error due to potential integer overflows
On Sat, May 18, 2024 at 11:29:23AM -0700, Guenter Roeck wrote:
> Trying to build parisc:allmodconfig with gcc 12.x or later results
> in the following build error.
>
> drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_mthd':
> drivers/gpu/drm/nouveau/nvif/object.c:161:9: error:
> 'memcpy' accessing 4294967264 or more bytes at offsets 0 and 32
> overlaps 6442450881 bytes at offset -2147483617 [-Werror=restrict]
> 161 | memcpy(data, args->mthd.data, size);
> | ^~~
> drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_ctor':
> drivers/gpu/drm/nouveau/nvif/object.c:298:17: error:
> 'memcpy' accessing 4294967240 or more bytes at offsets 0 and 56
> overlaps 6442450833 bytes at offset -2147483593 [-Werror=restrict]
> 298 | memcpy(data, args->new.data, size);
>
> gcc assumes that 'sizeof(*args) + size' can overflow, which would result
> in the problem.
>
> The problem is not new, only it is now no longer a warning but an error
> since W=1 has been enabled for the drm subsystem and since Werror is
> enabled for test builds.
>
> Rearrange arithmetic and use check_add_overflow() for validating the
> allocation size to avoid the overflow.
>
> Fixes: a61ddb4393ad ("drm: enable (most) W=1 warnings by default across the
> subsystem")
> Cc: Javier Martinez Canillas
> Cc: Jani Nikula
> Cc: Thomas Zimmermann
> Cc: Danilo Krummrich
> Cc: Maxime Ripard
> Cc: Kees Cook
> Cc: Christophe JAILLET
> Signed-off-by: Guenter Roeck
Yeah, looks good to me. Thanks!
Reviewed-by: Kees Cook
--
Kees Cook
[PATCH] kunit/fortify: Fix memcmp() test to be amplitude agnostic
When memcmp() returns a non-zero value, only the signed bit has any meaning. The actual value may differ between implementations. Reported-by: Nathan Chancellor Closes: https://github.com/ClangBuiltLinux/linux/issues/2025 Tested-by: Nathan Chancellor Signed-off-by: Kees Cook --- Cc: linux-hardening@vger.kernel.org --- lib/fortify_kunit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c index d2377e00caab..39da5b3bc649 100644 --- a/lib/fortify_kunit.c +++ b/lib/fortify_kunit.c @@ -990,7 +990,7 @@ static void fortify_test_memcmp(struct kunit *test) KUNIT_ASSERT_EQ(test, memcmp(one, two, one_len), 0); KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0); /* Still in bounds, but no longer matching. */ - KUNIT_ASSERT_EQ(test, memcmp(one, two, one_len + 1), -32); + KUNIT_ASSERT_LT(test, memcmp(one, two, one_len + 1), 0); KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0); /* Catch too-large ranges. */ -- 2.34.1
Re: [PATCH] dma-buf/fence-array: Add flex array to struct dma_fence_array
On Sat, May 18, 2024 at 07:47:02PM +0200, Christophe JAILLET wrote: > This is an effort to get rid of all multiplications from allocation > functions in order to prevent integer overflows [1][2]. > > The "struct dma_fence_array" can be refactored to add a flex array in order > to have the "callback structures allocated behind the array" be more > explicit. > > Do so: >- makes the code more readable and safer. >- allows using __counted_by() for additional checks >- avoids some pointer arithmetic in dma_fence_array_enable_signaling() > > 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: Christophe JAILLET Yes please! :) Reviewed-by: Kees Cook -- Kees Cook
Re: [WARNING] memcpy: detected field-spanning write (size 1005) of single field "_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3173 (size 320)
On Sat, May 18, 2024 at 11:29:39AM +0200, Stanislaw Gruszka wrote:
> Hi
>
> On Fri, Apr 12, 2024 at 07:48:39PM +0200, Xose Vazquez Perez wrote:
> > Hi,
> >
> > In Fedora kernel 6.8.5-301.fc40.x86_64, dmesg shows:
> >
> > [ device: 03:00.0 Network controller [0280]: Intel Corporation PRO/Wireless
> > 4965 AG or AGN [Kedron] Network Connection [8086:4230] (rev 61) ]
> >
> > Thanks.
> >
> > [ 53.407607] [ cut here ]
> > [ 53.407622] memcpy: detected field-spanning write (size 1005) of single
> > field "_cmd->cmd.payload" at
> > drivers/net/wireless/intel/iwlegacy/common.c:3173 (size 320)
> > [ 53.407721] WARNING: CPU: 1 PID: 1632 at
> > drivers/net/wireless/intel/iwlegacy/common.c:3173
> > il_enqueue_hcmd+0x477/0x560 [iwlegacy]
>
> For CMD_SIZE_HUGE we have allocated 4k, so we do not do anything wrong.
> Except maybe code is convoluted, since we use same structure for
> huge and small il_device_cmd allocations.
>
> But I'm thinking how to fix this fortify warning without refactoring and
> some extra runtime cost ...
>
> Xose, could you test below patch? I did not tested it, but I think
> it should make this particular warning gone and does not break
> anything. But maybe it will trigger some others fortify warnings.
tl;dr: the proposed patch should work. Refactoring will still be needed
in the future. :)
Long version:
struct il_device_cmd {
struct il_cmd_header hdr; /* uCode API */
union {
u32 flags;
u8 val8;
u16 val16;
u32 val32;
struct il_tx_cmd tx;
u8 payload[DEF_CMD_PAYLOAD_SIZE];
} __packed cmd;
} __packed;
struct il_cmd_header {
...
/* command or response/notification data follows immediately */
u8 data[];
} __packed;
Yes, the proposed fix will silence the warning, but this struct is
certainly on Gustavo's list to fix for "flexible arrays not-at-end"
warnings[1].
This memcpy() is the perfect example of why we need to refactor these
kinds of structs: the object size is ambiguous for the compiler. It
could be as little as sizeof(struct il_device_cmd), but it could larger,
because of the "hdr" member. Right now, it depends on how we happen to
address it (as your change is doing).
Regardless, thanks for tracking this down and fixing it!
-Kees
[1] https://lore.kernel.org/lkml/ZgsAFhl90kecIR00@neat/
>
> Regards
> Stanislaw
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/common.c
> b/drivers/net/wireless/intel/iwlegacy/common.c
> index 9d33a66a49b5..c4ccc5df6419 100644
> --- a/drivers/net/wireless/intel/iwlegacy/common.c
> +++ b/drivers/net/wireless/intel/iwlegacy/common.c
> @@ -3170,7 +3170,7 @@ il_enqueue_hcmd(struct il_priv *il, struct il_host_cmd
> *cmd)
> out_meta->callback = cmd->callback;
>
> out_cmd->hdr.cmd = cmd->id;
> - memcpy(_cmd->cmd.payload, cmd->data, cmd->len);
> + memcpy(_cmd->hdr.data, cmd->data, cmd->len);
>
> /* At this point, the out_cmd now has all of the incoming cmd
>* information */
--
Kees Cook
Re: [PATCH] drm/nouveau/nvif: Avoid build error due to potential integer overflows
On Sat, May 18, 2024 at 06:54:36PM +0200, Christophe JAILLET wrote:
> (adding linux-harden...@vger.kernel.org)
>
>
> Le 18/05/2024 à 16:37, Guenter Roeck a écrit :
> > Trying to build parisc:allmodconfig with gcc 12.x or later results
> > in the following build error.
> >
> > drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_mthd':
> > drivers/gpu/drm/nouveau/nvif/object.c:161:9: error:
> > 'memcpy' accessing 4294967264 or more bytes at offsets 0 and 32
> > overlaps 6442450881 bytes at offset -2147483617 [-Werror=restrict]
> >161 | memcpy(data, args->mthd.data, size);
> >| ^~~
> > drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_ctor':
> > drivers/gpu/drm/nouveau/nvif/object.c:298:17: error:
> > 'memcpy' accessing 4294967240 or more bytes at offsets 0 and 56
> > overlaps 6442450833 bytes at offset -2147483593 [-Werror=restrict]
> >298 | memcpy(data, args->new.data, size);
> >
> > gcc assumes that 'sizeof(*args) + size' can overflow, which would result
> > in the problem.
> >
> > The problem is not new, only it is now no longer a warning but an error
> > since W=1
> > has been enabled for the drm subsystem and since Werror is enabled for test
> > builds.
> >
> > Rearrange arithmetic and add extra size checks to avoid the overflow.
> >
> > Fixes: a61ddb4393ad ("drm: enable (most) W=1 warnings by default across the
> > subsystem")
> > Cc: Javier Martinez Canillas
> >
> > Cc: Jani Nikula
> > Cc: Thomas Zimmermann
> > Cc: Danilo Krummrich
> > Cc: Maxime Ripard
> > Signed-off-by: Guenter Roeck
> > ---
> > checkpatch complains about the line length in the description and the
> > (pre-existing)
> > assignlemts in if conditions, but I did not want to split lines in the
> > description
> > or rearrange the code further.
> >
> > I don't know why I only see the problem with parisc builds (at least so
> > far).
> >
> > drivers/gpu/drm/nouveau/nvif/object.c | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nvif/object.c
> > b/drivers/gpu/drm/nouveau/nvif/object.c
> > index 4d1aaee8fe15..baf623a48874 100644
> > --- a/drivers/gpu/drm/nouveau/nvif/object.c
> > +++ b/drivers/gpu/drm/nouveau/nvif/object.c
> > @@ -145,8 +145,9 @@ nvif_object_mthd(struct nvif_object *object, u32 mthd,
> > void *data, u32 size)
> > u8 stack[128];
> > int ret;
> > - if (sizeof(*args) + size > sizeof(stack)) {
> > - if (!(args = kmalloc(sizeof(*args) + size, GFP_KERNEL)))
> > + if (size > sizeof(stack) - sizeof(*args)) {
> > + if (size > INT_MAX ||
> > + !(args = kmalloc(sizeof(*args) + size, GFP_KERNEL)))
>
> Hi,
>
> Would it be cleaner or better to use size_add(sizeof(*args), size)?
I think the INT_MAX test is actually better in this case because
nvif_object_ioctl()'s size argument is u32:
ret = nvif_object_ioctl(object, args, sizeof(*args) + size, NULL);
So that could wrap around, even though the allocation may not.
Better yet, since "sizeof(*args) + size" is repeated 3 times in the
function, I'd recommend:
...
u32 args_size;
if (check_add_overflow(sizeof(*args), size, _size))
return -ENOMEM;
if (args_size > sizeof(stack)) {
if (!(args = kmalloc(args_size, GFP_KERNEL)))
return -ENOMEM;
} else {
args = (void *)stack;
}
...
ret = nvif_object_ioctl(object, args, args_size, NULL);
This will catch the u32 overflow to nvif_object_ioctl(), catch an
allocation underflow on 32-bits systems, and make the code more
readable. :)
-Kees
--
Kees Cook
Re: [PATCH] drm/nouveau/nvif: Avoid build error due to potential integer overflows
On Sat, May 18, 2024 at 06:54:36PM +0200, Christophe JAILLET wrote:
> (adding linux-harden...@vger.kernel.org)
>
>
> Le 18/05/2024 à 16:37, Guenter Roeck a écrit :
> > Trying to build parisc:allmodconfig with gcc 12.x or later results
> > in the following build error.
> >
> > drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_mthd':
> > drivers/gpu/drm/nouveau/nvif/object.c:161:9: error:
> > 'memcpy' accessing 4294967264 or more bytes at offsets 0 and 32
> > overlaps 6442450881 bytes at offset -2147483617 [-Werror=restrict]
> >161 | memcpy(data, args->mthd.data, size);
> >| ^~~
> > drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_ctor':
> > drivers/gpu/drm/nouveau/nvif/object.c:298:17: error:
> > 'memcpy' accessing 4294967240 or more bytes at offsets 0 and 56
> > overlaps 6442450833 bytes at offset -2147483593 [-Werror=restrict]
> >298 | memcpy(data, args->new.data, size);
> >
> > gcc assumes that 'sizeof(*args) + size' can overflow, which would result
> > in the problem.
> >
> > The problem is not new, only it is now no longer a warning but an error
> > since W=1
> > has been enabled for the drm subsystem and since Werror is enabled for test
> > builds.
> >
> > Rearrange arithmetic and add extra size checks to avoid the overflow.
> >
> > Fixes: a61ddb4393ad ("drm: enable (most) W=1 warnings by default across the
> > subsystem")
> > Cc: Javier Martinez Canillas
> >
> > Cc: Jani Nikula
> > Cc: Thomas Zimmermann
> > Cc: Danilo Krummrich
> > Cc: Maxime Ripard
> > Signed-off-by: Guenter Roeck
> > ---
> > checkpatch complains about the line length in the description and the
> > (pre-existing)
> > assignlemts in if conditions, but I did not want to split lines in the
> > description
> > or rearrange the code further.
> >
> > I don't know why I only see the problem with parisc builds (at least so
> > far).
> >
> > drivers/gpu/drm/nouveau/nvif/object.c | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nvif/object.c
> > b/drivers/gpu/drm/nouveau/nvif/object.c
> > index 4d1aaee8fe15..baf623a48874 100644
> > --- a/drivers/gpu/drm/nouveau/nvif/object.c
> > +++ b/drivers/gpu/drm/nouveau/nvif/object.c
> > @@ -145,8 +145,9 @@ nvif_object_mthd(struct nvif_object *object, u32 mthd,
> > void *data, u32 size)
> > u8 stack[128];
> > int ret;
> > - if (sizeof(*args) + size > sizeof(stack)) {
> > - if (!(args = kmalloc(sizeof(*args) + size, GFP_KERNEL)))
> > + if (size > sizeof(stack) - sizeof(*args)) {
> > + if (size > INT_MAX ||
> > + !(args = kmalloc(sizeof(*args) + size, GFP_KERNEL)))
>
> Hi,
>
> Would it be cleaner or better to use size_add(sizeof(*args), size)?
I think the INT_MAX test is actually better in this case because
nvif_object_ioctl()'s size argument is u32:
ret = nvif_object_ioctl(object, args, sizeof(*args) + size, NULL);
So that could wrap around, even though the allocation may not.
Better yet, since "sizeof(*args) + size" is repeated 3 times in the
function, I'd recommend:
...
u32 args_size;
if (check_add_overflow(sizeof(*args), size, _size))
return -ENOMEM;
if (args_size > sizeof(stack)) {
if (!(args = kmalloc(args_size, GFP_KERNEL)))
return -ENOMEM;
} else {
args = (void *)stack;
}
...
ret = nvif_object_ioctl(object, args, args_size, NULL);
This will catch the u32 overflow to nvif_object_ioctl(), catch an
allocation underflow on 32-bits systems, and make the code more
readable. :)
-Kees
--
Kees Cook
Re: [PATCH 1/2] selftests: harness: remove unneeded __constructor_order_last()
On Sat, May 18, 2024 at 12:29:00PM +0900, Masahiro Yamada wrote: > It will be set to "true" eventually, > but __LIST_APPEND() still sees "false" > on backward-order systems. Ah, yes -- you are right. I looked through the commit history (I had to go back to when the seccomp test, and the harness, was out of tree). There was a time when the logic happened during the list walking, rather than during list _creation_. I was remembering the former. So, yes, let's make this change. As you say, it also solves for defining TEST_HARNESS_MAIN before the tests. Thank you! I'd still like to replace all the open-coded TEST_HARNESS_MAIN calls, though. -- Kees Cook
[PATCH] selftests: drivers/s390x: Use SKIP() during FIXTURE_SETUP
Instead of mixing selftest harness and ksft helpers, perform SKIP
testing from the FIXTURE_SETUPs. This also means TEST_HARNESS_MAIN does
not need to be open-coded.
Signed-off-by: Kees Cook
---
Cc: Christian Borntraeger
Cc: Janosch Frank
Cc: Claudio Imbrenda
Cc: David Hildenbrand
Cc: Shuah Khan
Cc: Masahiro Yamada
Cc: k...@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
---
I wasn't able to build or run test this, since it's very s390 specific, it
seems?
---
.../drivers/s390x/uvdevice/test_uvdevice.c| 32 ---
1 file changed, 13 insertions(+), 19 deletions(-)
diff --git a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
index ea0cdc37b44f..9622fac42597 100644
--- a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
+++ b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
@@ -18,6 +18,16 @@
#define UV_PATH "/dev/uv"
#define BUFFER_SIZE 0x200
+
+#define open_uv() \
+do { \
+ self->uv_fd = open(UV_PATH, O_ACCMODE); \
+ if (self->uv_fd < 0) \
+ SKIP(return, "No uv-device or cannot access " UV_PATH "\n" \
+"Enable CONFIG_S390_UV_UAPI and check the access
rights on " \
+UV_PATH ".\n"); \
+} while (0)
+
FIXTURE(uvio_fixture) {
int uv_fd;
struct uvio_ioctl_cb uvio_ioctl;
@@ -37,7 +47,7 @@ FIXTURE_VARIANT_ADD(uvio_fixture, att) {
FIXTURE_SETUP(uvio_fixture)
{
- self->uv_fd = open(UV_PATH, O_ACCMODE);
+ open_uv();
self->uvio_ioctl.argument_addr = (__u64)self->buffer;
self->uvio_ioctl.argument_len = variant->arg_size;
@@ -159,7 +169,7 @@ FIXTURE(attest_fixture) {
FIXTURE_SETUP(attest_fixture)
{
- self->uv_fd = open(UV_PATH, O_ACCMODE);
+ open_uv();
self->uvio_ioctl.argument_addr = (__u64)>uvio_attest;
self->uvio_ioctl.argument_len = sizeof(self->uvio_attest);
@@ -257,20 +267,4 @@ TEST_F(attest_fixture, att_inval_addr)
att_inval_addr_test(>uvio_attest.meas_addr, _metadata, self);
}
-static void __attribute__((constructor)) __constructor_order_last(void)
-{
- if (!__constructor_order)
- __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
-}
-
-int main(int argc, char **argv)
-{
- int fd = open(UV_PATH, O_ACCMODE);
-
- if (fd < 0)
- ksft_exit_skip("No uv-device or cannot access " UV_PATH "\n"
- "Enable CONFIG_S390_UV_UAPI and check the access
rights on "
- UV_PATH ".\n");
- close(fd);
- return test_harness_run(argc, argv);
-}
+TEST_HARNESS_MAIN
--
2.34.1
[PATCH] selftests: hid: Do not open-code TEST_HARNESS_MAIN
Avoid open-coding TEST_HARNESS_MAIN. (It might change, for example.)
Signed-off-by: Kees Cook
---
Cc: Jiri Kosina
Cc: Benjamin Tissoires
Cc: Shuah Khan
Cc: Masahiro Yamada
Cc: linux-in...@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
---
tools/testing/selftests/hid/hid_bpf.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/hid/hid_bpf.c
b/tools/testing/selftests/hid/hid_bpf.c
index f825623e3edc..943fa62a4f78 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -961,17 +961,11 @@ static int libbpf_print_fn(enum libbpf_print_level level,
return 0;
}
-static void __attribute__((constructor)) __constructor_order_last(void)
-{
- if (!__constructor_order)
- __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
-}
-
-int main(int argc, char **argv)
+static void __attribute__((constructor)) __one_time_init(void)
{
/* Use libbpf 1.0 API mode */
libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
libbpf_set_print(libbpf_print_fn);
-
- return test_harness_run(argc, argv);
}
+
+TEST_HARNESS_MAIN
--
2.34.1
[PATCH] selftests: rtc: rtctest: Do not open-code TEST_HARNESS_MAIN
Argument processing is specific to the test harness code. Any optional
information needs to be passed via environment variables. Move alternate
path to the RTC_DEV environment variable. Also do not open-code
TEST_HARNESS_MAIN because its definition may change.
Additionally, setup checking can be done in the FIXTURE_SETUP(). With
this adjustment, also improve the error reporting when the device cannot
be opened.
Signed-off-by: Kees Cook
---
Cc: Alexandre Belloni
Cc: Shuah Khan
Cc: Masahiro Yamada
Cc: linux-...@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
---
tools/testing/selftests/rtc/Makefile | 2 +-
tools/testing/selftests/rtc/rtctest.c | 66 +--
2 files changed, 13 insertions(+), 55 deletions(-)
diff --git a/tools/testing/selftests/rtc/Makefile
b/tools/testing/selftests/rtc/Makefile
index 55198ecc04db..654f9d58da3c 100644
--- a/tools/testing/selftests/rtc/Makefile
+++ b/tools/testing/selftests/rtc/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
-CFLAGS += -O3 -Wl,-no-as-needed -Wall
+CFLAGS += -O3 -Wl,-no-as-needed -Wall $(KHDR_INCLUDES)
LDLIBS += -lrt -lpthread -lm
TEST_GEN_PROGS = rtctest
diff --git a/tools/testing/selftests/rtc/rtctest.c
b/tools/testing/selftests/rtc/rtctest.c
index 63ce02d1d5cc..41cfefcc20e1 100644
--- a/tools/testing/selftests/rtc/rtctest.c
+++ b/tools/testing/selftests/rtc/rtctest.c
@@ -30,7 +30,18 @@ FIXTURE(rtc) {
};
FIXTURE_SETUP(rtc) {
+ char *alternate = getenv("RTC_DEV");
+
+ if (alternate)
+ rtc_file = alternate;
+
self->fd = open(rtc_file, O_RDONLY);
+
+ if (self->fd == -1 && errno == ENOENT)
+ SKIP(return, "Skipping test since %s does not exist", rtc_file);
+ EXPECT_NE(-1, self->fd) {
+ TH_LOG("%s: %s\n", rtc_file, strerror(errno));
+ }
}
FIXTURE_TEARDOWN(rtc) {
@@ -41,10 +52,6 @@ TEST_F(rtc, date_read) {
int rc;
struct rtc_time rtc_tm;
- if (self->fd == -1 && errno == ENOENT)
- SKIP(return, "Skipping test since %s does not exist", rtc_file);
- ASSERT_NE(-1, self->fd);
-
/* Read the RTC time/date */
rc = ioctl(self->fd, RTC_RD_TIME, _tm);
ASSERT_NE(-1, rc);
@@ -88,10 +95,6 @@ TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC +
2) {
struct rtc_time rtc_tm;
time_t start_rtc_read, prev_rtc_read;
- if (self->fd == -1 && errno == ENOENT)
- SKIP(return, "Skipping test since %s does not exist", rtc_file);
- ASSERT_NE(-1, self->fd);
-
TH_LOG("Continuously reading RTC time for %ds (with %dms breaks after
every read).",
READ_LOOP_DURATION_SEC, READ_LOOP_SLEEP_MS);
@@ -126,10 +129,6 @@ TEST_F_TIMEOUT(rtc, uie_read, NUM_UIE + 2) {
int i, rc, irq = 0;
unsigned long data;
- if (self->fd == -1 && errno == ENOENT)
- SKIP(return, "Skipping test since %s does not exist", rtc_file);
- ASSERT_NE(-1, self->fd);
-
/* Turn on update interrupts */
rc = ioctl(self->fd, RTC_UIE_ON, 0);
if (rc == -1) {
@@ -155,10 +154,6 @@ TEST_F(rtc, uie_select) {
int i, rc, irq = 0;
unsigned long data;
- if (self->fd == -1 && errno == ENOENT)
- SKIP(return, "Skipping test since %s does not exist", rtc_file);
- ASSERT_NE(-1, self->fd);
-
/* Turn on update interrupts */
rc = ioctl(self->fd, RTC_UIE_ON, 0);
if (rc == -1) {
@@ -198,10 +193,6 @@ TEST_F(rtc, alarm_alm_set) {
time_t secs, new;
int rc;
- if (self->fd == -1 && errno == ENOENT)
- SKIP(return, "Skipping test since %s does not exist", rtc_file);
- ASSERT_NE(-1, self->fd);
-
rc = ioctl(self->fd, RTC_RD_TIME, );
ASSERT_NE(-1, rc);
@@ -256,10 +247,6 @@ TEST_F(rtc, alarm_wkalm_set) {
time_t secs, new;
int rc;
- if (self->fd == -1 && errno == ENOENT)
- SKIP(return, "Skipping test since %s does not exist", rtc_file);
- ASSERT_NE(-1, self->fd);
-
rc = ioctl(self->fd, RTC_RD_TIME, );
ASSERT_NE(-1, rc);
@@ -308,10 +295,6 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) {
time_t secs, new;
int rc;
- if (self->fd == -1 && errno == ENOENT)
- SKIP(return, "Skipping test since %s does not exist", rtc_file);
- ASSERT_NE(-1, self->fd);
-
rc = ioctl(self->fd, RTC_RD_TIME, );
ASSERT_NE(-1, rc);
@@ -366,10 +349,6 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) {
time_t secs, new;
int rc;
- if (self->fd == -1 && errno == ENOENT)
- SKIP(return, "Skipping test since
Re: [PATCH 1/2] selftests: harness: remove unneeded __constructor_order_last()
On Fri, May 17, 2024 at 08:45:05PM +0900, Masahiro Yamada wrote:
> __constructor_order_last() is unneeded.
>
> If __constructor_order_last() is not called on reverse-order systems,
> __constructor_order will remain 0 instead of being set to
> _CONSTRUCTOR_ORDER_BACKWARD (= -1).
>
> __LIST_APPEND() will still take the 'else' branch, so there is no
> difference in the behavior.
>
> Signed-off-by: Masahiro Yamada
> ---
>
> .../selftests/drivers/s390x/uvdevice/test_uvdevice.c | 6 --
> tools/testing/selftests/hid/hid_bpf.c | 6 --
> tools/testing/selftests/kselftest_harness.h| 10 +-
> tools/testing/selftests/rtc/rtctest.c | 7 ---
> 4 files changed, 1 insertion(+), 28 deletions(-)
>
> diff --git a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
> b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
> index ea0cdc37b44f..7ee7492138c6 100644
> --- a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
> +++ b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c
> @@ -257,12 +257,6 @@ TEST_F(attest_fixture, att_inval_addr)
> att_inval_addr_test(>uvio_attest.meas_addr, _metadata, self);
> }
>
> -static void __attribute__((constructor)) __constructor_order_last(void)
> -{
> - if (!__constructor_order)
> - __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
> -}
> -
> int main(int argc, char **argv)
> {
> int fd = open(UV_PATH, O_ACCMODE);
> diff --git a/tools/testing/selftests/hid/hid_bpf.c
> b/tools/testing/selftests/hid/hid_bpf.c
> index 2cf96f818f25..f47feef2aced 100644
> --- a/tools/testing/selftests/hid/hid_bpf.c
> +++ b/tools/testing/selftests/hid/hid_bpf.c
> @@ -853,12 +853,6 @@ static int libbpf_print_fn(enum libbpf_print_level level,
> return 0;
> }
>
> -static void __attribute__((constructor)) __constructor_order_last(void)
> -{
> - if (!__constructor_order)
> - __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
> -}
> -
> int main(int argc, char **argv)
> {
> /* Use libbpf 1.0 API mode */
> diff --git a/tools/testing/selftests/kselftest_harness.h
> b/tools/testing/selftests/kselftest_harness.h
> index ba3ddeda24bf..60c1cf5b0f0d 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -444,12 +444,6 @@
> * Use once to append a main() to the test file.
> */
> #define TEST_HARNESS_MAIN \
> - static void __attribute__((constructor)) \
> - __constructor_order_last(void) \
> - { \
> - if (!__constructor_order) \
> - __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; \
> - } \
> int main(int argc, char **argv) { \
> return test_harness_run(argc, argv); \
> }
This won't work. All constructors are executed, so we have to figure
out which is run _first_. Switching this to a boolean means we gain no
information about ordering: it'll always be set to "true". We need to
detect which constructor sets it first so that we can walk the lists
(that are built via all the constructors in between) in the correct
order.
> @@ -846,7 +840,6 @@ static struct __fixture_metadata *__fixture_list =
> &_fixture_global;
> static int __constructor_order;
>
> #define _CONSTRUCTOR_ORDER_FORWARD 1
> -#define _CONSTRUCTOR_ORDER_BACKWARD -1
>
> static inline void __register_fixture(struct __fixture_metadata *f)
> {
> @@ -1272,8 +1265,7 @@ static int test_harness_run(int argc, char **argv)
>
> static void __attribute__((constructor)) __constructor_order_first(void)
> {
> - if (!__constructor_order)
> - __constructor_order = _CONSTRUCTOR_ORDER_FORWARD;
> + __constructor_order = _CONSTRUCTOR_ORDER_FORWARD;
> }
>
> #endif /* __KSELFTEST_HARNESS_H */
> diff --git a/tools/testing/selftests/rtc/rtctest.c
> b/tools/testing/selftests/rtc/rtctest.c
> index 63ce02d1d5cc..9647b14b47c5 100644
> --- a/tools/testing/selftests/rtc/rtctest.c
> +++ b/tools/testing/selftests/rtc/rtctest.c
> @@ -410,13 +410,6 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) {
> ASSERT_EQ(new, secs);
> }
>
> -static void __attribute__((constructor))
> -__constructor_order_last(void)
> -{
> - if (!__constructor_order)
> - __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;
> -}
> -
> int main(int argc, char **argv)
> {
> switch (argc) {
A better question is why these tests are open-coding the execution of
"main"...
--
Kees Cook
Re: [PATCH 0/2] selftests: harness: refactor __constructor_order
On Fri, May 17, 2024 at 08:45:04PM +0900, Masahiro Yamada wrote:
>
> This series refactors __constructor_order because
> __constructor_order_last() is unneeded.
>
> BTW, the comments in kselftest_harness.h was confusing to me.
>
> As far as I tested, all arches executed constructors in the forward
> order.
>
> [test code]
>
> #include
>
> static int x;
>
> static void __attribute__((constructor)) increment(void)
> {
>x += 1;
> }
>
> static void __attribute__((constructor)) multiply(void)
> {
> x *= 2;
> }
>
> int main(void)
> {
> printf("foo = %d\n", x);
> return 0;
> }
>
> It should print 2 for forward order systems, 1 for reverse order systems.
>
> I executed it on some archtes by using QEMU. I always got 2.
IIRC, and it was a long time ago now, it was actually a difference
between libc implementations where I encountered the problem. Maybe
glibc vs Bionic?
-Kees
--
Kees Cook
Re: [PATCH v3] fs: fix unintentional arithmetic wraparound in offset calculation
On Fri, May 17, 2024 at 02:26:47AM +0100, Al Viro wrote: > On Fri, May 17, 2024 at 02:13:22AM +0100, Matthew Wilcox wrote: > > On Fri, May 17, 2024 at 12:29:06AM +, Justin Stitt wrote: > > > When running syzkaller with the newly reintroduced signed integer > > > overflow sanitizer we encounter this report: > > > > why do you keep saying it's unintentional? it's clearly intended. > > Because they are short on actual bugs to be found by their tooling > and attempt to inflate the sound/noise rate; therefore, every time "short on bugs"? We're trying to drive it to zero. I would *love* to be short on bugs. See my reply[1] to Ted. > when overflow _IS_ handled correctly, it must have been an accident - > we couldn't have possibly done the analysis correctly. And if somebody > insists that they _are_ capable of basic math, they must be dishonest. > So... "unintentional" it's going to be. As Justin said, this is a poor choice in wording. In other cases I've tried to describe this as making changes so that intent is unambiguous (to both a human and a compiler). > Math is hard, mmkay? > > Al, more than slightly annoyed by that aspect of the entire thing... I'm sorry about that. None of this is a commentary on code correctness; we're just trying to refactor things so that the compiler can help us catch the _unintended_ overflows. This one is _intended_, so here we are to find a palatable way to leave the behavior unchanged while gaining compiler coverage. -Kees [1] https://lore.kernel.org/linux-hardening/202405171329.019F2F566C@keescook/ -- Kees Cook
Re: [RFC] Mitigating unexpected arithmetic overflow
n. This does mean we need to change some styles to be more "observable" (and unambiguous) for the compiler, though. I think a great example recently was Peter's work creating "cleanup.h". This feature totally changes how people have to read a function (increase in burden), leans heavily on compiler behaviors, and shifts the burden of correctness away from the human. It's great! But it's also _very different_ from traditional/old-school C. -Kees -- Kees Cook
Re: [PATCH] kselftest: Desecalate reporting of missing _GNU_SOURCE
On Thu, May 16, 2024 at 04:28:48PM +0100, Mark Brown wrote:
> Commit daef47b89efd0b7 ("selftests: Compile kselftest headers with
> -D_GNU_SOURCE") adds a static_assert() which means that things which
> would be warnings about undeclared functions get escalated into build
> failures. While we do actually want _GNU_SOURCE to be defined for users
> of kselftest_harness we haven't actually done that yet and this is
> causing widespread build breaks which were previously warnings about
> uses of asprintf() without prototypes, including causing other test
> programs in the same directory to fail to build.
>
> Since the build failures that are introduced cause additional issues due
> to make stopping builds early replace the static_assert() with a
> missing without making the error more severe than it already was. This
> will be moot once the issue is fixed properly but reduces the disruption
> while that happens.
>
> Signed-off-by: Mark Brown
Reviewed-by: Kees Cook
--
Kees Cook
Re: [RFC] Mitigating unexpected arithmetic overflow
On Thu, May 16, 2024 at 12:48:47PM -0700, Justin Stitt wrote: > I don't think we're capable of identifying every single problematic > overflow/wraparound case in the kernel, this is pretty obvious > considering we've had decades to do so. Instead, it seems much more > feasible that we annotate (very, very minimally so as not to disrupt > code readability and style) the spots where we _know_ overflow should > happen. For the baby steps Linus wants, we can walk this path: - Finish the *signed* integer overflow refactoring/annotation. This is nearly done already, and every case we've found is either a legitimate bug (thankfully rare), or happens in code that is either accidentally correct (thanks to no UB), or the correctness is very unclear. Refactoring these cases improves readability for everyone and doesn't change the behavior. - Begin *signed* integer implicit truncation refactoring/annotation. As Linus suggested, dealing with this will catch a bunch of the flaws we've seen recently. Handling the false positives here will need some investigation and some compiler support, and that'll happen in parallel. - Tackle *unsigned* integer overflow on a per-type basis: we can start with the place Linus called out: size_t. This will let us focus on the first of the unsigned types that is not commonly wrapping, and is a regular place that unexpected overflow gets the kernel into big trouble. What we learn from these three steps should inform us what further steps down this path can look like. -Kees -- Kees Cook
Re: Extending Linux' Coverity model and also cover aarch64
On Thu, May 16, 2024 at 03:28:16PM +, Manthey, Norbert wrote: > we published an extension for the Coverity model that is used by the > CoverityScan setup for the Linux kernel [1]. We have been using this > extension to analyze the 6.1 kernel branch, and reported some fixes to > the upstream code base that are based on this model [2]. Feel free to > merge the pull request, and update the model in the CoverityScan setup. > We do not have access to that project to perform these updates > ourselves. Thanks for this! I'll get it loaded into the Linux-Next scanner. > To increase the analysis coverage to aarch64, we analyzed a x86 and a > aarch64 configuration. The increased coverage is achieved by using re- > configuration and cross-compilation during the analysis build. If you > are interested in this setup we can share the Dockerfile and script we > used for this process. We've only got access to the free Coverity scanner, but it would be nice to see if there was anything specific to arm64. > To prevent regressions in backports to LTS kernels, we wondered whether > the community is interested in setting up CoverityScan projects for > older kernel releases. Would such an extension be useful to show new > defects in addition to the current release testing? The only one we (lightly) manage right now is the linux-next scanner. If other folks want to host scanners for -stable kernels, that would be interesting, yes. -Kees -- Kees Cook
Re: [RFC] Mitigating unexpected arithmetic overflow
On May 15, 2024 12:36:36 AM PDT, Peter Zijlstra wrote: >On Wed, May 08, 2024 at 04:47:25PM -0700, Linus Torvalds wrote: >> For example, the most common case of overflow we've ever had has very >> much been array indexing. Now, sometimes that has actually been actual >> undefined behavior, because it's been overflow in signed variables, >> and those are "easy" to find in the sense that you just say "no, can't >> do that". UBSAN finds them, and that's good. > >We build with -fno-strict-overflow, which implies -fwrapv, which removes >the UB from signed overflow by mandating 2s complement. I am a broken record. :) This is _not_ about undefined behavior. This is about finding a way to make the intent of C authors unambiguous. That overflow wraps is well defined. It is not always _desired_. C has no way to distinguish between the two cases. -Kees -- Kees Cook
[PATCH] ubsan: Restore dependency on ARCH_HAS_UBSAN
While removing CONFIG_UBSAN_SANITIZE_ALL, ARCH_HAS_UBSAN wasn't correctly
depended on. Restore this, as we do not want to attempt UBSAN builds
unless it's actually been tested on a given architecture.
Reported-by: Masahiro Yamada
Closes: https://lore.kernel.org/all/20240514095427.541201-1-masahi...@kernel.org
Fixes: 918327e9b7ff ("ubsan: Remove CONFIG_UBSAN_SANITIZE_ALL")
Signed-off-by: Kees Cook
---
Cc: Marco Elver
Cc: Andrey Konovalov
Cc: Andrey Ryabinin
Cc: kasan-...@googlegroups.com
Cc: linux-hardening@vger.kernel.org
---
lib/Kconfig.ubsan | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index e81e1ac4a919..bdda600f8dfb 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -4,6 +4,7 @@ config ARCH_HAS_UBSAN
menuconfig UBSAN
bool "Undefined behaviour sanity checker"
+ depends on ARCH_HAS_UBSAN
help
This option enables the Undefined Behaviour sanity checker.
Compile-time instrumentation is used to detect various undefined
--
2.34.1
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/kees approved this pull request. Thanks for the updates! Let's get this in and continue with the rest of the support. :) https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] ubsan: remove meaningless CONFIG_ARCH_HAS_UBSAN
On Tue, May 14, 2024 at 06:54:26PM +0900, Masahiro Yamada wrote:
> All architectures can enable UBSAN regardless of ARCH_HAS_UBSAN
> because there is no "depends on ARCH_HAS_UBSAN" line.
>
> Fixes: 918327e9b7ff ("ubsan: Remove CONFIG_UBSAN_SANITIZE_ALL")
> Signed-off-by: Masahiro Yamada
Oh, er, we probably want the reverse of this: add a depends to
CONFIG_UBSAN, otherwise we may end up trying to build with UBSAN when it
is not supported. That was my intention -- it looks like I missed adding
the line. :(
-Kees
--
Kees Cook
Re: [PATCH v10 0/5] Introduce mseal
On Tue, May 14, 2024 at 10:46:46AM -0700, Andrew Morton wrote: > On Mon, 15 Apr 2024 16:35:19 + jef...@chromium.org wrote: > > > This patchset proposes a new mseal() syscall for the Linux kernel. > > I have not moved this into mm-stable for a 6.10 merge. Mainly because > of the total lack of Reviewed-by:s and Acked-by:s. Oh, I thought I had already reviewed it. FWIW, please consider it: Reviewed-by: Kees Cook > The code appears to be stable enough for a merge. Agreed. > It's awkward that we're in conference this week, but I ask people to > give consideration to the desirability of moving mseal() into mainline > sometime over the next week, please. Yes please. :) -- Kees Cook
Re: [RFC][PATCH] PR tree-optimization/109071 - -Warray-bounds false positive warnings due to code duplication from jump threading
On Tue, May 14, 2024 at 02:17:16PM +, Qing Zhao wrote:
> The current major issue with the warning is: the constant index value 4
> is not in the source code, it’s a compiler generated intermediate value
> (even though it’s a correct value -:)). Such warning messages confuse
> the end-users with information that cannot be connected directly to the
> source code.
Right -- this "4" comes from -fsanitize=array-bounds (in "warn but
continue" mode).
Now, the minimized PoC shows a situation that triggers the situation, but
I think it's worth looking at the original code that caused this false
positive:
for (i = 0; i < sg->num_entries; i++) {
gce = >gce[i];
The issue here is that sg->num_entries has already been bounds-checked
in a separate function. As a result, the value range tracking for "i"
here is unbounded.
Enabling -fsanitize=array-bounds means the sg->gce[i] access gets
instrumented, and suddenly "i" gains an implicit range, induced by the
sanitizer.
(I would point out that this is very similar to the problems we've had
with -fsanitize=shift[1][2]: the sanitizer induces a belief about a
given variable's range this isn't true.)
Now, there is an argument to be made that the original code should be
doing:
for (i = 0; i < 4 && i < sg->num_entries; i++) {
But this is:
a) logically redundant (Linux maintainers don't tend to like duplicating
their range checking)
b) a very simple case
The point of the sanitizers is to catch "impossible" situations at
run-time for the cases where some value may end up out of range. Having
it _induce_ a range on the resulting code makes no sense.
Could we, perhaps, have sanitizer code not influence the value range
tracking? That continues to look like the root cause for these things.
-Kees
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108306
--
Kees Cook
Re: [RFC][PATCH] PR tree-optimization/109071 - -Warray-bounds false positive warnings due to code duplication from jump threading
On Mon, May 13, 2024 at 07:48:30PM +, Qing Zhao wrote: > The false positive warnings are moved from -Warray-bounds=1 to > -Warray-bounds=2 now. On a Linux kernel x86_64 allmodconfig build, this takes the -Warray-bounds warnings from 14 to 9. After examining these 9, I see: - 4: legitimate bugs in Linux source (3 distinct, 1 repeat). I'll be reporting/fixing these in Linux. - 4: 4 instances of 1 case of buggy interaction with -fsanitize=shift, looking similar to these past bugs: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108306 the difference being operating on an enum. I will reduce the case and open a bug report for it. - 1: still examining. It looks like a false positive so far. Thanks! -Kees -- Kees Cook
Re: [PATCH] hpfs: Annotate struct hpfs_dirent with __counted_by
On Mon, May 13, 2024 at 04:45:54PM -0700, Bill Wendling 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 checking
> via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> (for strcpy/memcpy-family functions).
>
> Cc: Mikulas Patocka
> Cc: Kees Cook
> Cc: "Gustavo A. R. Silva"
> Cc: Nathan Chancellor
> Cc: Nick Desaulniers
> Cc: Justin Stitt
> Cc: linux-ker...@vger.kernel.org
> Cc: linux-hardening@vger.kernel.org
> Cc: l...@lists.linux.dev
> Signed-off-by: Bill Wendling
> ---
> fs/hpfs/hpfs.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/hpfs/hpfs.h b/fs/hpfs/hpfs.h
> index 281dec8f636b..ac137787f1f9 100644
> --- a/fs/hpfs/hpfs.h
> +++ b/fs/hpfs/hpfs.h
> @@ -357,7 +357,7 @@ struct hpfs_dirent {
>u8 ix; /* code page index (of filename), see
> struct code_page_data */
>u8 namelen; /* file name length */
> - u8 name[]; /* file name */
> + u8 name[] __counted_by(namelen); /* file name */
>/* dnode_secno down; btree down pointer, if present,
> follows name on next word boundary, or maybe it
> precedes next dirent, which is on a word boundary. */
Looking through struct hpfs_dirent::name uses, I think everything checks
out. I do see some confusing manipulations in hpfs_add_de(), though. I
*think* it'll be okay, though.
Reviewed-by: Kees Cook
--
Kees Cook
Re: [RFC][PATCH] PR tree-optimization/109071 - -Warray-bounds false positive warnings due to code duplication from jump threading
On Tue, May 14, 2024 at 01:38:49AM +0200, Andrew Pinski wrote:
> On Mon, May 13, 2024, 11:41 PM Kees Cook wrote:
> > But it makes no sense to warn about:
> >
> > void sparx5_set (int * ptr, struct nums * sg, int index)
> > {
> >if (index >= 4)
> > warn ();
> >*ptr = 0;
> >*val = sg->vals[index];
> >if (index >= 4)
> > warn ();
> >*ptr = *val;
> > }
> >
> > Because at "*val = sg->vals[index];" the actual value range tracking for
> > index is _still_ [INT_MIN,INT_MAX]. (Only within the "then" side of the
> > "if" statements is the range tracking [4,INT_MAX].)
> >
> > However, in the case where jump threading has split the execution flow
> > and produced a copy of "*val = sg->vals[index];" where the value range
> > tracking for "index" is now [4,INT_MAX], is the warning valid. But it
> > is only for that instance. Reporting it for effectively both (there is
> > only 1 source line for the array indexing) is misleading because there
> > is nothing the user can do about it -- the compiler created the copy and
> > then noticed it had a range it could apply to that array index.
> >
>
> "there is nothing the user can do about it" is very much false. They could
> change warn call into a noreturn function call instead. (In the case of
> the Linux kernel panic). There are things the user can do to fix the
> warning and even get better code generation out of the compilers.
This isn't about warn() not being noreturn. The warn() could be any
function call; the jump threading still happens.
GCC is warning about a compiler-constructed situation that cannot be
reliably fixed on the source side (GCC emitting the warning is highly
unstable in these cases), since the condition is not *always* true for
the given line of code. If it is not useful to warn for "array[index]"
being out of range when "index" is always [INT_MIN,INT_MAX], then it
is not useful to warn when "index" MAY be [INT_MIN,INT_MAX] for a given
line of code.
-Kees
--
Kees Cook
Re: [PATCH v2] fs: fix unintentional arithmetic wraparound in offset calculation
On Mon, May 13, 2024 at 10:06:59PM +, Justin Stitt wrote:
> On Mon, May 13, 2024 at 01:01:57PM -0700, Kees Cook wrote:
> > On Thu, May 09, 2024 at 11:42:07PM +, Justin Stitt wrote:
> > > fs/read_write.c | 18 +++---
> > > fs/remap_range.c | 12 ++--
> > > 2 files changed, 17 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/read_write.c b/fs/read_write.c
> > > index d4c036e82b6c..d116e6e3eb3d 100644
> > > --- a/fs/read_write.c
> > > +++ b/fs/read_write.c
> > > @@ -88,7 +88,7 @@ generic_file_llseek_size(struct file *file, loff_t
> > > offset, int whence,
> > > {
> > > switch (whence) {
> > > case SEEK_END:
> > > - offset += eof;
> > > + offset = min(offset, maxsize - eof) + eof;
> >
> > This seems effectively unchanged compared to v1?
> >
> > https://lore.kernel.org/all/cafhgd8qbuyxmgifulgq7dwxfutzacvt82wd4jss-xntvtzx...@mail.gmail.com/
> >
>
> Right, please note the timestamps of Jan's review of v1 and when I sent
> v2. Essentially, I sent v2 before Jan's review of v1 and as such v2 does
> not fix the problem pointed out by Jan (the behavior of seek is
> technically different for VERY LARGE offsets).
Oh! Heh. I was tricked by versioning! ;)
--
Kees Cook
Re: [PATCH 0/3] kbuild: remove many tool coverage variables
On Tue, May 14, 2024 at 07:39:31AM +0900, Masahiro Yamada wrote: > On Tue, May 14, 2024 at 3:48 AM Kees Cook wrote: > > I am worried about the use of "guess" and "most", though. :) Before, we > > had some clear opt-out situations, and now it's more of a side-effect. I > > think this is okay, but I'd really like to know more about your testing. > > - defconfig for arc, hexagon, loongarch, microblaze, sh, xtensa > - allmodconfig for the other architectures > > (IIRC, allmodconfig failed for the first case, for reasons unrelated > to this patch set, so I used defconfig instead. > I do not remember what errors I observed) > > I checked the diff of .*.cmd files. Ah-ha, perfect! Thanks. :) > > Did you find any cases where you found that instrumentation was _removed_ > > where not expected? > > See the commit log of 1/3. Okay, thanks. I wasn't sure if that was the complete set or just part of the "most" bit. :) Thanks! I think this should all be fine. I'm not aware of anything melting down yet from these changes being in -next, so: Reviewed-by: Kees Cook -- Kees Cook
Re: [RFC][PATCH] PR tree-optimization/109071 - -Warray-bounds false positive warnings due to code duplication from jump threading
ge tracking for "index" is now [4,INT_MAX], is the warning valid. But it is only for that instance. Reporting it for effectively both (there is only 1 source line for the array indexing) is misleading because there is nothing the user can do about it -- the compiler created the copy and then noticed it had a range it could apply to that array index. This situation makes -Warray-bounds unusable for the Linux kernel (we cannot have false positives says BDFL), but we'd *really* like to have it enabled since it usually finds real bugs. But these false positives can't be fixed on our end. :( So, moving them to -Warray-bounds=2 makes sense as that's the level documented to have potential false positives. -Kees -- Kees Cook
Re: [PATCH v2] x86: um: vdso: Disable UBSAN instrumentation
On Mon, May 13, 2024 at 03:10:24PM +0200, Roberto Sassu wrote: > From: Roberto Sassu > > The UBSAN instrumentation cannot work in the vDSO since it is executing in > userspace, so disable it in the Makefile. Fixes the build failures such as: > > CALLscripts/checksyscalls.sh > VDSOarch/x86/um/vdso/vdso.so.dbg > arch/x86/um/vdso/vdso.so.dbg: undefined symbols found > > Signed-off-by: Roberto Sassu This is fixed differently (and more universally) here: https://lore.kernel.org/lkml/20240506133544.2861555-1-masahi...@kernel.org/ -- Kees Cook
Re: [PATCH v2] fs: fix unintentional arithmetic wraparound in offset calculation
ATA:
> @@ -1416,7 +1417,7 @@ static int generic_copy_file_checks(struct file
> *file_in, loff_t pos_in,
> struct inode *inode_in = file_inode(file_in);
> struct inode *inode_out = file_inode(file_out);
> uint64_t count = *req_count;
> - loff_t size_in;
> + loff_t size_in, in_sum, out_sum;
> int ret;
>
> ret = generic_file_rw_checks(file_in, file_out);
> @@ -1450,8 +1451,8 @@ static int generic_copy_file_checks(struct file
> *file_in, loff_t pos_in,
> if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
> return -ETXTBSY;
>
> - /* Ensure offsets don't wrap. */
> - if (pos_in + count < pos_in || pos_out + count < pos_out)
> + if (check_add_overflow(pos_in, count, _sum) ||
> + check_add_overflow(pos_out, count, _sum))
> return -EOVERFLOW;
I like these changes -- they make this much more readable.
>
> /* Shorten the copy to EOF */
> @@ -1467,8 +1468,8 @@ static int generic_copy_file_checks(struct file
> *file_in, loff_t pos_in,
>
> /* Don't allow overlapped copying within the same file. */
> if (inode_in == inode_out &&
> - pos_out + count > pos_in &&
> - pos_out < pos_in + count)
> + out_sum > pos_in &&
> + pos_out < in_sum)
> return -EINVAL;
>
> *req_count = count;
> @@ -1649,6 +1650,9 @@ int generic_write_check_limits(struct file *file,
> loff_t pos, loff_t *count)
> loff_t max_size = inode->i_sb->s_maxbytes;
> loff_t limit = rlimit(RLIMIT_FSIZE);
>
> + if (pos < 0)
> + return -EINVAL;
> +
> if (limit != RLIM_INFINITY) {
> if (pos >= limit) {
> send_sig(SIGXFSZ, current, 0);
> diff --git a/fs/remap_range.c b/fs/remap_range.c
> index de07f978ce3e..4570be4ef463 100644
> --- a/fs/remap_range.c
> +++ b/fs/remap_range.c
> @@ -36,7 +36,7 @@ static int generic_remap_checks(struct file *file_in,
> loff_t pos_in,
> struct inode *inode_out = file_out->f_mapping->host;
> uint64_t count = *req_count;
> uint64_t bcount;
> - loff_t size_in, size_out;
> + loff_t size_in, size_out, in_sum, out_sum;
> loff_t bs = inode_out->i_sb->s_blocksize;
> int ret;
>
> @@ -44,17 +44,17 @@ static int generic_remap_checks(struct file *file_in,
> loff_t pos_in,
> if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs))
> return -EINVAL;
>
> - /* Ensure offsets don't wrap. */
> - if (pos_in + count < pos_in || pos_out + count < pos_out)
> - return -EINVAL;
> + if (check_add_overflow(pos_in, count, _sum) ||
> + check_add_overflow(pos_out, count, _sum))
> + return -EOVERFLOW;
Yeah, this is a good error code change. This is ultimately exposed via
copy_file_range, where this error is documented as possible.
-Kees
--
Kees Cook
Re: [PATCH v3] perf/ring_buffer: Prefer struct_size over open coded arithmetic
On Sat, May 11, 2024 at 03:24:37PM +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].
>
> 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
Thanks!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [RFC] Mitigating unexpected arithmetic overflow
to_jiffies() conversion also has
> an integer overflow in it.
If we want these wrapping:
unsigned long __wraps timeout = arg * 10;
client->adapter->timeout = msecs_to_jiffies(timeout);
and:
static inline unsigned long _msecs_to_jiffies(const unsigned int __wraps m)
>
>486 break;
>
>
> drivers/i2c/busses/i2c-bcm2835.c:93 clk_bcm2835_i2c_calc_divider() warn:
> potential integer overflow from user 'parent_rate + rate'
> 90 static int clk_bcm2835_i2c_calc_divider(unsigned long rate,
> 91 unsigned long parent_rate)
> 92 {
> 93 u32 divider = DIV_ROUND_UP(parent_rate, rate);
> 94
> 95 /*
> 96 * Per the datasheet, the register is always interpreted as
> an even
> 97 * number, by rounding down. In other words, the LSB is
> ignored. So,
> 98 * if the LSB is set, increment the divider to avoid any
> issue.
> 99 */
>100 if (divider & 1)
>101 divider++;
>102 if ((divider < BCM2835_I2C_CDIV_MIN) ||
>103 (divider > BCM2835_I2C_CDIV_MAX))
>104 return -EINVAL;
>
> Again, math first then check the result later. Integer overflow is
> basically harmless.
Either fix the check or:
u32 __wraps divider = ...
>
>105
>106 return divider;
>107 }
>
>
> drivers/hwmon/nct6775-core.c:2265 store_temp_offset() warn: potential integer
> overflow from user '__x + (__d / 2)'
> 2251 static ssize_t
> 2252 store_temp_offset(struct device *dev, struct device_attribute *attr,
> 2253const char *buf, size_t count)
> 2254 {
> 2255 struct nct6775_data *data = dev_get_drvdata(dev);
> 2256 struct sensor_device_attribute *sattr =
> to_sensor_dev_attr(attr);
> 2257 int nr = sattr->index;
> 2258 long val;
> 2259 int err;
> 2260
> 2261 err = kstrtol(buf, 10, );
> 2262 if (err < 0)
> 2263 return err;
> 2264
> 2265 val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127);
> ^^
> Overflow and then clamp.
Looks like DIV_ROUND_CLOSEST() needs to be made sane and use more modern
helpers (it appears to be doing open-coded is_signed(), wants
check_*_overflow(), etc).
>
> 2266
> 2267 mutex_lock(>update_lock);
>
> regards,
> dan carpenter
-Kees
--
Kees Cook
Re: [PATCH] net: prestera: Add flex arrays to some structs
On Sun, May 12, 2024 at 06:10:27PM +0200, Erick Archer wrote: > The "struct prestera_msg_vtcam_rule_add_req" uses a dynamically sized > set of trailing elements. Specifically, it uses an array of structures > of type "prestera_msg_acl_action actions_msg". > > The "struct prestera_msg_flood_domain_ports_set_req" also uses a > dynamically sized set of trailing elements. Specifically, it uses an > array of structures of type "prestera_msg_acl_action actions_msg". > > So, use the preferred way in the kernel declaring flexible arrays [1]. > > 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 attribute used is specifically __counted_by_le since the > counters are of type __le32. > > The logic does not need to change since the counters for the flexible > arrays are asigned before any access to the arrays. > > The order in which the structure prestera_msg_vtcam_rule_add_req and the > structure prestera_msg_flood_domain_ports_set_req are defined must be > changed to avoid incomplete type errors. > > Also, avoid the open-coded arithmetic in memory allocator functions [2] > using the "struct_size" macro. > > Moreover, the new structure members also allow us to avoid the open- > coded arithmetic on pointers. So, take advantage of this refactoring > accordingly. > > This code was detected with the help of Coccinelle, and audited and > modified manually. > > Link: > https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays > [1] > Link: > https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > [2] > Signed-off-by: Erick Archer This is a really nice cleanup. :) Fewer lines of code, more readable, and protected by __counted_by! Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v2] tty: rfcomm: prefer struct_size over open coded arithmetic
On Mon, May 13, 2024 at 07:12:57PM +0200, Erick Archer wrote:
> Hi Kees, Jiri and Luiz,
> First of all, thanks for the reviews.
>
> On Mon, May 13, 2024 at 12:29:04PM -0400, Luiz Augusto von Dentz wrote:
> > Hi Jiri, Eric,
> >
> > On Mon, May 13, 2024 at 1:07 AM Jiri Slaby wrote:
> > >
> > > On 12. 05. 24, 13:17, 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 "dl" variable is a pointer to "struct rfcomm_dev_list_req" and
> > > > this structure ends in a flexible array:
> > > ...
> > > > --- a/include/net/bluetooth/rfcomm.h
> > > > +++ b/include/net/bluetooth/rfcomm.h
> > > ...
> > > > @@ -528,12 +527,12 @@ static int rfcomm_get_dev_list(void __user *arg)
> > > > list_for_each_entry(dev, _dev_list, list) {
> > > > if (!tty_port_get(>port))
> > > > continue;
> > > > - (di + n)->id = dev->id;
> > > > - (di + n)->flags = dev->flags;
> > > > - (di + n)->state = dev->dlc->state;
> > > > - (di + n)->channel = dev->channel;
> > > > - bacpy(&(di + n)->src, >src);
> > > > - bacpy(&(di + n)->dst, >dst);
> > > > + di[n].id = dev->id;
> > > > + di[n].flags = dev->flags;
> > > > + di[n].state = dev->dlc->state;
> > > > + di[n].channel = dev->channel;
> > > > + bacpy([n].src, >src);
> > > > + bacpy([n].dst, >dst);
> > >
> > > This does not relate much to "prefer struct_size over open coded
> > > arithmetic". It should have been in a separate patch.
> >
> > +1, please split these changes into its own patch so we can apply it
> > separately.
>
> Ok, no problem. Also, I will simplify the "bacpy" lines with direct
> assignments as Kees suggested:
>
>di[n].src = dev->src;
>di[n].dst = dev->dst;
>
> instead of:
>
>bacpy([n].src, >src);
>bacpy([n].dst, >dst);
I think that's a separate thing and you can leave bacpy() as-is for now.
--
Kees Cook
Re: [PATCH 0/3] kbuild: remove many tool coverage variables
In the future can you CC the various maintainers of the affected tooling? :) On Mon, May 06, 2024 at 10:35:41PM +0900, Masahiro Yamada wrote: > > This patch set removes many instances of the following variables: > > - OBJECT_FILES_NON_STANDARD > - KASAN_SANITIZE > - UBSAN_SANITIZE > - KCSAN_SANITIZE > - KMSAN_SANITIZE > - GCOV_PROFILE > - KCOV_INSTRUMENT > > Such tools are intended only for kernel space objects, most of which > are listed in obj-y, lib-y, or obj-m. This is a reasonable assertion, and the changes really simplify things now and into the future. Thanks for finding such a clean solution! I note that it also immediately fixes the issue noticed and fixed here: https://lore.kernel.org/all/20240513122754.1282833-1-roberto.sa...@huaweicloud.com/ > The best guess is, objects in $(obj-y), $(lib-y), $(obj-m) can opt in > such tools. Otherwise, not. > > This works in most places. I am worried about the use of "guess" and "most", though. :) Before, we had some clear opt-out situations, and now it's more of a side-effect. I think this is okay, but I'd really like to know more about your testing. It seems like you did build testing comparing build flags, since you call out some of the explicit changes in patch 2, quoting: > - include arch/mips/vdso/vdso-image.o into UBSAN, GCOV, KCOV > - include arch/sparc/vdso/vdso-image-*.o into UBSAN > - include arch/sparc/vdso/vma.o into UBSAN > - include arch/x86/entry/vdso/extable.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > - include arch/x86/entry/vdso/vdso-image-*.o into KASAN, KCSAN, UBSAN, GCOV, > KCOV > - include arch/x86/entry/vdso/vdso32-setup.o into KASAN, KCSAN, UBSAN, GCOV, > KCOV > - include arch/x86/entry/vdso/vma.o into GCOV, KCOV > - include arch/x86/um/vdso/vma.o into KASAN, GCOV, KCOV I would agree that these cases are all likely desirable. Did you find any cases where you found that instrumentation was _removed_ where not expected? -Kees -- Kees Cook
Re: [RFC] Mitigating unexpected arithmetic overflow
On Sun, May 12, 2024 at 09:09:08AM -0700, Linus Torvalds wrote:
> unsigned char *p;
> u32 val;
>
> p[0] = val;
> p[1] = val >> 8;
> p[2] = val >> 16;
> p[3] = val >> 24;
>
> kind of code is both traditional and correct, but obviously drops bits
> very much intentionally on each of those assignments.
The good news here is that the integer implicit truncation sanitizers
are already split between "signed" and "unsigned". So the 2 cases of
exploitable flaws mentioned earlier:
u8 num_elems;
...
num_elems++;/* int promotion stored back to u8 */
and
int size;
u16 read_size;
...
read_size = size; /* large int stored to u16 */
are both confusions across signed/unsigned types, which the signed
sanitizer would catch. The signed sanitizer would entirely ignore
the quoted example at the top: everything is unsigned and no int
promotion is happening.
So, I think we can start with just the "signed integer implicit
truncation" sanitizer. The compiler will still need to deal with the
issues I outlined in [1], where I think we need some consideration
specifically on how to handle things like this (that have a
smaller-than-int size and trip the sanitizer due to int promotion):
u8 checksum(const u8 *buf)
{
u8 sum = 0;
for (int i = 0; i < 4; i++)
sum += buf[i]; /* int promotion */
return sum;
}
We want "sum" to wrap. We could avoid the "implicit" truncation by
explicitly truncating with something eye-bleedingly horrible like:
sum = (u8)(sum + buf[i]);
Adding a wrapper for the calculation could work but repeats "sum", and
needs to be explicitly typed, making it just as unfriendly:
sum = truncate(u8, sum + buf[i]);
Part of the work I'd done in preparation for all this was making the
verbosely named wrapping_assign_add() helper which handles all the
types by examining the arguments and avoids repeating the destination
argument. So this would become:
wrapping_assign_add(sum, buf[i]);
Still not as clean as "+=", but at least more readable than the
alternatives and leaves no question about wrapping intent.
-Kees
[1] https://lore.kernel.org/lkml/202405081949.0565810E46@keescook/
--
Kees Cook
Re: [PATCH] Bluetooth: hci_core: Prefer struct_size over open coded arithmetic
On Sun, May 12, 2024 at 04:17:06PM +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].
>
> As the "dl" variable is a pointer to "struct hci_dev_list_req" and this
> structure ends in a flexible array:
>
> struct hci_dev_list_req {
> [...]
> struct hci_dev_req dev_req[]; /* hci_dev_req structures */
> };
>
> 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() and copy_to_user() functions.
>
> 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 "dev_num" member is initialized before
> the first access to the flex array. Specifically, add the assignment
> before the list_for_each_entry() loop.
>
> Also remove the "size" variable as it is no longer needed and refactor
> the list_for_each_entry() loop to use dr[n] instead of (dr + n).
>
> This way, the code is more readable, idiomatic 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
Looks right to me. Thanks!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v2] tty: rfcomm: prefer struct_size over open coded arithmetic
On Sun, May 12, 2024 at 01:17:24PM +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].
>
> As the "dl" variable is a pointer to "struct rfcomm_dev_list_req" and
> this structure ends in a flexible array:
>
> struct rfcomm_dev_list_req {
> [...]
> struct rfcomm_dev_info dev_info[];
> };
>
> 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() and copy_to_user() functions.
>
> 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 "dev_num" member is initialized before
> the first access to the flex array. Specifically, add the assignment
> before the list_for_each_entry() loop.
>
> Also remove the "size" variable as it is no longer needed and refactor
> the list_for_each_entry() loop to use di[n] instead of (di + n).
>
> This way, the code is more readable, idiomatic 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
Looks good!
Reviewed-by: Kees Cook
> [...]
> - bacpy(&(di + n)->src, >src);
> - bacpy(&(di + n)->dst, >dst);
> + bacpy([n].src, >src);
> + bacpy([n].dst, >dst);
Not an issue with your patch, but this helper is really pointless in the
Bluetooth tree:
static inline void bacpy(bdaddr_t *dst, const bdaddr_t *src)
{
memcpy(dst, src, sizeof(bdaddr_t));
}
So the above could just be:
di[n].src = dev->src;
di[n].dst = dev->dst;
:P
-Kees
--
Kees Cook
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Sun, May 12, 2024 at 09:32:40PM +0200, Joel Granados wrote: > On Sat, May 11, 2024 at 11:51:18AM +0200, Thomas Weißschuh wrote: > > Hi Kees, > > > > On 2024-05-08 10:11:35+, Kees Cook wrote: > > > On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > > > > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > > > > The series was split from my larger series sysctl-const series [0]. > > > > > It only focusses on the proc_handlers but is an important step to be > > > > > able to move all static definitions of ctl_table into .rodata. > > > > > > > > Split this per subsystem, please. > > > > > > I've done a few painful API transitions before, and I don't think the > > > complexity of these changes needs a per-subsystem constification pass. I > > > think this series is the right approach, but that patch 11 will need > > > coordination with Linus. We regularly do system-wide prototype changes > > > like this right at the end of the merge window before -rc1 comes out. > > > > That sounds good. > > > > > The requirements are pretty simple: it needs to be a obvious changes > > > (this certainly is) and as close to 100% mechanical as possible. I think > > > patch 11 easily qualifies. Linus should be able to run the same Coccinelle > > > script and get nearly the same results, etc. And all the other changes > > > need to have landed. This change also has no "silent failure" conditions: > > > anything mismatched will immediately stand out. > > > > Unfortunately coccinelle alone is not sufficient, as some helpers with > > different prototypes are called by handlers and themselves are calling > > handler and therefore need to change in the same commit. > > But if I add a diff for those on top of the coccinelle script to the > > changelog it should be obvious. > Judging by Kees' comment on "100% mechanical", it might be better just > having the diff and have Linus apply than rather than two step process? > Have not these types of PRs, so am interested in what folks think. I tried to soften it a little with my "*close* to 100%" modifier, and I think that patch basically matched that requirement, and where it had manual changes it was detailed in the commit log. I only split out the seccomp part because it could actually stand alone. So yeah, let's get the last of the subsystem specific stuff landed after -rc1, and it should be possible to finish it all up for 6.11. Yay! :) -Kees -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Sun, May 12, 2024 at 09:32:40PM +0200, Joel Granados wrote: > On Sat, May 11, 2024 at 11:51:18AM +0200, Thomas Weißschuh wrote: > > Hi Kees, > > > > On 2024-05-08 10:11:35+, Kees Cook wrote: > > > On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > > > > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > > > > The series was split from my larger series sysctl-const series [0]. > > > > > It only focusses on the proc_handlers but is an important step to be > > > > > able to move all static definitions of ctl_table into .rodata. > > > > > > > > Split this per subsystem, please. > > > > > > I've done a few painful API transitions before, and I don't think the > > > complexity of these changes needs a per-subsystem constification pass. I > > > think this series is the right approach, but that patch 11 will need > > > coordination with Linus. We regularly do system-wide prototype changes > > > like this right at the end of the merge window before -rc1 comes out. > > > > That sounds good. > > > > > The requirements are pretty simple: it needs to be a obvious changes > > > (this certainly is) and as close to 100% mechanical as possible. I think > > > patch 11 easily qualifies. Linus should be able to run the same Coccinelle > > > script and get nearly the same results, etc. And all the other changes > > > need to have landed. This change also has no "silent failure" conditions: > > > anything mismatched will immediately stand out. > > > > Unfortunately coccinelle alone is not sufficient, as some helpers with > > different prototypes are called by handlers and themselves are calling > > handler and therefore need to change in the same commit. > > But if I add a diff for those on top of the coccinelle script to the > > changelog it should be obvious. > Judging by Kees' comment on "100% mechanical", it might be better just > having the diff and have Linus apply than rather than two step process? > Have not these types of PRs, so am interested in what folks think. I tried to soften it a little with my "*close* to 100%" modifier, and I think that patch basically matched that requirement, and where it had manual changes it was detailed in the commit log. I only split out the seccomp part because it could actually stand alone. So yeah, let's get the last of the subsystem specific stuff landed after -rc1, and it should be possible to finish it all up for 6.11. Yay! :) -Kees -- Kees Cook
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Sun, May 12, 2024 at 09:32:40PM +0200, Joel Granados wrote: > On Sat, May 11, 2024 at 11:51:18AM +0200, Thomas Weißschuh wrote: > > Hi Kees, > > > > On 2024-05-08 10:11:35+, Kees Cook wrote: > > > On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > > > > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > > > > The series was split from my larger series sysctl-const series [0]. > > > > > It only focusses on the proc_handlers but is an important step to be > > > > > able to move all static definitions of ctl_table into .rodata. > > > > > > > > Split this per subsystem, please. > > > > > > I've done a few painful API transitions before, and I don't think the > > > complexity of these changes needs a per-subsystem constification pass. I > > > think this series is the right approach, but that patch 11 will need > > > coordination with Linus. We regularly do system-wide prototype changes > > > like this right at the end of the merge window before -rc1 comes out. > > > > That sounds good. > > > > > The requirements are pretty simple: it needs to be a obvious changes > > > (this certainly is) and as close to 100% mechanical as possible. I think > > > patch 11 easily qualifies. Linus should be able to run the same Coccinelle > > > script and get nearly the same results, etc. And all the other changes > > > need to have landed. This change also has no "silent failure" conditions: > > > anything mismatched will immediately stand out. > > > > Unfortunately coccinelle alone is not sufficient, as some helpers with > > different prototypes are called by handlers and themselves are calling > > handler and therefore need to change in the same commit. > > But if I add a diff for those on top of the coccinelle script to the > > changelog it should be obvious. > Judging by Kees' comment on "100% mechanical", it might be better just > having the diff and have Linus apply than rather than two step process? > Have not these types of PRs, so am interested in what folks think. I tried to soften it a little with my "*close* to 100%" modifier, and I think that patch basically matched that requirement, and where it had manual changes it was detailed in the commit log. I only split out the seccomp part because it could actually stand alone. So yeah, let's get the last of the subsystem specific stuff landed after -rc1, and it should be possible to finish it all up for 6.11. Yay! :) -Kees -- Kees Cook
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -0,0 +1,187 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define __counted_by(f) __attribute__((counted_by(f)))
+
+struct bar;
+
+struct not_found {
+ int count;
+ struct bar *fam[] __counted_by(bork); // expected-error {{use of undeclared
identifier 'bork'}}
+};
+
+struct no_found_count_not_in_substruct {
+ unsigned long flags;
+ unsigned char count; // expected-note {{'count' declared here}}
+ struct A {
+int dummy;
+int array[] __counted_by(count); // expected-error {{'counted_by' field
'count' isn't within the same struct as the flexible array}}
+ } a;
+};
+
+struct not_found_count_not_in_unnamed_substruct {
+ unsigned char count; // expected-note {{'count' declared here}}
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{'counted_by' field
'count' isn't within the same struct as the flexible array}}
+ } a;
+};
+
+struct not_found_count_not_in_unnamed_substruct_2 {
+ struct {
+unsigned char count; // expected-note {{'count' declared here}}
+ };
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{'counted_by' field
'count' isn't within the same struct as the flexible array}}
+ } a;
+};
+
+struct not_found_count_in_other_unnamed_substruct {
+ struct {
+unsigned char count;
+ } a1;
+
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{use of undeclared
identifier 'count'}}
+ };
+};
+
+struct not_found_count_in_other_substruct {
+ struct _a1 {
+unsigned char count;
+ } a1;
+
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{use of undeclared
identifier 'count'}}
+ };
+};
+
+struct not_found_count_in_other_substruct_2 {
+ struct _a2 {
+unsigned char count;
+ } a2;
+
+ int array[] __counted_by(count); // expected-error {{use of undeclared
identifier 'count'}}
+};
+
+struct not_found_suggest {
+ int bork;
+ struct bar *fam[] __counted_by(blork); // expected-error {{use of undeclared
identifier 'blork'}}
+};
+
+int global; // expected-note {{'global' declared here}}
+
+struct found_outside_of_struct {
+ int bork;
+ struct bar *fam[] __counted_by(global); // expected-error {{field 'global'
in 'counted_by' not inside structure}}
+};
+
+struct self_referrential {
+ int bork;
+ struct bar *self[] __counted_by(self); // expected-error {{use of undeclared
identifier 'self'}}
+};
+
+struct non_int_count {
+ double dbl_count;
+ struct bar *fam[] __counted_by(dbl_count); // expected-error {{'counted_by'
requires a non-boolean integer type argument}}
+};
+
+struct array_of_ints_count {
+ int integers[2];
+ struct bar *fam[] __counted_by(integers); // expected-error {{'counted_by'
requires a non-boolean integer type argument}}
+};
+
+struct not_a_fam {
+ int count;
+ // expected-error@+1{{'counted_by' cannot be applied to a pointer with
pointee of unknown size because 'struct bar' is an incomplete type}}
+ struct bar *non_fam __counted_by(count);
+};
+
+struct not_a_c99_fam {
+ int count;
+ struct bar *non_c99_fam[0] __counted_by(count); // expected-error
{{'counted_by' on arrays only applies to C99 flexible array members}}
+};
+
+struct annotated_with_anon_struct {
+ unsigned long flags;
+ struct {
+unsigned char count;
+int array[] __counted_by(crount); // expected-error {{use of undeclared
identifier 'crount'}}
+ };
+};
+
+//==
+// __counted_by on a struct VLA with element type that has unknown size
+//==
+
+struct size_unknown; // expected-note 2{{forward declaration of 'struct
size_unknown'}}
+struct on_member_arr_incomplete_ty_ty_pos {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{array has incomplete element type 'struct
size_unknown'}}
+ struct size_unknown buf[] __counted_by(count);
+};
+
+struct on_member_arr_incomplete_const_ty_ty_pos {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{array has incomplete element type 'const struct
size_unknown'}}
+ const struct size_unknown buf[] __counted_by(count);
+};
+
+struct on_member_arr_void_ty_ty_pos {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{array has incomplete element type 'void'}}
+ void buf[] __counted_by(count);
+};
+
+typedef void(fn_ty)(int);
+
+struct on_member_arr_fn_ty {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{'buf' declared as array of functions of type 'fn_ty'
(aka 'void (int)')}}
+ fn_ty buf[] __counted_by(count);
+};
kees wrote:
Please add a positive test for `typedef
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
kees wrote:
> Consider this example. It tries to illustrate why putting `__counted_by()` on
> a pointer to a structs containing flexible array members doesn't make sense.
>
> ```c
> struct HasFAM {
> int count;
> char buffer[] __counted_by(count); // This is OK
> };
>
> struct BufferOfFAMS {
> int count;
> struct HasFAM* fams __counted_by(count); // This is invalid
> };
> ```
Agreed: structs with flexible array members must be considered to be
singletons. This property is actually important for being able to have
`__builtin_dynamic_object_size()` work on pointers to flexible array structs.
i.e.:
```
size_t func(struct foo *p)
{
return__builtin_dynamic_object_size(p, 0);
}
```
This must always return `SIZE_MAX` for fixed-size arrays since the pointer may
be in the middle of a larger array of `struct foo`s, but if it is a struct with
a flexible array marked with `counted_by`, then we know deterministically what
the size is, since it must be a single complete object.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
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
[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
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
kees wrote: > As @apple-fcloutier @rapidsna noted this is how `-fbounds-safety` is > currently implemented (because its much simpler) but it is a restriction that > could be lifted in future by only requiring `struct bar` to be defined at the > point that `foo::bar` is used rather than when the `__counted_by` attribute > is applied. Given that this PR is allowing `__counted_by` in a new place > (pointers in structs) I think its reasonable for us to place restrictions on > that until we've proved its actually necessary to have the more complicated > implementation. The main concern I have with delaying support for this is that header files could find themselves in a state where they could not be refactored without removing counted_by attributes that refer to now-incomplete structs. For example, in Linux we've been separating structs from implementations, and that usually means using incomplete structs as much as possible to avoid lots of #includes. So, no objection on this PR, but I think the more complete behavior needs to follow soonish. :) https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH v4 55/66] selftests/seccomp: Drop define _GNU_SOURCE
On Fri, May 10, 2024 at 12:07:12AM +, Edward Liaw wrote: > _GNU_SOURCE is provided by lib.mk, so it should be dropped to prevent > redefinition warnings. > > Reviewed-by: John Hubbard > Reviewed-by: Muhammad Usama Anjum > Signed-off-by: Edward Liaw Acked-by: Kees Cook -- Kees Cook
Re: [PATCH v4 13/66] selftests/exec: Drop duplicate -D_GNU_SOURCE
On Fri, May 10, 2024 at 12:06:30AM +, Edward Liaw wrote: > -D_GNU_SOURCE can be de-duplicated here, as it is added by lib.mk. > > Signed-off-by: Edward Liaw Thanks! Acked-by: Kees Cook -- Kees Cook
Re: [PATCH 1/3] selftests/exec: Build both static and non-static load_address tests
On Wed, May 08, 2024 at 07:54:13PM -0700, John Hubbard wrote: > Didn't we learn recently, though, that -static-pie is gcc 8.1+, while the > kernel's minimum gcc version is 5? Yes, that's true. If we encounter anyone trying to build the selftests with <8.1 I think we'll have to add a compiler version test in the Makefile to exclude the static pie tests. There's also the potential issue with arm64 builds that caused the original attempt at -static. We'll likely need an exclusion there too. -- Kees Cook
Re: [RFC] Mitigating unexpected arithmetic overflow
aces where we expect
truncation.
>[...thread merge...]
> I still suspect the "implicit truncating cast at assignment" is likely
> a much more common case of loss of information than actual arithmetic
> wrap-around, but clearly the two have commonalities.
Both of the "real world" examples we had in this thread were due
to implicit truncation. I do think it's less noisy than the unsigned
arithmetic cases, but it does have significant commonalities. So much so
that it still looks to me like using the "wraps" attribute makes sense
for this.
> Summary: targeted baby steps, not some draconian "wrap-around is wrong".
I still think we should bite the bullet, but let's finish up signed
overflow (this just keeps finding accidental but mostly harmless
overflows), and then dig into implicit integer truncation next.
Thank you for spending the time to look at all of this. It's a big topic
that many people feel very strongly about, so it's good to get some
"approved" direction on it. :)
-Kees
[1] These are NOT meant to be upstreamed as-is. We have idiom exclusions
to deal with, etc, but here's the tree I haven't refreshed yet:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=dev/v6.8-rc2/signed-trunc-sanitizer
[2] c14679d7005a ("wifi: cfg80211: Annotate struct cfg80211_mbssid_elems with
__counted_by")
--
Kees Cook
Re: [RFC] Mitigating unexpected arithmetic overflow
On Wed, May 08, 2024 at 12:22:38PM +, David Laight wrote: > Have you estimated the performance cost of checking the result of > all integer arithmetic. I hadn't included that in my already very long email as performance is somewhat secondary to the correctness goals. Perhaps that was a mistake, as it is going to be everyone's first question anyway. :) But yes, I did have an initial analysis: Performance Considerations == Adding arithmetic overflow checks, regardless of implementation, will add more cycles. The good news is that the overflow outcome can be pessimized, and checking the overflow bit on most architectures is extraordinarily fast. Regardless, for many Linux deployments, the cost of this correctness is seen as acceptable, though all users will benefit from the fixing of bugs that the mitigation will find. Looking specifically at proposal #1 below, we can do some estimations. For a defconfig+kvm+FORTIFY config on v6.9-rc2 x86_64, the signed integer overflow (SIO) checking is added in 21,552 places. The unsigned integer overflow (UIO) checking is around 55,000 (though I would expect a good portion of these to be eliminated as they are shown to be "wrap-around expected"). Running with SIO enabled is mostly flawless, though a long tail of false positives is expected. Running with UIO is not ready for general consumption, and performing benchmarking there is not going to give useful numbers. However, we can at least attempt to extrapolate from an SIO build how a full SIO+UIO build would behave. For example, based on instance counts, we could maybe assume SIO+UIO to be ~3x compared to SIO. This could easily be an over or under estimation, though. Regardless, some rough draft comparisons: Image size Stock60197540 SIO,warn60299436 +0.169% SIO,trap60195567 -0.003% (trap mode probably found code to drop) Kernel build 10x benchmark Avg(s) Std Dev Stock 439.58 1.68 SIO,warn 444.20 (+1.00%) 1.35 SIO,trap 442.10 (+0.57%) 1.52 > If you have a cpu with 'signed/unsigned add(etc) with trap on overflow' > instructions then maybe you could use them to panic the kernel. > But otherwise you'll need a conditional branch after pretty much > every arithmetic instruction. Yes. This would be true for any implementation. Thankfully in some places where bounds checking has already happened manually, the added instrumentation checks will have been optimized away. But yes, turning such a mitigation on isn't without impact. :) But a significant install base is interested in correctness within a reasonable performance budget. And some will take correctness over all other considerations. > As well as the code bloat there is likely to be a 50% chance they > are mis-predicted slowing things down a lot more. > IIRC at least some x86 cpu do not default to static prediction (eg > backwards taken forwards not) but always use data from the branch > prediction logic - so the first time a branch is seen it is predicted > randomly. Sure, though I think the nuances of CPU design are somewhat tangential to the overall topic: how do we provide a way for Linux to gain this correctness coverage? It's accepted that there will be a non-trivial impact, and I agree we can start to consider how to optimize implementations. But for now I'd like to solve for how to even represent arithmetic overflow intent at the source level. -Kees -- Kees Cook
