Re: [PATCH] x86/bzimage: Check that the payload lies within the image

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Tue, Oct 20, 2026 at 02:48:25PM -0700, Stefano Stabellini wrote:
> bzimage_check() doesn't validate setup_sects, payload_offset or
> payload_length against the image size.  A truncated or malformed bzImage
> therefore makes bzimage_headroom() and bzimage_parse() read beyond the
> end of the boot module, which shows up as a fatal page fault, a bogus
> headroom, or a decompression failure, rather than as a bad image.
> 
> Reject such a header up front.  bzimage_parse() passes bzimage_check() a
> length which includes the decompression headroom in front of the image,
> so subtract it to check against the image alone.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> ---
>  xen/arch/x86/bzimage.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
> index 66f648f311..a5c48755b6 100644
> --- a/xen/arch/x86/bzimage.c
> +++ b/xen/arch/x86/bzimage.c
> @@ -54,6 +54,8 @@ struct __packed setup_header {
>  
>  static __init int bzimage_check(struct setup_header *hdr, unsigned long len)
>  {
> +    unsigned long payload_start;
> +
>      if ( len < sizeof(struct setup_header) )
>          return 0;
>  
> @@ -65,6 +67,15 @@ static __init int bzimage_check(struct setup_header *hdr, 
> unsigned long len)
>             hdr->version >> 8, hdr->version & 0xff);
>          return -EINVAL;
>      }
> +
> +    payload_start = (hdr->setup_sects + 1) * 512UL + hdr->payload_offset;

Do you really need the UL, doesn't it get auto-promoted to unsigned long
already?

If there's some issue with the types, that requires the calculation to
be promoted top unsigned long, the other instances of this calculation
should also be adjusted.

> +    if ( payload_start > len || hdr->payload_length > len - payload_start )
> +    {
> +        printk("bzImage payload %#x@%#lx exceeds the %#lx byte image\n",

Maybe best wqorded as:

"... payload XXX past the image size (%#lx)"

I think the "byte image" wording is not very clear IMO.

> +               hdr->payload_length, payload_start, len);
> +        return -EINVAL;
> +    }
> +
>      return 1;
>  }
>  
> @@ -107,7 +118,8 @@ int __init bzimage_parse(void *image_base, void 
> **image_start,
>                           unsigned long *image_len)
>  {
>      struct setup_header *hdr = (struct setup_header *)(*image_start);
> -    int err = bzimage_check(hdr, *image_len);
> +    /* *image_len also counts the headroom ahead of *image_start. */

IMO you can remove the dereferences from the variable names, it just
makes the comment harder to reads for no real benefit, readers already
know by context you are referencing the contents, not the pointer
address.

Thanks, Roger.