[Qemu-devel] [PATCH 0/5] multiboot: Fix buffer overflow on invalid kernels

[Qemu-devel] [PATCH 0/5] multiboot: Fix buffer overflow on invalid kernels

[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 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

-- 
2.13.6

[Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space

[Kevin Wolf]

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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/i386/multiboot.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index b9064264d8..1e215bf8d3 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -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");
-- 
2.13.6

[Qemu-devel] [PATCH 2/5] multiboot: Check validity of mh_header_addr

[Kevin Wolf]

I 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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/i386/multiboot.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 1e215bf8d3..5bc0a2cddb 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -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;
-- 
2.13.6

[Qemu-devel] [PATCH 3/5] tests/multiboot: Test exit code for every qemu run

[Kevin Wolf]

Testing 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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/multiboot/run_test.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
index 0278148b43..bc9c3670af 100755
--- a/tests/multiboot/run_test.sh
+++ b/tests/multiboot/run_test.sh
@@ -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"
-- 
2.13.6

[Qemu-devel] [PATCH 4/5] tests/multiboot: Add tests for the a.out kludge

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 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

diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
index 36f01dc647..ed4225e7d1 100644
--- a/tests/multiboot/Makefile
+++ b/tests/multiboot/Makefile
@@ -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 $@ $^
diff --git a/tests/multiboot/aout_kludge.S b/tests/multiboot/aout_kludge.S
new file mode 100644
index 0000000000..52e8ebd766
--- /dev/null
+++ b/tests/multiboot/aout_kludge.S
@@ -0,0 +1,138 @@
+/*
+ * Copyright (c) 2018 Kevin Wolf <kwolf@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+.section multiboot
+
+#define MB_MAGIC 0x1badb002
+#define MB_FLAGS 0x10000
+#define MB_CHECKSUM -(MB_MAGIC + MB_FLAGS)
+
+.align  4
+.int    MB_MAGIC
+.int    MB_FLAGS
+.int    MB_CHECKSUM
+
+#define LAST_BYTE_VALUE 0xa5
+
+/*
+ * Order of fields in the a.out kludge header fields:
+ *
+ * header_addr
+ * load_addr
+ * load_end_addr
+ * bss_end_addr
+ * entry_addr
+ */
+#if SCENARIO == 1
+/* Well-behaved kernel file with explicit bss_end */
+.int    0x100000
+.int    0x100000
+.int    data_end
+.int    data_end
+.int    _start
+#elif SCENARIO == 2
+/* Well-behaved kernel file with default bss_end */
+.int    0x100000
+.int    0x100000
+.int    data_end
+.int    0
+.int    _start
+#elif SCENARIO == 3
+/* Well-behaved kernel file with default load_end */
+.int    0x100000
+.int    0x100000
+.int    0
+.int    0
+.int    _start
+#elif SCENARIO == 4
+/* Well-behaved kernel file with load_end < data_end and bss > data_end */
+#undef LAST_BYTE_VALUE
+#define LAST_BYTE_VALUE 0
+.int    0x100000
+.int    0x100000
+.int    code_end
+.int    0x140000
+.int    _start
+#elif SCENARIO == 5
+/* header < load */
+.int    0x10000
+.int    0x100000
+.int    data_end
+.int    data_end
+.int    _start
+#elif SCENARIO == 6
+/* load_end < load */
+.int    0x100000
+.int    0x100000
+.int    0x10000
+.int    data_end
+.int    _start
+#elif SCENARIO == 7
+/* header much larger than in reality with default load_end */
+.int    0x80000000
+.int    0x100000
+.int    0
+.int    data_end
+.int    _start
+#elif SCENARIO == 8
+/* bss_end < load_end - load (regression test for CVE-2018-7550) */
+.int    0x100000
+.int    0x100000
+.int    data_end
+.int    code_end
+.int    _start
+#elif SCENARIO == 9
+/* Default load_end_addr, load_addr + kernel_file_size > UINT32_MAX */
+.int    0xfffff000
+.int    0xfffff000
+.int    0
+.int    0xfffff001
+.int    _start
+#else
+#error Invalid SCENARIO
+#endif
+
+.section .text
+.global _start
+_start:
+    xor     %eax, %eax
+
+    cmpb    $LAST_BYTE_VALUE, last_byte
+    je      passed
+    or      $0x1, %eax
+passed:
+
+    /* Test device exit */
+    outl    %eax, $0xf4
+
+    cli
+    hlt
+    jmp .
+code_end:
+
+#if SCENARIO != 8
+.space 8192
+#endif
+
+last_byte:
+.byte 0xa5
+data_end:
diff --git a/tests/multiboot/aout_kludge.out b/tests/multiboot/aout_kludge.out
new file mode 100644
index 0000000000..031459275b
--- /dev/null
+++ b/tests/multiboot/aout_kludge.out
@@ -0,0 +1,42 @@
+
+
+
+=== Running test case: aout_kludge_1.bin  ===
+
+
+
+=== Running test case: aout_kludge_2.bin  ===
+
+
+
+=== Running test case: aout_kludge_3.bin  ===
+
+
+
+=== Running test case: aout_kludge_4.bin  ===
+
+
+
+=== Running test case: aout_kludge_5.bin  ===
+
+qemu-system-x86_64: invalid load_addr address
+
+
+=== Running test case: aout_kludge_6.bin  ===
+
+qemu-system-x86_64: invalid load_end_addr address
+
+
+=== Running test case: aout_kludge_7.bin  ===
+
+qemu-system-x86_64: invalid header_addr address
+
+
+=== Running test case: aout_kludge_8.bin  ===
+
+qemu-system-x86_64: invalid bss_end_addr address
+
+
+=== Running test case: aout_kludge_9.bin  ===
+
+qemu-system-x86_64: kernel does not fit in address space
diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
index bc9c3670af..6c33003e71 100755
--- a/tests/multiboot/run_test.sh
+++ b/tests/multiboot/run_test.sh
@@ -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
-- 
2.13.6

[Qemu-devel] [PATCH 5/5] tests/multiboot: Add .gitignore

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/multiboot/.gitignore | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 tests/multiboot/.gitignore

diff --git a/tests/multiboot/.gitignore b/tests/multiboot/.gitignore
new file mode 100644
index 0000000000..93ef99800b
--- /dev/null
+++ b/tests/multiboot/.gitignore
@@ -0,0 +1,3 @@
+*.bin
+*.elf
+test.out
-- 
2.13.6

Re: [Qemu-devel] [PATCH 5/5] tests/multiboot: Add .gitignore

[Eric Blake]

On 03/14/2018 12:32 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/multiboot/.gitignore | 3 +++
>   1 file changed, 3 insertions(+)
>   create mode 100644 tests/multiboot/.gitignore

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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

[Eric Blake]

On 03/14/2018 12:43 PM, Eric Blake wrote:
> On 03/14/2018 12:32 PM, Kevin Wolf wrote:
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   tests/multiboot/.gitignore | 3 +++
>>   1 file changed, 3 insertions(+)
>>   create mode 100644 tests/multiboot/.gitignore
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Huh - and I even proposed something similar a while back:
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg00305.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 0/5] multiboot: Fix buffer overflow on invalid kernels

[Jack Schwartz]

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:
     Reviewed-by: Jack Schwartz <jack.schwartz@oracle.com>

     Thanks,
     Jack

On 2018-03-14 10:32, Kevin Wolf wrote:
> 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
>

Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space

[Jack Schwartz]

Hi Kevin.

My comments are inline...

On 2018-03-14 10:32, Kevin Wolf wrote:
> 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.
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.)
> 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.
Code itself looks fine.

Modulo above comments:
     Reviewed-by: Jack Schwartz <jack.schwartz@oracle.com>

     Thanks,
     Jack
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   hw/i386/multiboot.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index b9064264d8..1e215bf8d3 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -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

[Kevin Wolf]

Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
> Hi Kevin.
> 
> My comments are inline...
> 
> On 2018-03-14 10:32, Kevin Wolf wrote:
> > 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.
> 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.)

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?

> > 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.
> Code itself looks fine.
> 
> Modulo above comments:
>     Reviewed-by: Jack Schwartz <jack.schwartz@oracle.com>

Thanks for your review of the series!

Kevin

Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space

[Jack Schwartz]

On 03/15/18 08:54, Kevin Wolf wrote:
> Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
>> Hi Kevin.
>>
>> My comments are inline...
>>
>> On 2018-03-14 10:32, Kevin Wolf wrote:
>>> 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.
>> 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.)
> 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?

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

>
>>> 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.
>> Code itself looks fine.
>>
>> Modulo above comments:
>>      Reviewed-by: Jack Schwartz <jack.schwartz@oracle.com>
> Thanks for your review of the series!
>
> Kevin

Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space

[Kevin Wolf]

Am 15.03.2018 um 17:55 hat Jack Schwartz geschrieben:
> On 03/15/18 08:54, Kevin Wolf wrote:
> > Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
> > > Hi Kevin.
> > > 
> > > My comments are inline...
> > > 
> > > On 2018-03-14 10:32, Kevin Wolf wrote:
> > > > 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.
> > > 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.)
> > 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?
> 
> 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.

Ok, technically that's more accurate.

> 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.

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_addr

Kevin

Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space

[Jack Schwartz]

On 03/15/18 10:18, Kevin Wolf wrote:
> Am 15.03.2018 um 17:55 hat Jack Schwartz geschrieben:
>> On 03/15/18 08:54, Kevin Wolf wrote:
>>> Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
>>>> Hi Kevin.
>>>>
>>>> My comments are inline...
>>>>
>>>> On 2018-03-14 10:32, Kevin Wolf wrote:
>>>>> 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.
>>>> 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.)
>>> 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?
>> 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 is checked.  However, mb_load_size is not checked when
>>    calculated from the file size, when mh_load_end_addr is 0.
> Ok, technically that's more accurate.
OK.  Note that I fixed a couple of missing spaces if you decide to use 
it...
>> 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.
> Ultimately, mb_load_size > mb_kernel_size is what kills us,
Right.
>   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_addr
OK, 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
>
> Kevin
>