Re: [PATCH v14 02/19] unwind_user/sframe: Add support for reading .sframe headers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing validation of shdr.abi_arch allows parsing SFrame data from mismatched architectures.
- [Medium] VMA addresses are incorrectly evaluated as boolean flags to check for zero-length inputs.
- [Low] Mixing goto error handling with scoped_guard scope-based cleanup.
--

commit afe7c9eb6f0e6fc3f73d0b6a555f8d3926cf6ccf
Author: Josh Poimboeuf <jpoimboe@kernel.org>

unwind_user/sframe: Add support for reading .sframe headers

This commit introduces basic infrastructure for unwinding user space stacks
with SFrame. It adds support for parsing the .sframe section header.

Link: https://lore.kernel.org/all/f27e8463783febfa0dabb0432a3dd6be8ad98412.1737511963.git.jpoimboe@kernel.org/

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> new file mode 100644
> index 0000000000000..c128c1eeaca20
> --- /dev/null
> +++ b/kernel/unwind/sframe.c
[ ... ]
> +static int sframe_read_header(struct sframe_section *sec)
> +{
> +	unsigned long header_end, fdes_start, fdes_end, fres_start, fres_end;
> +	struct sframe_header shdr;
> +	unsigned int num_fdes;
> +
> +	if (copy_from_user(&shdr, (void __user *)sec->sframe_start, sizeof(shdr))) {
> +		dbg("header usercopy failed\n");
> +		return -EFAULT;
> +	}
> +
> +	if (shdr.preamble.magic != SFRAME_MAGIC ||
> +	    shdr.preamble.version != SFRAME_VERSION_3 ||
> +	    !(shdr.preamble.flags & SFRAME_F_FDE_SORTED) ||
> +	    !(shdr.preamble.flags & SFRAME_F_FDE_FUNC_START_PCREL) ||
> +	    shdr.auxhdr_len) {
> +		dbg("bad/unsupported sframe header\n");
> +		return -EINVAL;
> +	}

Should we also validate shdr.abi_arch to ensure it matches the expected
architecture?

If malformed or foreign-architecture SFrame data is passed in (for instance,
with an unexpected endianness or register mapping), the kernel might interpret
it incorrectly and generate invalid stack traces.

Furthermore, accepting arbitrary values here could risk ABI calcification if
userspace starts supplying invalid values that we later want to reject.

[ ... ]
> +int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
> +		       unsigned long text_start, unsigned long text_end)
> +{
> +	struct vm_area_struct *sframe_vma, *text_vma;
> +	struct mm_struct *mm = current->mm;
> +	struct sframe_section *sec;
> +	int ret;
> +
> +	if (!sframe_start || !sframe_end || !text_start || !text_end) {

Is it safe to use VMA start addresses as boolean flags?

Since vm_start can legitimately be zero in certain configurations (if
mmap_min_addr permits), evaluating !sframe_start or !text_start could
incorrectly reject perfectly valid mappings at address 0.

Also, to check for zero-length sections, would it be better to explicitly use
comparisons like sframe_start >= sframe_end instead?

> +		dbg("zero-length sframe/text address\n");
> +		return -EINVAL;
> +	}
> +
> +	scoped_guard(mmap_read_lock, mm) {
[ ... ]
> +	sec = kzalloc(sizeof(*sec), GFP_KERNEL);
> +	if (!sec)
> +		return -ENOMEM;
> +
> +	sec->sframe_start	= sframe_start;
> +	sec->sframe_end		= sframe_end;
> +	sec->text_start		= text_start;
> +	sec->text_end		= text_end;
> +
> +	ret = sframe_read_header(sec);
> +	if (ret)
> +		goto err_free;

Is it acceptable to mix goto error handling with scoped_guard() cleanup
helpers in the same function?

The kernel's cleanup subsystem guidelines generally suggest that usage of
goto and cleanup helpers shouldn't be mixed in the same routine.

Could sec be allocated using __free(kfree) to avoid the goto entirely?

> +
> +	/* TODO nowhere to store it yet - just free it and return an error */
> +	ret = -ENOSYS;
> +
> +err_free:
> +	free_section(sec);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505121718.3572346-1-jremus@linux.ibm.com?part=2