multiboot: Fix buffer overflow on invalid kernels
cover multiboot: Fix buffer overflow on invalid kernels
Patch 1 fixes another Multiboot kernel validation bug that could cause QEMU to load the kernel image file into a too small buffer. Patch 2 adds another check to harden the code. The rest of the series adds Multiboot test cases for kernels using the a.out kludge, which is where the recent bugs were found.
Kevin Wolf (5):
multiboot: Reject kernels exceeding the address space
multiboot: Check validity of mh_header_addr tests/multiboot: Test exit code for every qemu run tests/multiboot: Add tests for the a.out kludge tests/multiboot: Add .gitignore
hw/i386/multiboot.c | 8 +++ tests/multiboot/.gitignore | 3 + tests/multiboot/Makefile | 22 +++++-- tests/multiboot/aout_kludge.S | 138 ++++++++++++++++++++++++++++++++++++++++ tests/multiboot/aout_kludge.out | 42 ++++++++++++ tests/multiboot/run_test.sh | 34 ++++++---- 6 files changed, 227 insertions(+), 20 deletions(-) create mode 100644 tests/multiboot/.gitignore create mode 100644 tests/multiboot/aout_kludge.S create mode 100644 tests/multiboot/aout_kludge.out
Signature · 1 line
2.13.6
PATCH 1/5 multiboot: Reject kernels exceeding the address space
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.
--- hw/i386/multiboot.c | 4 ++++ 1 file changed, 4 insertions(+)
hw/i386/multiboot.c +4 -0
@@ -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" );
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space
Hi Kevin.
My comments are inline...
Quoted 4 lines · Kevin Wolf
The code path with a manually set
mh_load_addrin the Multiboot header checks thatload_end_addr<=load_addr, but the path whereload_end_addris 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.)
Quoted 6 lines
If the kernel binary size is larger than can fit in
the address space afterload_addr, we ended up with akernel_sizethat is smaller thanload_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:
Thanks,
Jack
Quoted 21 lines
Signed-off-by Kevin Wolf <[email protected]>--- hw/i386/multiboot.c | 4 ++++ 1 file changed, 4 insertions(+)hw/i386/multiboot.c +4
-0@@ -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" );
Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space
Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
Quoted 9 lines
Hi Kevin.
My comments are inline...
Quoted 4 lines · Kevin Wolf
The code path with a manually set
mh_load_addrin the Multiboot header checks thatload_end_addr<=load_addr, but the path whereload_end_addris automatically detected if 0 is given in the header misses the corresponding check.
Quoted 6 lines
1) The code checks that
load_end_addr>=load_addr(before letting it through).2)
load_end_addris 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 calculatemb_load_sizeand 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?
Quoted 10 lines
Quoted 6 lines
If the kernel binary size is larger than can fit in
the address space afterload_addr, we ended up with akernel_sizethat is smaller thanload_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 <[email protected]>
Thanks for your review of the series!
Kevin
Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space
Quoted 16 lines · Kevin Wolf
Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
Quoted 9 lines
Hi Kevin.
My comments are inline...
Quoted 4 lines · Kevin Wolf
The code path with a manually set
mh_load_addrin the Multiboot header checks thatload_end_addr<=load_addr, but the path whereload_end_addris automatically detected if 0 is given in the header misses the corresponding check.Quoted 6 lines
1) The code checks that
load_end_addr>=load_addr(before letting it through).2)
load_end_addris 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 calculatemb_load_sizeand only when non-zero.)
Quoted 8 lines
Oops, good point. I'll change the start of the commit message as follows:
The code path with a manually set
mh_load_end_addrin the Multiboot header checks thatmh_load_end_addr>=mh_load_addr, but the path wheremh_load_end_addris 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
Quoted 14 lines
Quoted 11 lines
Quoted 7 lines
If the kernel binary size is larger than can fit in
the address space afterload_addr, we ended up with akernel_sizethat is smaller thanload_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 <[email protected]>Thanks for your review of the series!
Kevin
Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space
Am 15.03.2018 um 17:55 hat Jack Schwartz geschrieben:
Quoted 26 lines · Kevin Wolf
Quoted 16 lines
Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
Quoted 9 lines
Hi Kevin.
My comments are inline...
Quoted 4 lines · Kevin Wolf
The code path with a manually set
mh_load_addrin the Multiboot header checks thatload_end_addr<=load_addr, but the path whereload_end_addris automatically detected if 0 is given in the header misses the corresponding check.Quoted 6 lines
1) The code checks that
load_end_addr>=load_addr(before letting it through).2)
load_end_addris 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 calculatemb_load_sizeand only when non-zero.)Quoted 8 lines
Oops, good point. I'll change the start of the commit message as follows:
The code path with a manually set
mh_load_end_addrin the Multiboot header checks thatmh_load_end_addr>=mh_load_addr, but the path wheremh_load_end_addris 0 in the header and therefore automatically calculated from the file size misses the corresponding check.Does this look better?
Quoted 8 lines
mb_load_sizeis calculated from the file size, notmh_load_end_addr, so I think you meanmb_load_sizerather than mh_load_end_addr. Do you intend to say:The code path where
mh_load_end_addris non-zero in the Multiboot header checks thatmh_load_end_addr>=mh_load_addrand somb_load_sizeischecked. However,mb_load_sizeis not checked when calculated from thefile size, whenmh_load_end_addris 0.
Ok, technically that's more accurate.
Quoted 25 lines
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);
+ }
Quoted 5 lines
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_addris 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_addrKevin
Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space
Quoted 30 lines · Kevin Wolf
Am 15.03.2018 um 17:55 hat Jack Schwartz geschrieben:
Quoted 29 lines · Kevin Wolf
Quoted 16 lines
Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
Quoted 9 lines
Hi Kevin.
My comments are inline...
Quoted 4 lines · Kevin Wolf
The code path with a manually set
mh_load_addrin the Multiboot header checks thatload_end_addr<=load_addr, but the path whereload_end_addris automatically detected if 0 is given in the header misses the corresponding check.Quoted 6 lines
1) The code checks that
load_end_addr>=load_addr(before letting it through).2)
load_end_addris 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 calculatemb_load_sizeand only when non-zero.)Quoted 8 lines
Oops, good point. I'll change the start of the commit message as follows:
The code path with a manually set
mh_load_end_addrin the Multiboot header checks thatmh_load_end_addr>=mh_load_addr, but the path wheremh_load_end_addris 0 in the header and therefore automatically calculated from the file size misses the corresponding check.Does this look better?
mb_load_sizeis calculated from the file size, notmh_load_end_addr, so I think you meanmb_load_sizerather than mh_load_end_addr. Do you intend to say:
Quoted 5 lines
Quoted 4 lines
The code path where
mh_load_end_addris non-zero in the Multiboot header checks thatmh_load_end_addr>=mh_load_addrand somb_load_sizeis checked. However,mb_load_sizeis not checked when calculated from the file size, whenmh_load_end_addris 0.Ok, technically that's more accurate.
OK. Note that I fixed a couple of missing spaces if you decide to use it...
Quoted 25 lines
Quoted 25 lines
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);
+ }
Quoted 6 lines
Quoted 5 lines
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_addris non-zero.Ultimately,
mb_load_size>mb_kernel_sizeis what kills us,
Right.
Quoted 18 lines
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
Quoted 10 lines
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_addrOK, 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
Quoted 3 lines
Kevin
PATCH 2/5 multiboot: Check validity of mh_header_addr
In reply to Kevin WolfI couldn't find a case where this prevents something bad from happening that isn't already caught by other checks, but let's err on the safe side and check that mh_header_addr is as expected.
--- hw/i386/multiboot.c | 4 ++++ 1 file changed, 4 insertions(+)
hw/i386/multiboot.c +4 -0
@@ -229,6 +229,10 @@ int load_multiboot(FWCfgState *fw_cfg,error_report ("invalid load_addr address" );exit (1 ); } +if (mh_header_addr - mh_load_addr > i) { +error_report ("invalid header_addr address" ); +exit (1 ); + }uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);uint32_t mb_load_size =0 ;
Signature · 1 line
2.13.6
PATCH 3/5 tests/multiboot: Test exit code for every qemu run
In reply to Kevin WolfTesting the exit code only once after a whole group of tests has completed is not enough, it catches errors only in the very last qemu invocation. We need to have the check after each qemu run.
The logging and diff with the reference output is still done once per group to keep things more managable. This is not a problem because the log file accumulates the output of all runs.
--- tests/multiboot/run_test.sh | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
tests/multiboot/run_test.sh +12 -12
@@ -38,6 +38,17 @@ run_qemu() { ret=$? cat test.out >> test.log + + debugexit=$((ret & 0x1)) + ret=$((ret >> 1)) + + if [ $debugexit != 1 ]; then + printf %b "\e[31m ?? \e[0m $kernel $* (no debugexit used, exit code $ret)\n" + pass=0 + elif [ $ret != 0 ]; then + printf %b "\e[31mFAIL\e[0m $kernel $* (exit code $ret)\n" + pass=0 + fi } mmap() {
@@ -61,19 +72,8 @@ make all for t in mmap modules; do echo > test.log - $t - - debugexit=$((ret & 0x1)) - ret=$((ret >> 1)) pass=1 - - if [ $debugexit != 1 ]; then - printf %b "\e[31m ?? \e[0m $t (no debugexit used, exit code $ret)\n" - pass=0 - elif [ $ret != 0 ]; then - printf %b "\e[31mFAIL\e[0m $t (exit code $ret)\n" - pass=0 - fi + $t if ! diff $t.out test.log > /dev/null 2>&1; then printf %b "\e[31mFAIL\e[0m $t (output difference)\n"
Signature · 1 line
2.13.6
PATCH 4/5 tests/multiboot: Add tests for the a.out kludge
In reply to Kevin Wolf--- tests/multiboot/Makefile | 22 +++++-- tests/multiboot/aout_kludge.S | 138 ++++++++++++++++++++++++++++++++++++++++ tests/multiboot/aout_kludge.out | 42 ++++++++++++ tests/multiboot/run_test.sh | 10 ++- 4 files changed, 204 insertions(+), 8 deletions(-) create mode 100644 tests/multiboot/aout_kludge.S create mode 100644 tests/multiboot/aout_kludge.out
tests/multiboot/Makefile +16 -6
@@ -3,16 +3,26 @@ CCFLAGS=-m32 -Wall -Wextra -Werror -fno-stack-protector -nostdinc -fno-builtin ASFLAGS=-m32 LD=ld -LDFLAGS=-melf_i386 -T link.ld +LDFLAGS_ELF=-melf_i386 -T link.ld +LDFLAGS_BIN=-melf_i386 -T link.ld --oformat=binary LIBS=$(shell $(CC) $(CCFLAGS) -print-libgcc-file-name) -all: mmap.elf modules.elf +AOUT_KLUDGE_BIN=$(foreach x,$(shell seq 1 9),aout_kludge_$x.bin) -mmap.elf: start.o mmap.o libc.o - $(LD) $(LDFLAGS) -o $@ $^ $(LIBS) +all: mmap.elf modules.elf $(AOUT_KLUDGE_BIN) -modules.elf: start.o modules.o libc.o - $(LD) $(LDFLAGS) -o $@ $^ $(LIBS) +mmap.elf: start.o mmap.o libc.o link.ld + $(LD) $(LDFLAGS_ELF) -o $@ $^ $(LIBS) + +modules.elf: start.o modules.o libc.o link.ld + $(LD) $(LDFLAGS_ELF) -o $@ $^ $(LIBS) + +aout_kludge_%.bin: aout_kludge_%.o link.ld + $(LD) $(LDFLAGS_BIN) -o $@ $^ $(LIBS) + +.PRECIOUS: aout_kludge_%.o +aout_kludge_%.o: aout_kludge.S + $(CC) $(ASFLAGS) -DSCENARIO=$* -c -o $@ $^ %.o: %.c $(CC) $(CCFLAGS) -c -o $@ $^
tests/multiboot/aout_kludge.S new file +138 -0
138 lines not on this page · raw message
tests/multiboot/aout_kludge.out new file +42 -0
42 lines not on this page · raw message
tests/multiboot/run_test.sh +8 -2
@@ -34,7 +34,7 @@ run_qemu() { -device isa-debugcon,chardev=stdio \ -chardev file,path=test.out,id=stdio \ -device isa-debug-exit,iobase=0xf4,iosize=0x4 \ - "$@" + "$@" >> test.log 2>&1 ret=$? cat test.out >> test.log
@@ -67,9 +67,15 @@ modules() { run_qemu modules.elf -initrd "module.txt,module.txt argument,module.txt" } +aout_kludge() { + for i in $(seq 1 9); do + run_qemu aout_kludge_$i.bin + done +} + make all -for t in mmap modules; do +for t in mmap modules aout_kludge; do echo > test.log pass=1
Signature · 1 line
2.13.6
PATCH 5/5 tests/multiboot: Add .gitignore
In reply to Kevin Wolf--- tests/multiboot/.gitignore | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 tests/multiboot/.gitignore
tests/multiboot/.gitignore new file +3 -0
@@ -0,0 +1,3 @@ +*.bin +*.elf +test.out
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 5/5] tests/multiboot: Add .gitignore
Quoted 5 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- tests/multiboot/.gitignore | 3 +++ 1 file changed, 3 insertions(+)create mode 100644 tests/multiboot/.gitignore
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 5/5] tests/multiboot: Add .gitignore
Quoted 8 lines · Eric Blake
Quoted 5 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>---
tests/multiboot/.gitignore | 3 +++1 file changed, 3 insertions(+)create mode 100644 tests/multiboot/.gitignore
Reviewed-by Eric Blake <[email protected]>
Huh - and I even proposed something similar a while back:
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg00305.html
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Hi Kevin.
I see an issue with the commit message of patch 1; please see my reply to that patch for details. I fully understand patches 1,2,3, patch 4 except for some of the Makefile black magic, and patch 5 looks reasonable to me.
So, for patches 2,3,4,5:
Thanks,
Jack
Quoted 24 lines · Kevin Wolf
Patch 1 fixes another Multiboot kernel validation bug that could cause QEMU to load the kernel image file into a too small buffer. Patch 2 adds another check to harden the code. The rest of the series adds Multiboot test cases for kernels using the a.out kludge, which is where the recent bugs were found.
Kevin Wolf (5):
multiboot: Reject kernels exceeding the address space
multiboot: Check validity ofmh_header_addrtests/multiboot: Test exit code for every qemu run tests/multiboot: Add tests for the a.out kludge tests/multiboot: Add .gitignorehw/i386/multiboot.c | 8 +++ tests/multiboot/.gitignore | 3 + tests/multiboot/Makefile | 22 +++++-- tests/multiboot/aout_kludge.S | 138 ++++++++++++++++++++++++++++++++++++++++ tests/multiboot/aout_kludge.out | 42 ++++++++++++ tests/multiboot/run_test.sh | 34 ++++++---- 6 files changed, 227 insertions(+), 20 deletions(-)create mode 100644 tests/multiboot/.gitignore create mode 100644
tests/multiboot/aout_kludge.Screate mode 100644 tests/multiboot/aout_kludge.out