Re: [Qemu-devel] [PATCH] multiboot: validate multiboot header address values

[Thomas Garnier]

On Tue, Sep 5, 2017 at 11:12 AM, Thomas Garnier <address@hidden> wrote:
> On Tue, Sep 5, 2017 at 10:49 AM, P J P <address@hidden> wrote:
>> From: Prasad J Pandit <address@hidden>
>>
>> While loading kernel via multiboot-v1 image, (flags & 0x00010000)
>> indicates that multiboot header contains valid addresses to load
>> the kernel image. These addresses are used to compute kernel
>> size and kernel text offset in the OS image. Validate these
>> address values to avoid an OOB access issue.
>>
>> Reported-by: Thomas Garnier <address@hidden>
>> Signed-off-by: Prasad J Pandit <address@hidden>
>> ---
>
> Looks good, tested.
>
> Tested-by: Thomas Garnier <address@hidden>

Btw, can you open a CVE for that? (and reference it in the commit).

>
>>  hw/i386/multiboot.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
>> index 6001f4caa2..c7b70c91d5 100644
>> --- a/hw/i386/multiboot.c
>> +++ b/hw/i386/multiboot.c
>> @@ -221,15 +221,34 @@ int load_multiboot(FWCfgState *fw_cfg,
>>          uint32_t mh_header_addr = ldl_p(header+i+12);
>>          uint32_t mh_load_end_addr = ldl_p(header+i+20);
>>          uint32_t mh_bss_end_addr = ldl_p(header+i+24);
>> +
>>          mh_load_addr = ldl_p(header+i+16);
>> +        if (mh_header_addr < mh_load_addr) {
>> +            fprintf(stderr, "invalid mh_load_addr address\n");
>> +            exit(1);
>> +        }
>> +
>>          uint32_t mb_kernel_text_offset = i - (mh_header_addr - 
>> mh_load_addr);
>>          uint32_t mb_load_size = 0;
>>          mh_entry_addr = ldl_p(header+i+28);
>>
>>          if (mh_load_end_addr) {
>> +            if (mh_bss_end_addr < mh_load_addr) {
>> +                fprintf(stderr, "invalid mh_bss_end_addr address\n");
>> +                exit(1);
>> +            }
>>              mb_kernel_size = mh_bss_end_addr - mh_load_addr;
>> +
>> +            if (mh_load_end_addr < mh_load_addr) {
>> +                fprintf(stderr, "invalid mh_load_end_addr address\n");
>> +                exit(1);
>> +            }
>>              mb_load_size = mh_load_end_addr - mh_load_addr;
>>          } else {
>> +            if (kernel_file_size < mb_kernel_text_offset) {
>> +                fprintf(stderr, "invalid kernel_file_size\n");
>> +                exit(1);
>> +            }
>>              mb_kernel_size = kernel_file_size - mb_kernel_text_offset;
>>              mb_load_size = mb_kernel_size;
>>          }
>> --
>> 2.13.5
>>
>
>
>
> --
> Thomas



-- 
Thomas