The code path with a manually set mh_load_addr in the Multiboot header
checks that load_end_addr <= load_addr, but the path where load_end_addr
is automatically detected if 0 is given in the header misses the
corresponding check. If the kernel binary size is larger than can fit in
the address space after load_addr, we ended up with a kernel_size that
is smaller than load_size, which means that we read the file into a too
small buffer.
Add a check to reject kernel files with such Multiboot headers.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
hw/i386/multiboot.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index b9064264d8..1e215bf8d3 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -247,6 +247,10 @@ int load_multiboot(FWCfgState *fw_cfg,
}
mb_load_size = kernel_file_size - mb_kernel_text_offset;
}
+ if (mb_load_size > UINT32_MAX - mh_load_addr) {
+ error_report("kernel does not fit in address space");
+ exit(1);
+ }
if (mh_bss_end_addr) {
if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
error_report("invalid bss_end_addr address");
--
2.13.6
Hi Kevin.
My comments are inline...
On 2018-03-14 10:32, Kevin Wolf wrote:
> The code path with a manually set mh_load_addr in the Multiboot header
> checks that load_end_addr <= load_addr, but the path where load_end_addr
> is automatically detected if 0 is given in the header misses the
> corresponding check.
1) The code checks that load_end_addr >= load_addr (before letting it
through).
2) load_end_addr is used only when it is read in as non-zero, so no
check is needed if zero. (It gets debug-printed even when zero, but is
used only to calculate mb_load_size and only when non-zero.)
> If the kernel binary size is larger than can fit in
> the address space after load_addr, we ended up with a kernel_size that
> is smaller than load_size, which means that we read the file into a too
> small buffer.
>
> Add a check to reject kernel files with such Multiboot headers.
Code itself looks fine.
Modulo above comments:
Reviewed-by: Jack Schwartz <jack.schwartz@oracle.com>
Thanks,
Jack
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> hw/i386/multiboot.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index b9064264d8..1e215bf8d3 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -247,6 +247,10 @@ int load_multiboot(FWCfgState *fw_cfg,
> }
> mb_load_size = kernel_file_size - mb_kernel_text_offset;
> }
> + if (mb_load_size > UINT32_MAX - mh_load_addr) {
> + error_report("kernel does not fit in address space");
> + exit(1);
> + }
> if (mh_bss_end_addr) {
> if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
> error_report("invalid bss_end_addr address");
Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
> Hi Kevin.
>
> My comments are inline...
>
> On 2018-03-14 10:32, Kevin Wolf wrote:
> > The code path with a manually set mh_load_addr in the Multiboot header
> > checks that load_end_addr <= load_addr, but the path where load_end_addr
> > is automatically detected if 0 is given in the header misses the
> > corresponding check.
> 1) The code checks that load_end_addr >= load_addr (before letting it
> through).
>
> 2) load_end_addr is used only when it is read in as non-zero, so no check is
> needed if zero. (It gets debug-printed even when zero, but is used only to
> calculate mb_load_size and only when non-zero.)
Oops, good point. I'll change the start of the commit message as follows:
The code path with a manually set mh_load_end_addr in the Multiboot
header checks that mh_load_end_addr >= mh_load_addr, but the path where
mh_load_end_addr is 0 in the header and therefore automatically
calculated from the file size misses the corresponding check.
Does this look better?
> > If the kernel binary size is larger than can fit in
> > the address space after load_addr, we ended up with a kernel_size that
> > is smaller than load_size, which means that we read the file into a too
> > small buffer.
> >
> > Add a check to reject kernel files with such Multiboot headers.
> Code itself looks fine.
>
> Modulo above comments:
> Reviewed-by: Jack Schwartz <jack.schwartz@oracle.com>
Thanks for your review of the series!
Kevin
On 03/15/18 08:54, Kevin Wolf wrote:
> Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
>> Hi Kevin.
>>
>> My comments are inline...
>>
>> On 2018-03-14 10:32, Kevin Wolf wrote:
>>> The code path with a manually set mh_load_addr in the Multiboot header
>>> checks that load_end_addr <= load_addr, but the path where load_end_addr
>>> is automatically detected if 0 is given in the header misses the
>>> corresponding check.
>> 1) The code checks that load_end_addr >= load_addr (before letting it
>> through).
>>
>> 2) load_end_addr is used only when it is read in as non-zero, so no check is
>> needed if zero. (It gets debug-printed even when zero, but is used only to
>> calculate mb_load_size and only when non-zero.)
> Oops, good point. I'll change the start of the commit message as follows:
>
> The code path with a manually set mh_load_end_addr in the Multiboot
> header checks that mh_load_end_addr >= mh_load_addr, but the path where
> mh_load_end_addr is 0 in the header and therefore automatically
> calculated from the file size misses the corresponding check.
>
> Does this look better?
mb_load_size is calculated from the file size, not mh_load_end_addr, so
I think you mean mb_load_size rather than mh_load_end_addr. Do you
intend to say:
The code path where mh_load_end_addr is non-zero in the Multiboot
header checks that mh_load_end_addr >= mh_load_addr and so
mb_load_size ischecked. However, mb_load_size is not checked when
calculated from thefile size, when mh_load_end_addr is 0.
Also, if this is what you intend to say, would the following code change
be more ofwhat you want:
Remove this:
mb_load_size = kernel_file_size - mb_kernel_text_offset;
}
- if (mb_load_size > UINT32_MAX - mh_load_addr) {
- error_report("kernel does not fit in address space");
- exit(1);
- }
if (mh_bss_end_addr) {
and instead do this a few lines further down:
mb_kernel_size = mh_bss_end_addr - mh_load_addr;
} else {
mb_kernel_size = mb_load_size;
}
+ if (mb_kernel_size > UINT32_MAX - mh_load_addr) {
+ error_report("kernel does not fit in address space");
+ exit(1);
+ }
mb_debug("multiboot: header_addr = %#x", mh_header_addr);
mb_debug("multiboot: load_addr = %#x", mh_load_addr);
The reason would be to include the bss area in the calculation, when
mh_bss_end_addr is non-zero.
Thanks,
Jack
>
>>> If the kernel binary size is larger than can fit in
>>> the address space after load_addr, we ended up with a kernel_size that
>>> is smaller than load_size, which means that we read the file into a too
>>> small buffer.
>>>
>>> Add a check to reject kernel files with such Multiboot headers.
>> Code itself looks fine.
>>
>> Modulo above comments:
>> Reviewed-by: Jack Schwartz <jack.schwartz@oracle.com>
> Thanks for your review of the series!
>
> Kevin
Am 15.03.2018 um 17:55 hat Jack Schwartz geschrieben:
> On 03/15/18 08:54, Kevin Wolf wrote:
> > Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
> > > Hi Kevin.
> > >
> > > My comments are inline...
> > >
> > > On 2018-03-14 10:32, Kevin Wolf wrote:
> > > > The code path with a manually set mh_load_addr in the Multiboot header
> > > > checks that load_end_addr <= load_addr, but the path where load_end_addr
> > > > is automatically detected if 0 is given in the header misses the
> > > > corresponding check.
> > > 1) The code checks that load_end_addr >= load_addr (before letting it
> > > through).
> > >
> > > 2) load_end_addr is used only when it is read in as non-zero, so no check is
> > > needed if zero. (It gets debug-printed even when zero, but is used only to
> > > calculate mb_load_size and only when non-zero.)
> > Oops, good point. I'll change the start of the commit message as follows:
> >
> > The code path with a manually set mh_load_end_addr in the Multiboot
> > header checks that mh_load_end_addr >= mh_load_addr, but the path where
> > mh_load_end_addr is 0 in the header and therefore automatically
> > calculated from the file size misses the corresponding check.
> >
> > Does this look better?
>
> mb_load_size is calculated from the file size, not mh_load_end_addr, so
> I think you mean mb_load_size rather than mh_load_end_addr. Do you intend
> to say:
>
> The code path where mh_load_end_addr is non-zero in the Multiboot
> header checks that mh_load_end_addr >= mh_load_addr and so
> mb_load_size ischecked. However, mb_load_size is not checked when
> calculated from thefile size, when mh_load_end_addr is 0.
Ok, technically that's more accurate.
> Also, if this is what you intend to say, would the following code change be
> more ofwhat you want:
>
> Remove this:
>
> mb_load_size = kernel_file_size - mb_kernel_text_offset;
> }
> - if (mb_load_size > UINT32_MAX - mh_load_addr) {
> - error_report("kernel does not fit in address space");
> - exit(1);
> - }
> if (mh_bss_end_addr) {
>
> and instead do this a few lines further down:
>
> mb_kernel_size = mh_bss_end_addr - mh_load_addr;
> } else {
> mb_kernel_size = mb_load_size;
> }
>
> + if (mb_kernel_size > UINT32_MAX - mh_load_addr) {
> + error_report("kernel does not fit in address space");
> + exit(1);
> + }
>
> mb_debug("multiboot: header_addr = %#x", mh_header_addr);
> mb_debug("multiboot: load_addr = %#x", mh_load_addr);
>
> The reason would be to include the bss area in the calculation, when
> mh_bss_end_addr is non-zero.
Ultimately, mb_load_size > mb_kernel_size is what kills us, so maybe we
could add an assertion for that.
But the reason why this condition could ever be true is the integer
overflow in this line:
if (mh_bss_end_addr < (mh_load_addr + mb_load_size))
It's generally better to check for integer overflows before they happen
than trying to infer them from the result. In fact, your condition
wouldn't catch the error of test scenario 9:
kernel_file_size = 0x2035
mh_header_addr = 0xfffff000
mh_load_addr = 0xfffff000
mh_load_end_addr = 0
mh_bss_end_addr = 0xfffff001
mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr)
= 0
mb_load_size = kernel_file_size - mb_kernel_text_offset
= 0x2035
> UINT32_MAX - mh_load_addr
mb_kernel_size = mh_bss_end_addr - mh_load_addr
= 1
< UINT32_MAX - mh_load_addr
Kevin
On 03/15/18 10:18, Kevin Wolf wrote:
> Am 15.03.2018 um 17:55 hat Jack Schwartz geschrieben:
>> On 03/15/18 08:54, Kevin Wolf wrote:
>>> Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
>>>> Hi Kevin.
>>>>
>>>> My comments are inline...
>>>>
>>>> On 2018-03-14 10:32, Kevin Wolf wrote:
>>>>> The code path with a manually set mh_load_addr in the Multiboot header
>>>>> checks that load_end_addr <= load_addr, but the path where load_end_addr
>>>>> is automatically detected if 0 is given in the header misses the
>>>>> corresponding check.
>>>> 1) The code checks that load_end_addr >= load_addr (before letting it
>>>> through).
>>>>
>>>> 2) load_end_addr is used only when it is read in as non-zero, so no check is
>>>> needed if zero. (It gets debug-printed even when zero, but is used only to
>>>> calculate mb_load_size and only when non-zero.)
>>> Oops, good point. I'll change the start of the commit message as follows:
>>>
>>> The code path with a manually set mh_load_end_addr in the Multiboot
>>> header checks that mh_load_end_addr >= mh_load_addr, but the path where
>>> mh_load_end_addr is 0 in the header and therefore automatically
>>> calculated from the file size misses the corresponding check.
>>>
>>> Does this look better?
>> mb_load_size is calculated from the file size, not mh_load_end_addr, so
>> I think you mean mb_load_size rather than mh_load_end_addr. Do you intend
>> to say:
>>
>> The code path where mh_load_end_addr is non-zero in the Multiboot
>> header checks that mh_load_end_addr >= mh_load_addr and so
>> mb_load_size is checked. However, mb_load_size is not checked when
>> calculated from the file size, when mh_load_end_addr is 0.
> Ok, technically that's more accurate.
OK. Note that I fixed a couple of missing spaces if you decide to use
it...
>> Also, if this is what you intend to say, would the following code change be
>> more ofwhat you want:
>>
>> Remove this:
>>
>> mb_load_size = kernel_file_size - mb_kernel_text_offset;
>> }
>> - if (mb_load_size > UINT32_MAX - mh_load_addr) {
>> - error_report("kernel does not fit in address space");
>> - exit(1);
>> - }
>> if (mh_bss_end_addr) {
>>
>> and instead do this a few lines further down:
>>
>> mb_kernel_size = mh_bss_end_addr - mh_load_addr;
>> } else {
>> mb_kernel_size = mb_load_size;
>> }
>>
>> + if (mb_kernel_size > UINT32_MAX - mh_load_addr) {
>> + error_report("kernel does not fit in address space");
>> + exit(1);
>> + }
>>
>> mb_debug("multiboot: header_addr = %#x", mh_header_addr);
>> mb_debug("multiboot: load_addr = %#x", mh_load_addr);
>>
>> The reason would be to include the bss area in the calculation, when
>> mh_bss_end_addr is non-zero.
> Ultimately, mb_load_size > mb_kernel_size is what kills us,
Right.
> so maybe we
> could add an assertion for that.
>
> But the reason why this condition could ever be true is the integer
> overflow in this line:
>
> if (mh_bss_end_addr < (mh_load_addr + mb_load_size))
>
> It's generally better to check for integer overflows before they happen
> than trying to infer them from the result. In fact, your condition
> wouldn't catch the error of test scenario 9:
>
> kernel_file_size = 0x2035
> mh_header_addr = 0xfffff000
> mh_load_addr = 0xfffff000
> mh_load_end_addr = 0
> mh_bss_end_addr = 0xfffff001
>
> mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr)
> = 0
>
> mb_load_size = kernel_file_size - mb_kernel_text_offset
> = 0x2035
> > UINT32_MAX - mh_load_addr
>
> mb_kernel_size = mh_bss_end_addr - mh_load_addr
> = 1
> < UINT32_MAX - mh_load_addr
OK, fair enough...
One other suggestion, would be to move what you added up by one line,
into the "else" clause. This moves the check to run only where it is
needed. This is cleaner as it does the check only where mb_load_size is
based on the kernel_file_size.
Reason: it doesn't make sense to do it for the "if" clause as it will
never be true anyway:
if (mh_load_end_addr) {
...
mb_load_size = mh_load_end_addr - mh_load_addr;
if (mb_load_size > UINT32_MAX - mh_load_addr) {
bomb();
Thanks,
Jack
>
> Kevin
>
© 2016 - 2025 Red Hat, Inc.