Re: [PATCH v3 01/26] kernel: Introduce meminspect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Integer truncation in memory allocation size causes an undersized CMA block, leading to a heap buffer overflow during ELF header initialization.
- [High] Invalid use of `virt_to_phys()` on statically allocated kernel symbols causes boot panics or returns bogus physical addresses.
- [High] Missing bounds checking on the `id` field leads to out-of-bounds accesses on the `inspect_entries` array.
- [Medium] ELF core dump file layout will be corrupted because physical traversal order diverges from chronological ELF header offsets.
- [Medium] Missing padding for unaligned region sizes causes cumulative offset mismatches in the dumped core file.
- [Medium] `meminspect_prepare_table` issues duplicate `MEMINSPECT_NOTIFIER_ADD` notifications for dynamic entries registered before the late initcall.
- [Low] `meminspect_notifier_list` is defined as an `ATOMIC_NOTIFIER_HEAD`, pointlessly forcing driver callbacks into an atomic context.
--

--- Patch [1]: [PATCH v3 01/26] kernel: Introduce meminspect ---
commit 62611e2ce3f1357bd155ab67d8e06e76e5461779
Author: Eugen Hristev <ehristev@kernel.org>

kernel: Introduce meminspect
    
This commit introduces a mechanism to register memory areas into an inspection
table for later use by drivers. The inspection table can be requested and
analyzed for purposes such as memory dumping or debugging.

> diff --git a/kernel/meminspect/meminspect.c b/kernel/meminspect/meminspect.c
> new file mode 100644
> index 0000000000000..d9d38f484f1f4
> --- /dev/null
> +++ b/kernel/meminspect/meminspect.c
> @@ -0,0 +1,474 @@
[ ... ]
> +static DEFINE_MUTEX(meminspect_lock);
> +static struct inspect_entry inspect_entries[MEMINSPECT_ID_MAX];
> +
> +static ATOMIC_NOTIFIER_HEAD(meminspect_notifier_list);

[Severity: Low]
Since the notifier events are dispatched while holding a sleepable mutex
(meminspect_lock), does using an ATOMIC_NOTIFIER_HEAD unnecessarily force all
consumer callbacks into a non-sleepable atomic context?

[ ... ]
> +static void update_elfheader(const struct inspect_entry *e)
> +{
> +	struct elf_phdr *phdr;
> +
> +	phdr = elf_phdr_entry_addr(ehdr, ehdr->e_phnum++);
> +
> +	phdr->p_type = PT_LOAD;
> +	phdr->p_offset = elf_offset;

[Severity: Medium]
If dynamic regions are registered out of chronological order, will
meminspect_traverse() yielding entries in strict numerical ID order cause the
physical data layout dumped by firmware to mismatch the chronologically
assigned p_offset values in the ELF headers?

> +	phdr->p_vaddr = (elf_addr_t)e->va;
> +	if (e->pa)
> +		phdr->p_paddr = (elf_addr_t)e->pa;
> +	else
> +		phdr->p_paddr = (elf_addr_t)virt_to_phys(e->va);
> +
> +	phdr->p_filesz = ALIGN(e->size, 4);
> +	phdr->p_memsz = ALIGN(e->size, 4);
> +	phdr->p_flags = PF_R | PF_W;
> +	elf_offset += ALIGN(e->size, 4);

[Severity: Medium]
If an unaligned region is registered, update_elfheader() advances the global
elf_offset and header sizes with 4-byte padding, but the base table retains
the unaligned size.

When firmware iterates the table to dump the memory exactly by e->size, won't
this missing padding cause a cumulative drift between the actual file offsets
and the p_offset recorded in the ELF headers?

> +}
[ ... ]
> +static int init_elfheader(void)
> +{
[ ... ]
> +	buf_sz = elfh_size + note_len - ALIGN(vmcoreinfo_size, 4);
> +
> +	/* Never freed */
> +	p = dma_alloc_from_contiguous(NULL, buf_sz >> PAGE_SHIFT,
> +				      get_order(buf_sz), true);

[Severity: High]
Does the expression buf_sz >> PAGE_SHIFT truncate the fractional page
remainder for unaligned sizes?

If buf_sz requires multiple pages but the shift truncates the count, could
this allocate an undersized CMA block and lead to a heap buffer overflow when
mapping the memory?

> +	if (!p)
> +		return -ENOMEM;
[ ... ]
> +void meminspect_unregister_id(enum meminspect_uid id)
> +{
> +	struct inspect_entry *e;
> +
> +	WARN_ON(!mutex_is_locked(&meminspect_lock));
> +
> +	e = &inspect_entries[id];

[Severity: High]
Does this function lack bounds checking on the id argument?

If a caller passes an ID greater than or equal to MEMINSPECT_ID_MAX, could
this result in an out-of-bounds access and corruption of adjacent kernel
memory?

> +	if (!e->id)
> +		return;
[ ... ]
> +static int __init meminspect_prepare_table(void)
> +{
> +	const struct inspect_entry *e;
> +	enum meminspect_uid i;
> +	int ret;
> +
> +	meminspect_table_lock();
> +	/*
> +	 * First, copy all entries from the compiler built table
> +	 * In case some entries are registered multiple times,
> +	 * the last chronological entry will be stored.
> +	 * Previously registered entries will be dropped.
> +	 */
> +	for_each_meminspect_entry(e) {
> +		inspect_entries[e->id] = *e;

[Severity: High]
Does this lack bounds checking on e->id?

If a statically compiled entry provides an invalid ID, could this result in
an out-of-bounds write?

> +		if (!inspect_entries[e->id].pa && inspect_entries[e->id].va)
> +			inspect_entries[e->id].pa = virt_to_phys(inspect_entries[e->id].va);

[Severity: High]
Is it safe to use virt_to_phys() on statically allocated kernel symbols parsed
from the linker section here?

Since these symbols reside in the kernel image map (which is outside the
linear map on architectures like arm64), could this cause a panic on boot or
return incorrect physical addresses?

> +	}
> +#ifdef CONFIG_CRASH_DUMP
> +	ret = meminspect_prepare_crashdump();
> +	if (ret)
> +		pr_warn("meminspect: failed to prepare crashdump ELF header: %d\n", ret);
> +#endif
> +	/* if we have early notifiers registered, call them now */
> +	for (i = MEMINSPECT_ID_ELF; i < MEMINSPECT_ID_MAX; i++)
> +		if (inspect_entries[i].id)
> +			atomic_notifier_call_chain(&meminspect_notifier_list,
> +						   MEMINSPECT_NOTIFIER_ADD,
> +						   &inspect_entries[i]);

[Severity: Medium]
If a dynamic region is registered via meminspect_register_pa() before this
late_initcall runs, will this loop blindly issue a duplicate
MEMINSPECT_NOTIFIER_ADD notification for that region?

> +	meminspect_table_unlock();
> +
> +	pr_debug("Memory inspection table initialized\n");
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-meminspect-v3-v3-0-7aa5a0a74d5c@oss.qualcomm.com?part=1