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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2026 23:10, 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>
> ---
> Changes in v2:
> - drop the UL suffix: setup_sects is 8 bits wide, so the multiplication
>   cannot overflow, and the offset is now computed the same way as where
>   bzimage_headroom() and bzimage_parse() use it

Upon looking again, I think this may have been a mistake:

> @@ -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) * 512 + hdr->payload_offset;

The multiplication can't overflow, but the addition can. Question is what
the behavior should be in that case: Adding back the UL suffix would allow
payload_start to go beyond 4Gb, but that may not be what is intended. The
alternative would be to check for overflow in the calculation (easiest by
way of using __builtin_add_overflow(), yet then needing adjustment when
backporting to 4.20).

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

As we prefer messages to be terse, may I suggest to also drop "the"?

Jan