Re: [PATCH v3 26/26] meminspect: Add debug kinfo compatible driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing `return` statements after `dev_err_probe()` calls cause the driver to ignore size and base validations, leading to buffer overflows.
- [High] The driver attempts to use `rmem->priv` as a virtual address pointer, but it is never mapped or populated for this compatible string.
- [High] The driver uses `__pa()` instead of `__pa_symbol()` for statically allocated kernel symbols, which is invalid on architectures where kernel text/data is not in the linear map.
- [Medium] The shared memory checksum is updated non-atomically, exposing an intermediate, invalid state to concurrent external readers.
- [Low] Unconditional access to conditionally compiled `struct module` members causes build failures.
--

--- Patch [26]: [PATCH v3 26/26] meminspect: Add debug kinfo compatible driver ---
commit 8eff82a7a3c1597f04d6cc3418d6d9376460f4fc
Author: Eugen Hristev <ehristev@kernel.org>

meminspect: Add debug kinfo compatible driver

This commit adds a driver that copies registered memory regions to a shared
memory zone supplied via OF. It keeps only the addresses in a format that
is compatible with Kinfo.

> diff --git a/kernel/meminspect/kinfo.c b/kernel/meminspect/kinfo.c
> new file mode 100644
> index 0000000000000..7451c13bc3163
> --- /dev/null
> +++ b/kernel/meminspect/kinfo.c

[ ... ]

> +static void update_kernel_all_info(struct kernel_all_info *all_info)
> +{
> +	struct kernel_info *info;
> +	u32 *checksum_info;
> +	int index;
> +
> +	all_info->magic_number = DEBUG_KINFO_MAGIC;
> +	all_info->combined_checksum = 0;
> +
> +	info = &all_info->info;
> +	checksum_info = (u32 *)info;
> +	for (index = 0; index < sizeof(*info) / sizeof(u32); index++)
> +		all_info->combined_checksum ^= checksum_info[index];

[Severity: Medium]
Could this non-atomic update expose an invalid intermediate state to
concurrent external readers?

If a crash occurs concurrently with the execution of this loop, the exposed
checksum in the mapped shared memory might be zero or partially computed.
Would it be safer to compute the checksum locally and then write it to the
shared memory once?

> +}
> +
> +static void __maybe_unused register_kinfo_region(void *priv,
> +						 const struct inspect_entry *e)
> +{
> +	struct debug_kinfo *kinfo = priv;
> +	struct kernel_all_info *all_info = kinfo->all_info_addr;
> +	struct kernel_info *info = &all_info->info;
> +	struct uts_namespace *uts;
> +	u64 paddr;
> +
> +	if (e->pa)
> +		paddr = e->pa;
> +	else
> +		paddr = __pa(e->va);

[Severity: High]
Is __pa() the correct translation macro to use here for statically allocated
kernel symbols?

On architectures like x86_64, kernel symbols reside in the kernel image
mapping outside the direct linear mapping. Calling __pa() on these
addresses can trigger a virtual bug on kernel panic if CONFIG_DEBUG_VIRTUAL
is enabled. Should this use __pa_symbol() instead?

> +
> +	switch (e->id) {

[ ... ]

> +static int debug_kinfo_probe(struct platform_device *pdev)
> +{
> +	struct kernel_all_info *all_info;
> +	struct device *dev = &pdev->dev;
> +	struct reserved_mem *rmem;
> +	struct debug_kinfo *kinfo;
> +	struct kernel_info *info;
> +
> +	rmem = of_reserved_mem_lookup(dev->of_node);
> +	if (!rmem)
> +		return dev_err_probe(dev, -ENODEV, "no such reserved mem of node name %s\n",
> +			      dev->of_node->name);
> +
> +	/* Need to wait for reserved memory to be mapped */
> +	if (!rmem->priv)
> +		return -EPROBE_DEFER;

[Severity: High]
Will rmem->priv ever be populated for this compatible string?

The core framework only populates rmem->priv if there is a
RESERVEDMEM_OF_DECLARE callback that explicitly maps the memory. Since
there appears to be no such callback for google,debug-kinfo, this might
cause the driver to enter an endless probe defer loop.

> +
> +	if (!rmem->base || !rmem->size)
> +		dev_err_probe(dev, -EINVAL, "unexpected reserved memory\n");
> +
> +	if (rmem->size < sizeof(struct kernel_all_info))
> +		dev_err_probe(dev, -EINVAL, "reserved memory size too small\n");

[Severity: High]
Are these dev_err_probe calls missing return statements?

Without returning an error, execution proceeds. If the reserved memory is
indeed too small, the subsequent memset will write out-of-bounds of the
reserved memory region.

> +
> +	kinfo = devm_kzalloc(dev, sizeof(*kinfo), GFP_KERNEL);
> +	if (!kinfo)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, kinfo);
> +
> +	kinfo->dev = dev;
> +	kinfo->all_info_addr = rmem->priv;
> +	kinfo->all_info_size = rmem->size;
> +
> +	all_info = kinfo->all_info_addr;
> +
> +	memset(all_info, 0, sizeof(struct kernel_all_info));
> +	info = &all_info->info;
> +	info->enabled_all = IS_ENABLED(CONFIG_KALLSYMS_ALL);
> +	info->enabled_absolute_percpu = IS_ENABLED(CONFIG_KALLSYMS_ABSOLUTE_PERCPU);
> +	info->enabled_base_relative = IS_ENABLED(CONFIG_KALLSYMS_BASE_RELATIVE);
> +	info->enabled_cfi_clang = IS_ENABLED(CONFIG_CFI_CLANG);
> +	info->name_len = KSYM_NAME_LEN;
> +	info->bit_per_long = BITS_PER_LONG;
> +	info->module_name_len = MODULE_NAME_LEN;
> +	info->symbol_len = KSYM_SYMBOL_LEN;
> +	info->thread_size = THREAD_SIZE;
> +	info->enabled_modules_tree_lookup = IS_ENABLED(CONFIG_MODULES_TREE_LOOKUP);
> +	info->mod_mem_offset = offsetof(struct module, mem);
> +	info->mod_kallsyms_offset = offsetof(struct module, kallsyms);

[Severity: Low]
Will this cause compilation errors if CONFIG_MODULES or CONFIG_KALLSYMS
are disabled?

The struct module type is incomplete if CONFIG_MODULES is disabled, and
the kallsyms member only exists if CONFIG_KALLSYMS is enabled.

> +
> +	kinfo->nb.notifier_call = kinfo_notifier_cb;

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