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

[Stefano Stabellini <stefano.stabellini@xxxxxxx>]

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>
---
Changes in v3:
- reject a payload offset whose 32-bit calculation wraps; the addend is
  at most 256 * 512, so a wrapped sum is smaller than payload_offset
- drop "the" from the error message

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
- reword the error message
- drop the dereferences from the comment
---
 xen/arch/x86/bzimage.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
index 66f648f311..a0cba65000 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 int payload_start;
+
     if ( len < sizeof(struct setup_header) )
         return 0;
 
@@ -65,6 +67,16 @@ 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;
+    if ( payload_start < hdr->payload_offset ||
+         payload_start > len || hdr->payload_length > len - payload_start )
+    {
+        printk("bzImage payload %#x@%#x past image size (%#lx)\n",
+               hdr->payload_length, payload_start, len);
+        return -EINVAL;
+    }
+
     return 1;
 }
 
@@ -107,7 +119,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. */
+    int err = bzimage_check(hdr, *image_len - (*image_start - image_base));
     unsigned long output_len;
 
     if ( err < 0 )
-- 
2.47.3