Re: [PATCH v8 05/11] module, sframe: Add sframe support for modules

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] unwind: Integer overflow in SFrame header parsing on 32-bit bypasses bounds checks, causing OOB write in sort()
--

commit 37859a60480c542bd986b5fe41b863323e42fd98
Author: Dylan Hatch <dylanbhatch@google.com>

module, sframe: Add sframe support for modules

This commit adds SFrame table support to struct module and enables SFrame
PC lookups for incoming modules. It parses the .sframe section and sorts
the FDEs if they are not already sorted.

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c
[ ... ]
> @@ -471,10 +517,72 @@ void __init init_sframe_table(void)
[ ... ]
> +static void sframe_sort_fdes(struct sframe_section *sec)
> +{
> +	void *fdes = (void *)sec->fdes_start;
> +	size_t num_fdes = sec->num_fdes;
> +
> +	if (sec->fdes_sorted)
> +		return;
> +
> +	sort(fdes, num_fdes, sizeof(struct sframe_fde_v3),
> +	     sframe_sort_cmp_fde, sframe_sort_swap_fde);

[Severity: High]
With module parsing introduced, could this sort() call lead to an
out-of-bounds write if num_fdes is maliciously inflated?

Looking at how sframe_read_header() validates the incoming headers:

    num_fdes   = shdr->num_fdes;
    fdes_start = header_end + shdr->fdes_off;
    fdes_end   = fdes_start + (num_fdes * sizeof(struct sframe_fde_v3));
    
    fres_start = header_end + shdr->fres_off;
    fres_end   = fres_start + shdr->fre_len;
    
    if (fres_start < fdes_end || fres_end > sec->sframe_end || 
        fres_end < sec->sframe_start) {
            pr_debug("inconsistent fde/fre offsets\n");
            return -EINVAL;
    }

On 32-bit architectures, sizeof(struct sframe_fde_v3) evaluates to 16.
If an incoming module sets shdr->num_fdes to a large value like 0x10000000,
the multiplication num_fdes * 16 overflows the 32-bit integer and wraps
around to 0, resulting in fdes_end equaling fdes_start.

If the module also sets shdr->fres_off equal to shdr->fdes_off and
shdr->fre_len to 0, fres_start and fres_end will equal fdes_end. This
evaluates all the bounds checks to false, fully bypassing validation.

When sframe_sort_fdes() is subsequently called on this parsed module data,
it would invoke sort() with the attacker-controlled num_fdes (for example,
268,435,456 elements). Does this need overflow checking (such as size_mul)
to prevent out-of-bounds memory corruption during module load?

Additionally, could fde->fres_off in __find_fre() similarly overflow when
added to sec->fres_start on 32-bit architectures, bypassing bounds checks
and causing out-of-bounds reads during stack unwinding?

> +	sec->fdes_sorted = true;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007114335.440322-1-dylanbhatch@google.com?part=5