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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.10.2026 20:43, Stefano Stabellini wrote:
> On Tue, 27 Oct 2026, Jan Beulich wrote:
>> 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).
> 
> What about this
> 
> 
> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> Subject: [PATCH v3] x86/bzimage: Check that the payload lies within the image
> 
> 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, including one whose payload offset
> overflows the 32-bit calculation bzimage_headroom() and bzimage_parse()
> use.  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>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>