multiboot: check mh_load_end_addr address field

[Qemu-devel] [PATCH] multiboot: check mh_load_end_addr address field

Posted by P J P 7 years, 7 months ago

From: Prasad J Pandit <pjp@fedoraproject.org>

While loading kernel via multiboot-v1 image, (flags & 0x00010000)
indicates that multiboot header contains valid addresses to load
the kernel image. In that, end of the data segment address
'mh_load_end_addr' should be less than the bss segment address
'mh_bss_end_addr'. Add check to validate that.

Reported-by: CERT CC <cert.cc@orange.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/i386/multiboot.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 46d9c68bf5..d16e32bf4a 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -227,6 +227,10 @@ int load_multiboot(FWCfgState *fw_cfg,
             fprintf(stderr, "invalid mh_load_addr address\n");
             exit(1);
         }
+        if (mh_load_end_addr > mh_bss_end_addr) {
+            fprintf(stderr, "invalid mh_load_end_addr address\n");
+            exit(1);
+        }
 
         uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
         uint32_t mb_load_size = 0;
-- 
2.14.3

Re: [Qemu-devel] [PATCH] multiboot: check mh_load_end_addr address field

Posted by P J P 7 years, 7 months ago

+-- On Wed, 28 Feb 2018, P J P wrote --+
| While loading kernel via multiboot-v1 image, (flags & 0x00010000)
| indicates that multiboot header contains valid addresses to load
| the kernel image. In that, end of the data segment address
| 'mh_load_end_addr' should be less than the bss segment address
| 'mh_bss_end_addr'. Add check to validate that.
| 
| +        if (mh_load_end_addr > mh_bss_end_addr) {
| +            fprintf(stderr, "invalid mh_load_end_addr address\n");
| +            exit(1);
| +        }
|  

Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] multiboot: check mh_load_end_addr address field

Posted by Jack Schwartz 7 years, 7 months ago

Hi Prasad.

The Multiboot Spec will allow for a zero bss end address.  (Please see 
section 3.1.3 at https://www.gnu.org/software/grub/manual/multiboot/  . 
) For a zero bss end address, this patch will not do the right thing.

I had proposed some patches to properly handle zero bss end address, but 
they got delayed.  However, I will be re-sending updated patches out 
later today.  Please stay tuned...

     Thanks,
     Jack

On 02/27/18 11:48, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While loading kernel via multiboot-v1 image, (flags & 0x00010000)
> indicates that multiboot header contains valid addresses to load
> the kernel image. In that, end of the data segment address
> 'mh_load_end_addr' should be less than the bss segment address
> 'mh_bss_end_addr'. Add check to validate that.
>
> Reported-by: CERT CC <cert.cc@orange.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>   hw/i386/multiboot.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index 46d9c68bf5..d16e32bf4a 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -227,6 +227,10 @@ int load_multiboot(FWCfgState *fw_cfg,
>               fprintf(stderr, "invalid mh_load_addr address\n");
>               exit(1);
>           }
> +        if (mh_load_end_addr > mh_bss_end_addr) {
> +            fprintf(stderr, "invalid mh_load_end_addr address\n");
> +            exit(1);
> +        }
>   
>           uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
>           uint32_t mb_load_size = 0;