| Message ID | 20170905174942.3094-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 5, 2017 at 10:49 AM, P J P <ppandit@redhat.com> 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. 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 <thgarnie@google.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- Looks good, tested. Tested-by: Thomas Garnier <thgarnie@google.com> > 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 >
On Tue, Sep 5, 2017 at 11:12 AM, Thomas Garnier <thgarnie@google.com> wrote: > On Tue, Sep 5, 2017 at 10:49 AM, P J P <ppandit@redhat.com> 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. 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 <thgarnie@google.com> >> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >> --- > > Looks good, tested. > > Tested-by: Thomas Garnier <thgarnie@google.com> 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
+-- On Tue, 5 Sep 2017, Thomas Garnier wrote --+ | Btw, can you open a CVE for that? (and reference it in the commit). Done; Sent revised patch v1. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
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; }