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

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

[Stefano Stabellini]

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@amd.com>
---
 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;
+    if ( payload_start > len || hdr->payload_length > len - payload_start )
+    {
+        printk("bzImage payload %#x@%#lx exceeds the %#lx byte image\n",
+               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. */
+    int err = bzimage_check(hdr, *image_len - (*image_start - image_base));
     unsigned long output_len;
 
     if ( err < 0 )
-- 
2.43.0

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

[Roger Pau Monné]

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@amd.com>
> ---
>  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.

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

[Jan Beulich]

On 21.10.2026 11:30, Roger Pau Monné wrote:
> On Tue, Oct 20, 2026 at 02:48:25PM -0700, Stefano Stabellini wrote:
>> @@ -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?

Without taking the type of .setup_sects into account, the suffix may appear
necessary. But yes, the field being a 8-bit one, the multiplication can't
overflow when done in 32 bits, so no suffix is needed (and generated code
may - at least in principle - be slightly better without the suffix).

Jan

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

[Stefano Stabellini]

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@amd.com>
---
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 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
index 66f648f311..41f19221ac 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) * 512 + hdr->payload_offset;
+    if ( payload_start > len || hdr->payload_length > len - payload_start )
+    {
+        printk("bzImage payload %#x@%#lx past the image size (%#lx)\n",
+               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. */
+    int err = bzimage_check(hdr, *image_len - (*image_start - image_base));
     unsigned long output_len;
 
     if ( err < 0 )
-- 
2.47.3

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

[Jan Beulich]

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@amd.com>
> ---
> 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

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

[Stefano Stabellini]

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@amd.com>
> > ---
> > 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@amd.com>
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@amd.com>
---
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

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

[Jan Beulich]

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@amd.com>
>>> ---
>>> 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@amd.com>
> 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@amd.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>