Re: [PATCH v3 13/26] kernel/configs: Register dynamic information into meminspect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing Cleanup / Use-After-Free on module unload
- [High] Invalid Virtual-to-Physical Translation for module/kernel symbols
- [Medium] Double dispatch of MEMINSPECT_NOTIFIER_ADD for built-in initializations
--

--- Patch [13]: [PATCH v3 13/26] kernel/configs: Register dynamic information into meminspect ---
commit 69a88fbb66bd86af76d457f0ad80ecb85abfe14b
Author: Eugen Hristev <ehristev@kernel.org>

kernel/configs: Register dynamic information into meminspect

This commit registers kernel_config_data information into the memory
inspection table. It captures the start and end markers of the region 
to assist debugging tools.

> diff --git a/kernel/configs.c b/kernel/configs.c
> --- a/kernel/configs.c
> +++ b/kernel/configs.c
> @@ -64,6 +65,11 @@ static int __init ikconfig_init(void)
>  
>  	proc_set_size(entry, &kernel_config_data_end - &kernel_config_data);
>  
> +	/* Register 8 bytes before and after, to catch the marker too */
> +	meminspect_lock_register_id_va(MEMINSPECT_ID_CONFIG,
> +				       (void *)&kernel_config_data - 8,
> +				       &kernel_config_data_end - &kernel_config_data + 16);

[Severity: High]
Since meminspect_lock_register_id_va() translates this address using
virt_to_phys(), is it safe to pass module or kernel symbol addresses here?

When built as a module, kernel_config_data resides in vmalloc space where
virt_to_phys() is invalid. Even when built-in, kernel symbols on some
architectures (like x86_64 or arm64) reside outside the linear mapping and
typically require __pa_symbol() instead.

[Severity: Medium]
Will this result in a double dispatch of the MEMINSPECT_NOTIFIER_ADD event
when the feature is built-in?

When CONFIG_IKCONFIG=y, this initialization runs during the device_initcall
phase, which fires the notifier. Later, meminspect_prepare_table() runs at
late_initcall and appears to fire the same event again for all populated
entries:

kernel/meminspect/meminspect.c:meminspect_prepare_table() {
    ...
    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]);
    ...
}

Won't consumers of the notifier chain receive two ADD events for the same
entry?

[Severity: High]
If this is built as a module, does ikconfig_cleanup() need to unregister
the dynamically added entry?

kernel/configs.c:ikconfig_cleanup() {
    remove_proc_entry("config.gz", NULL);
}

If the module is unloaded and the entry is not unregistered, won't the
meminspect table retain a dangling pointer to freed module memory, leading
to a use-after-free during subsequent table traversals?

> +
>  	return 0;
>  }

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