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

[Thomas Garnier]

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>

>  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