[Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Jack Schwartz]

Properly account for the possibility of multiboot kernels with a zero
bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
header field.

Do some cleanup to multiboot.c as well:
- Remove some unused variables.
- Use more intuitive header names when displaying fields in messages.
- Change fprintf(stderr...) to error_report

Testing:
  1) Ran the "make check" test suite.
  2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
     grub multiboot.elf test "kernel" by modifying source.)  Verified
     with gdb that new code that reads addresses/offsets from multiboot
     header was accessed.
  3) Booted multiboot kernel with non-zero bss_end_addr.
  4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
  5) Code has soaked in an internal repo for two months.

	Thanks,
	Jack

Jack Schwartz (4):
  multiboot: bss_end_addr can be zero
  multiboot: Remove unused variables from multiboot.c
  multiboot: Use header names when displaying fields
  multiboot: fprintf(stderr...) -> error_report()

 hw/i386/multiboot.c | 77 ++++++++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 39 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH QEMU v1 1/4] multiboot: bss_end_addr can be zero

[Jack Schwartz]

The multiboot spec (https://www.gnu.org/software/grub/manual/multiboot/),
section 3.1.3, allows for bss_end_addr to be zero.

A zero bss_end_addr signifies there is no .bss section.

Suggested-by: Daniel Kiper <daniel.kiper@oracle.com>
Signed-off-by: Jack Schwartz <jack.schwartz@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 hw/i386/multiboot.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index c7b70c9..ff2733d 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -233,12 +233,6 @@ int load_multiboot(FWCfgState *fw_cfg,
         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);
@@ -249,8 +243,16 @@ int load_multiboot(FWCfgState *fw_cfg,
                 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;
+            mb_load_size = kernel_file_size - mb_kernel_text_offset;
+        }
+        if (mh_bss_end_addr) {
+            if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
+                fprintf(stderr, "invalid mh_bss_end_addr address\n");
+                exit(1);
+            }
+            mb_kernel_size = mh_bss_end_addr - mh_load_addr;
+        } else {
+            mb_kernel_size = mb_load_size;
         }
 
         /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
-- 
1.8.3.1

[Qemu-devel] [PATCH QEMU v1 2/4] multiboot: Remove unused variables from multiboot.c

[Jack Schwartz]

Remove unused variables: mh_mode_type, mh_width, mh_height, mh_depth

Signed-off-by: Jack Schwartz <jack.schwartz@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 hw/i386/multiboot.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index ff2733d..964feaf 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -255,12 +255,6 @@ int load_multiboot(FWCfgState *fw_cfg,
             mb_kernel_size = mb_load_size;
         }
 
-        /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
-        uint32_t mh_mode_type = ldl_p(header+i+32);
-        uint32_t mh_width = ldl_p(header+i+36);
-        uint32_t mh_height = ldl_p(header+i+40);
-        uint32_t mh_depth = ldl_p(header+i+44); */
-
         mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
         mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
         mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
-- 
1.8.3.1

[Qemu-devel] [PATCH QEMU v1 3/4] multiboot: Use header names when displaying fields

[Jack Schwartz]

Refer to field names when displaying fields in printf and debug statements.

Signed-off-by: Jack Schwartz <jack.schwartz@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 hw/i386/multiboot.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 964feaf..818728b 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -224,7 +224,7 @@ int load_multiboot(FWCfgState *fw_cfg,
 
         mh_load_addr = ldl_p(header+i+16);
         if (mh_header_addr < mh_load_addr) {
-            fprintf(stderr, "invalid mh_load_addr address\n");
+            fprintf(stderr, "invalid load_addr address\n");
             exit(1);
         }
 
@@ -234,7 +234,7 @@ int load_multiboot(FWCfgState *fw_cfg,
 
         if (mh_load_end_addr) {
             if (mh_load_end_addr < mh_load_addr) {
-                fprintf(stderr, "invalid mh_load_end_addr address\n");
+                fprintf(stderr, "invalid load_end_addr address\n");
                 exit(1);
             }
             mb_load_size = mh_load_end_addr - mh_load_addr;
@@ -247,7 +247,7 @@ int load_multiboot(FWCfgState *fw_cfg,
         }
         if (mh_bss_end_addr) {
             if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
-                fprintf(stderr, "invalid mh_bss_end_addr address\n");
+                fprintf(stderr, "invalid bss_end_addr address\n");
                 exit(1);
             }
             mb_kernel_size = mh_bss_end_addr - mh_load_addr;
@@ -255,10 +255,10 @@ int load_multiboot(FWCfgState *fw_cfg,
             mb_kernel_size = mb_load_size;
         }
 
-        mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
-        mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
-        mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
-        mb_debug("multiboot: mh_bss_end_addr = %#x\n", mh_bss_end_addr);
+        mb_debug("multiboot: header_addr = %#x\n", mh_header_addr);
+        mb_debug("multiboot: load_addr = %#x\n", mh_load_addr);
+        mb_debug("multiboot: load_end_addr = %#x\n", mh_load_end_addr);
+        mb_debug("multiboot: bss_end_addr = %#x\n", mh_bss_end_addr);
         mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x\n",
                  mb_load_size, mh_load_addr);
 
@@ -361,7 +361,7 @@ int load_multiboot(FWCfgState *fw_cfg,
     stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
     stl_p(bootinfo + MBI_MMAP_ADDR,   ADDR_E820_MAP);
 
-    mb_debug("multiboot: mh_entry_addr = %#x\n", mh_entry_addr);
+    mb_debug("multiboot: entry_addr = %#x\n", mh_entry_addr);
     mb_debug("           mb_buf_phys   = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
     mb_debug("           mod_start     = "TARGET_FMT_plx"\n", mbs.mb_buf_phys + mbs.offset_mods);
     mb_debug("           mb_mods_count = %d\n", mbs.mb_mods_count);
-- 
1.8.3.1

[Qemu-devel] [PATCH QEMU v1 4/4] multiboot: fprintf(stderr...) -> error_report()

[Jack Schwartz]

Change all fprintf(stderr...) calls in hw/i386/multiboot.c to call
error_report() instead, including the mb_debug macro.  Remove the "\n"
from strings passed to all modified calls, since error_report() appends
one.

Signed-off-by: Jack Schwartz <jack.schwartz@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 hw/i386/multiboot.c | 55 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 818728b..d9a0a95 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -31,12 +31,13 @@
 #include "hw/loader.h"
 #include "elf.h"
 #include "sysemu/sysemu.h"
+#include "qemu/error-report.h"
 
 /* Show multiboot debug output */
 //#define DEBUG_MULTIBOOT
 
 #ifdef DEBUG_MULTIBOOT
-#define mb_debug(a...) fprintf(stderr, ## a)
+#define mb_debug(a...) error_report(a)
 #else
 #define mb_debug(a...)
 #endif
@@ -137,7 +138,7 @@ static void mb_add_mod(MultibootState *s,
     stl_p(p + MB_MOD_END,     end);
     stl_p(p + MB_MOD_CMDLINE, cmdline_phys);
 
-    mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx"\n",
+    mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx,
              s->mb_mods_count, start, end);
 
     s->mb_mods_count++;
@@ -179,12 +180,12 @@ int load_multiboot(FWCfgState *fw_cfg,
     if (!is_multiboot)
         return 0; /* no multiboot */
 
-    mb_debug("qemu: I believe we found a multiboot image!\n");
+    mb_debug("qemu: I believe we found a multiboot image!");
     memset(bootinfo, 0, sizeof(bootinfo));
     memset(&mbs, 0, sizeof(mbs));
 
     if (flags & 0x00000004) { /* MULTIBOOT_HEADER_HAS_VBE */
-        fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
+        error_report("qemu: multiboot knows VBE. we don't.");
     }
     if (!(flags & 0x00010000)) { /* MULTIBOOT_HEADER_HAS_ADDR */
         uint64_t elf_entry;
@@ -193,7 +194,7 @@ int load_multiboot(FWCfgState *fw_cfg,
         fclose(f);
 
         if (((struct elf64_hdr*)header)->e_machine == EM_X86_64) {
-            fprintf(stderr, "Cannot load x86-64 image, give a 32bit one.\n");
+            error_report("Cannot load x86-64 image, give a 32bit one.");
             exit(1);
         }
 
@@ -201,7 +202,7 @@ int load_multiboot(FWCfgState *fw_cfg,
                                &elf_low, &elf_high, 0, I386_ELF_MACHINE,
                                0, 0);
         if (kernel_size < 0) {
-            fprintf(stderr, "Error while loading elf kernel\n");
+            error_report("Error while loading elf kernel");
             exit(1);
         }
         mh_load_addr = elf_low;
@@ -210,12 +211,13 @@ int load_multiboot(FWCfgState *fw_cfg,
 
         mbs.mb_buf = g_malloc(mb_kernel_size);
         if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
-            fprintf(stderr, "Error while fetching elf kernel from rom\n");
+            error_report("Error while fetching elf kernel from rom");
             exit(1);
         }
 
-        mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
-                  mb_kernel_size, (size_t)mh_entry_addr);
+        mb_debug("qemu: loading multiboot-elf kernel "
+                 "(%#x bytes) with entry %#zx",
+                 mb_kernel_size, (size_t)mh_entry_addr);
     } else {
         /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
         uint32_t mh_header_addr = ldl_p(header+i+12);
@@ -224,7 +226,7 @@ int load_multiboot(FWCfgState *fw_cfg,
 
         mh_load_addr = ldl_p(header+i+16);
         if (mh_header_addr < mh_load_addr) {
-            fprintf(stderr, "invalid load_addr address\n");
+            error_report("invalid load_addr address");
             exit(1);
         }
 
@@ -234,20 +236,20 @@ int load_multiboot(FWCfgState *fw_cfg,
 
         if (mh_load_end_addr) {
             if (mh_load_end_addr < mh_load_addr) {
-                fprintf(stderr, "invalid load_end_addr address\n");
+                error_report("invalid load_end_addr address");
                 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");
+                error_report("invalid kernel_file_size");
                 exit(1);
             }
             mb_load_size = kernel_file_size - mb_kernel_text_offset;
         }
         if (mh_bss_end_addr) {
             if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
-                fprintf(stderr, "invalid bss_end_addr address\n");
+                error_report("invalid bss_end_addr address");
                 exit(1);
             }
             mb_kernel_size = mh_bss_end_addr - mh_load_addr;
@@ -255,17 +257,17 @@ int load_multiboot(FWCfgState *fw_cfg,
             mb_kernel_size = mb_load_size;
         }
 
-        mb_debug("multiboot: header_addr = %#x\n", mh_header_addr);
-        mb_debug("multiboot: load_addr = %#x\n", mh_load_addr);
-        mb_debug("multiboot: load_end_addr = %#x\n", mh_load_end_addr);
-        mb_debug("multiboot: bss_end_addr = %#x\n", mh_bss_end_addr);
-        mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x\n",
+        mb_debug("multiboot: header_addr = %#x", mh_header_addr);
+        mb_debug("multiboot: load_addr = %#x", mh_load_addr);
+        mb_debug("multiboot: load_end_addr = %#x", mh_load_end_addr);
+        mb_debug("multiboot: bss_end_addr = %#x", mh_bss_end_addr);
+        mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x",
                  mb_load_size, mh_load_addr);
 
         mbs.mb_buf = g_malloc(mb_kernel_size);
         fseek(f, mb_kernel_text_offset, SEEK_SET);
         if (fread(mbs.mb_buf, 1, mb_load_size, f) != mb_load_size) {
-            fprintf(stderr, "fread() failed\n");
+            error_report("fread() failed");
             exit(1);
         }
         memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
@@ -319,10 +321,10 @@ int load_multiboot(FWCfgState *fw_cfg,
             hwaddr c = mb_add_cmdline(&mbs, tmpbuf);
             if ((next_space = strchr(tmpbuf, ' ')))
                 *next_space = '\0';
-            mb_debug("multiboot loading module: %s\n", tmpbuf);
+            mb_debug("multiboot loading module: %s", tmpbuf);
             mb_mod_length = get_image_size(tmpbuf);
             if (mb_mod_length < 0) {
-                fprintf(stderr, "Failed to open file '%s'\n", tmpbuf);
+                error_report("Failed to open file '%s'", tmpbuf);
                 exit(1);
             }
 
@@ -333,7 +335,7 @@ int load_multiboot(FWCfgState *fw_cfg,
             mb_add_mod(&mbs, mbs.mb_buf_phys + offs,
                        mbs.mb_buf_phys + offs + mb_mod_length, c);
 
-            mb_debug("mod_start: %p\nmod_end:   %p\n  cmdline: "TARGET_FMT_plx"\n",
+            mb_debug("mod_start: %p\nmod_end:   %p\n  cmdline: "TARGET_FMT_plx,
                      (char *)mbs.mb_buf + offs,
                      (char *)mbs.mb_buf + offs + mb_mod_length, c);
             initrd_filename = next_initrd+1;
@@ -361,10 +363,11 @@ int load_multiboot(FWCfgState *fw_cfg,
     stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
     stl_p(bootinfo + MBI_MMAP_ADDR,   ADDR_E820_MAP);
 
-    mb_debug("multiboot: entry_addr = %#x\n", mh_entry_addr);
-    mb_debug("           mb_buf_phys   = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
-    mb_debug("           mod_start     = "TARGET_FMT_plx"\n", mbs.mb_buf_phys + mbs.offset_mods);
-    mb_debug("           mb_mods_count = %d\n", mbs.mb_mods_count);
+    mb_debug("multiboot: entry_addr = %#x", mh_entry_addr);
+    mb_debug("           mb_buf_phys   = "TARGET_FMT_plx, mbs.mb_buf_phys);
+    mb_debug("           mod_start     = "TARGET_FMT_plx,
+             mbs.mb_buf_phys + mbs.offset_mods);
+    mb_debug("           mb_mods_count = %d", mbs.mb_mods_count);
 
     /* save bootinfo off the stack */
     mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
-- 
1.8.3.1

[Qemu-devel] ping: Re: [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Jack Schwartz]

Hi everyone.

Ping!  Please review.

Patchwork links:

1/4 multiboot: bss_end_addr can be zero
http://patchwork.ozlabs.org/patch/852049/

2/4 multiboot: Remove unused variables from multiboot.c
http://patchwork.ozlabs.org/patch/852045/

3/4 multiboot: Use header names when displaying fields
http://patchwork.ozlabs.org/patch/852046/

4/4 multiboot: fprintf(stderr...) -> error_report()
http://patchwork.ozlabs.org/patch/852051/

     Thanks,
     Jack


On 12/21/17 09:25, Jack Schwartz wrote:
> Properly account for the possibility of multiboot kernels with a zero
> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
> kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
> header field.
>
> Do some cleanup to multiboot.c as well:
> - Remove some unused variables.
> - Use more intuitive header names when displaying fields in messages.
> - Change fprintf(stderr...) to error_report
>
> Testing:
>    1) Ran the "make check" test suite.
>    2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
>       grub multiboot.elf test "kernel" by modifying source.)  Verified
>       with gdb that new code that reads addresses/offsets from multiboot
>       header was accessed.
>    3) Booted multiboot kernel with non-zero bss_end_addr.
>    4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
>    5) Code has soaked in an internal repo for two months.
>
> 	Thanks,
> 	Jack
>
> Jack Schwartz (4):
>    multiboot: bss_end_addr can be zero
>    multiboot: Remove unused variables from multiboot.c
>    multiboot: Use header names when displaying fields
>    multiboot: fprintf(stderr...) -> error_report()
>
>   hw/i386/multiboot.c | 77 ++++++++++++++++++++++++++---------------------------
>   1 file changed, 38 insertions(+), 39 deletions(-)
>

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Kevin Wolf]

Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
> Properly account for the possibility of multiboot kernels with a zero
> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
> kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
> header field.
> 
> Do some cleanup to multiboot.c as well:
> - Remove some unused variables.
> - Use more intuitive header names when displaying fields in messages.
> - Change fprintf(stderr...) to error_report

There are some conflicts with Anatol's (CCed) multiboot series:
https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html

None if these should be hard to resolve, but it would be good if you
could agree with each other whose patch series should come first, and
then the other one should be rebased on top of that.

> Testing:
>   1) Ran the "make check" test suite.
>   2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
>      grub multiboot.elf test "kernel" by modifying source.)  Verified
>      with gdb that new code that reads addresses/offsets from multiboot
>      header was accessed.
>   3) Booted multiboot kernel with non-zero bss_end_addr.
>   4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
>   5) Code has soaked in an internal repo for two months.

Can you integrate your test kernel from 2) in tests/multiboot/ so we can
keep this as a regression test?

> Jack Schwartz (4):
>   multiboot: bss_end_addr can be zero
>   multiboot: Remove unused variables from multiboot.c
>   multiboot: Use header names when displaying fields
>   multiboot: fprintf(stderr...) -> error_report()

Apart from the conflicts, the patches look good to me.

Kevin

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Jack Schwartz]

Hi Kevin and Anatol.

Kevin, thanks for your review.

More inline below...

On 01/15/18 07:54, Kevin Wolf wrote:
> Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
>> Properly account for the possibility of multiboot kernels with a zero
>> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
>> kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
>> header field.
>>
>> Do some cleanup to multiboot.c as well:
>> - Remove some unused variables.
>> - Use more intuitive header names when displaying fields in messages.
>> - Change fprintf(stderr...) to error_report
> There are some conflicts with Anatol's (CCed) multiboot series:
> https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html
>
> None if these should be hard to resolve, but it would be good if you
> could agree with each other whose patch series should come first, and
> then the other one should be rebased on top of that.
Anatol,

from my side, there are pros and cons to either patch set going in 
first, but advantages to either are pretty negligible.  Pro for you 
going first: I can use the constants you will define in header files.  
Pro for me going first: your merge should be about the same as if you 
went first (since my changes are small, more localized and affect only 
multiboot.c) and my merge will be easier.

What are your thoughts?
> Testing:
>    1) Ran the "make check" test suite.
>    2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
>       grub multiboot.elf test "kernel" by modifying source.)  Verified
>       with gdb that new code that reads addresses/offsets from multiboot
>       header was accessed.
>    3) Booted multiboot kernel with non-zero bss_end_addr.
>    4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
>    5) Code has soaked in an internal repo for two months.
> Can you integrate your test kernel from 2) in tests/multiboot/ so we can
> keep this as a regression test?
Kevin and alias,

Before I proceed with adding my multiboot test file, I'll clarify here 
that I started with a version from the grub2 tree.  In that file I 
expanded a header file, also from the same tree.  Neither file had any 
license header, though the tree I got them from (Dated October 2017) 
contains the GNU GPLv3 license file.

I'll need to check with my company before I can say I can deliver this 
file.  If I deliver it, I'll add a header stating the GPL license, that 
it came from grub2 and to check its repository for contributors.
>> Jack Schwartz (4):
>>    multiboot: bss_end_addr can be zero
>>    multiboot: Remove unused variables from multiboot.c
>>    multiboot: Use header names when displaying fields
>>    multiboot: fprintf(stderr...) -> error_report()
> Apart from the conflicts, the patches look good to me.
     Thanks,
     Jack
>
> Kevin
>

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Kevin Wolf]

Am 17.01.2018 um 21:06 hat Jack Schwartz geschrieben:
> Before I proceed with adding my multiboot test file, I'll clarify here that
> I started with a version from the grub2 tree.  In that file I expanded a
> header file, also from the same tree.  Neither file had any license header,
> though the tree I got them from (Dated October 2017) contains the GNU GPLv3
> license file.

I see. QEMU as a whole is GPLv2, so this might be a problem. It's
probably not as bad as merging GPLv3 code into QEMU proper because it's
a standalone test kernel that I suppose could have a different license.
But IANAL and maybe it's safer not to go there.

Maybe it would be less hassle to just reimplement the tests, based on
the MIT licensed tests that are already in tests/multiboot/.

Kevin

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Daniel P. Berrange]

On Thu, Jan 18, 2018 at 12:35:00PM +0100, Kevin Wolf wrote:
> Am 17.01.2018 um 21:06 hat Jack Schwartz geschrieben:
> > Before I proceed with adding my multiboot test file, I'll clarify here that
> > I started with a version from the grub2 tree.  In that file I expanded a
> > header file, also from the same tree.  Neither file had any license header,
> > though the tree I got them from (Dated October 2017) contains the GNU GPLv3
> > license file.
> 
> I see. QEMU as a whole is GPLv2, so this might be a problem. It's
> probably not as bad as merging GPLv3 code into QEMU proper because it's
> a standalone test kernel that I suppose could have a different license.
> But IANAL and maybe it's safer not to go there.

As long as the GPLv3 code is not linked / otherwise combined with any
of the GPLv2 code in QEMU that will be ok. Since it is a self-contained
test kernel, that would not be an issue in this case - it only interfaces
to QEMU via the x86 machine ABI.

It just means that the QEMU source tar.gz will be under a conjunction
of licenses  "GPLv2 and GPLv2+ and GPLv3 and ...all our other licenss..."

The resulting binaries for emulators/tools will still be GPLv2 as they're
only linking in the GPLv2 + GPLv2+ source, never linking the GPlv3 test
image.

For simplicity of understanding though, it could be desirable to avoid
this if not an unreasonable amount of extra work

> Maybe it would be less hassle to just reimplement the tests, based on
> the MIT licensed tests that are already in tests/multiboot/.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Anatol Pomozov]

Hello Jack

On Wed, Jan 17, 2018 at 12:06 PM, Jack Schwartz
<jack.schwartz@oracle.com> wrote:
> Hi Kevin and Anatol.
>
> Kevin, thanks for your review.
>
> More inline below...
>
> On 01/15/18 07:54, Kevin Wolf wrote:
>>
>> Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
>>>
>>> Properly account for the possibility of multiboot kernels with a zero
>>> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
>>> kernels without a bss section, by allowing a zeroed bss_end_addr
>>> multiboot
>>> header field.
>>>
>>> Do some cleanup to multiboot.c as well:
>>> - Remove some unused variables.
>>> - Use more intuitive header names when displaying fields in messages.
>>> - Change fprintf(stderr...) to error_report
>>
>> There are some conflicts with Anatol's (CCed) multiboot series:
>> https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html
>>
>> None if these should be hard to resolve, but it would be good if you
>> could agree with each other whose patch series should come first, and
>> then the other one should be rebased on top of that.
>
> Anatol,
>
> from my side, there are pros and cons to either patch set going in first,
> but advantages to either are pretty negligible.  Pro for you going first: I
> can use the constants you will define in header files.  Pro for me going
> first: your merge should be about the same as if you went first (since my
> changes are small, more localized and affect only multiboot.c) and my merge
> will be easier.
>
> What are your thoughts?

Please move ahead with your patches. I'll rebase my changes on top of yours.

>>
>> Testing:
>>    1) Ran the "make check" test suite.
>>    2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
>>       grub multiboot.elf test "kernel" by modifying source.)  Verified
>>       with gdb that new code that reads addresses/offsets from multiboot
>>       header was accessed.
>>    3) Booted multiboot kernel with non-zero bss_end_addr.
>>    4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages
>> worked.
>>    5) Code has soaked in an internal repo for two months.
>> Can you integrate your test kernel from 2) in tests/multiboot/ so we can
>> keep this as a regression test?
>
> Kevin and alias,
>
> Before I proceed with adding my multiboot test file, I'll clarify here that
> I started with a version from the grub2 tree.  In that file I expanded a
> header file, also from the same tree.  Neither file had any license header,
> though the tree I got them from (Dated October 2017) contains the GNU GPLv3
> license file.
>
> I'll need to check with my company before I can say I can deliver this file.
> If I deliver it, I'll add a header stating the GPL license, that it came
> from grub2 and to check its repository for contributors.
>>>
>>> Jack Schwartz (4):
>>>    multiboot: bss_end_addr can be zero
>>>    multiboot: Remove unused variables from multiboot.c
>>>    multiboot: Use header names when displaying fields
>>>    multiboot: fprintf(stderr...) -> error_report()
>>
>> Apart from the conflicts, the patches look good to me.
>
>     Thanks,
>     Jack
>>
>>
>> Kevin
>>
>

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Jack Schwartz]

Hi Anatol, Daniel and Kevin.

On 01/19/18 10:36, Anatol Pomozov wrote:
> Hello Jack
>
> On Wed, Jan 17, 2018 at 12:06 PM, Jack Schwartz
> <jack.schwartz@oracle.com> wrote:
>> Hi Kevin and Anatol.
>>
>> Kevin, thanks for your review.
>>
>> More inline below...
>>
>> On 01/15/18 07:54, Kevin Wolf wrote:
>>> Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
>>>> Properly account for the possibility of multiboot kernels with a zero
>>>> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
>>>> kernels without a bss section, by allowing a zeroed bss_end_addr
>>>> multiboot
>>>> header field.
>>>>
>>>> Do some cleanup to multiboot.c as well:
>>>> - Remove some unused variables.
>>>> - Use more intuitive header names when displaying fields in messages.
>>>> - Change fprintf(stderr...) to error_report
>>> There are some conflicts with Anatol's (CCed) multiboot series:
>>> https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html
>>>
>>> None if these should be hard to resolve, but it would be good if you
>>> could agree with each other whose patch series should come first, and
>>> then the other one should be rebased on top of that.
>> Anatol,
>>
>> from my side, there are pros and cons to either patch set going in first,
>> but advantages to either are pretty negligible.  Pro for you going first: I
>> can use the constants you will define in header files.  Pro for me going
>> first: your merge should be about the same as if you went first (since my
>> changes are small, more localized and affect only multiboot.c) and my merge
>> will be easier.
>>
>> What are your thoughts?
> Please move ahead with your patches. I'll rebase my changes on top of yours.
OK.  I'm consulting with my company's legal department and waiting for 
their approvals for delivery of a test "kernel".  I'll get in touch will 
everyone once I have an answer about that.  I anticipate about a week 
before taking next steps to deliver.

Kevin and Daniel, thanks for your inputs on this issue (different 
subthread), which I have forwarded to our legal department for review.

     Thanks,
     Jack
>
>>> Testing:
>>>     1) Ran the "make check" test suite.
>>>     2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
>>>        grub multiboot.elf test "kernel" by modifying source.)  Verified
>>>        with gdb that new code that reads addresses/offsets from multiboot
>>>        header was accessed.
>>>     3) Booted multiboot kernel with non-zero bss_end_addr.
>>>     4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages
>>> worked.
>>>     5) Code has soaked in an internal repo for two months.
>>> Can you integrate your test kernel from 2) in tests/multiboot/ so we can
>>> keep this as a regression test?
>> Kevin and alias,
>>
>> Before I proceed with adding my multiboot test file, I'll clarify here that
>> I started with a version from the grub2 tree.  In that file I expanded a
>> header file, also from the same tree.  Neither file had any license header,
>> though the tree I got them from (Dated October 2017) contains the GNU GPLv3
>> license file.
>>
>> I'll need to check with my company before I can say I can deliver this file.
>> If I deliver it, I'll add a header stating the GPL license, that it came
>> from grub2 and to check its repository for contributors.
>>>> Jack Schwartz (4):
>>>>     multiboot: bss_end_addr can be zero
>>>>     multiboot: Remove unused variables from multiboot.c
>>>>     multiboot: Use header names when displaying fields
>>>>     multiboot: fprintf(stderr...) -> error_report()
>>> Apart from the conflicts, the patches look good to me.
>>      Thanks,
>>      Jack
>>>
>>> Kevin
>>>

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Daniel P. Berrange]

On Fri, Jan 19, 2018 at 04:18:07PM -0800, Jack Schwartz wrote:
> Hi Anatol, Daniel and Kevin.
> 
> On 01/19/18 10:36, Anatol Pomozov wrote:
> > Hello Jack
> > 
> > On Wed, Jan 17, 2018 at 12:06 PM, Jack Schwartz
> > <jack.schwartz@oracle.com> wrote:
> > > Hi Kevin and Anatol.
> > > 
> > > Kevin, thanks for your review.
> > > 
> > > More inline below...
> > > 
> > > On 01/15/18 07:54, Kevin Wolf wrote:
> > > > Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
> > > > > Properly account for the possibility of multiboot kernels with a zero
> > > > > bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
> > > > > kernels without a bss section, by allowing a zeroed bss_end_addr
> > > > > multiboot
> > > > > header field.
> > > > > 
> > > > > Do some cleanup to multiboot.c as well:
> > > > > - Remove some unused variables.
> > > > > - Use more intuitive header names when displaying fields in messages.
> > > > > - Change fprintf(stderr...) to error_report
> > > > There are some conflicts with Anatol's (CCed) multiboot series:
> > > > https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html
> > > > 
> > > > None if these should be hard to resolve, but it would be good if you
> > > > could agree with each other whose patch series should come first, and
> > > > then the other one should be rebased on top of that.
> > > Anatol,
> > > 
> > > from my side, there are pros and cons to either patch set going in first,
> > > but advantages to either are pretty negligible.  Pro for you going first: I
> > > can use the constants you will define in header files.  Pro for me going
> > > first: your merge should be about the same as if you went first (since my
> > > changes are small, more localized and affect only multiboot.c) and my merge
> > > will be easier.
> > > 
> > > What are your thoughts?
> > Please move ahead with your patches. I'll rebase my changes on top of yours.
> OK.  I'm consulting with my company's legal department and waiting for their
> approvals for delivery of a test "kernel".  I'll get in touch will everyone
> once I have an answer about that.  I anticipate about a week before taking
> next steps to deliver.
> 
> Kevin and Daniel, thanks for your inputs on this issue (different
> subthread), which I have forwarded to our legal department for review.

FWIW, I don't think it needs to be your responsibility to decide this. I
think the QEMU community / maintainer taking the patches needs to decide
whether it is acceptable / desirable.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Jack Schwartz]

Hi Kevin.

On 2018-01-15 07:54, Kevin Wolf wrote:
> Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
>> Properly account for the possibility of multiboot kernels with a zero
>> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
>> kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
>> header field.
>>
>> Do some cleanup to multiboot.c as well:
>> - Remove some unused variables.
>> - Use more intuitive header names when displaying fields in messages.
>> - Change fprintf(stderr...) to error_report
> There are some conflicts with Anatol's (CCed) multiboot series:
> https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html
>
> None if these should be hard to resolve, but it would be good if you
> could agree with each other whose patch series should come first, and
> then the other one should be rebased on top of that.
>
>> Testing:
>>    1) Ran the "make check" test suite.
>>    2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
>>       grub multiboot.elf test "kernel" by modifying source.)  Verified
>>       with gdb that new code that reads addresses/offsets from multiboot
>>       header was accessed.
>>    3) Booted multiboot kernel with non-zero bss_end_addr.
>>    4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
>>    5) Code has soaked in an internal repo for two months.
> Can you integrate your test kernel from 2) in tests/multiboot/ so we can
> keep this as a regression test?
If need be, would you be willing to accept updated versions of these 
patches (with another review, of course) without the test file?  I will 
deliver the test file later once I get company approvals.  I don't want 
the test file to continue holding everything up in the meantime.

Patchwork links, for reference:

1/4 multiboot: bss_end_addr can be zero
http://patchwork.ozlabs.org/patch/852049/

2/4 multiboot: Remove unused variables from multiboot.c
http://patchwork.ozlabs.org/patch/852045/

3/4 multiboot: Use header names when displaying fields
http://patchwork.ozlabs.org/patch/852046/

4/4 multiboot: fprintf(stderr...) -> error_report()
http://patchwork.ozlabs.org/patch/852051/


     Thanks,
     Jack
>
>> Jack Schwartz (4):
>>    multiboot: bss_end_addr can be zero
>>    multiboot: Remove unused variables from multiboot.c
>>    multiboot: Use header names when displaying fields
>>    multiboot: fprintf(stderr...) -> error_report()
> Apart from the conflicts, the patches look good to me.
>
> Kevin
>

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Kevin Wolf]

Am 02.03.2018 um 20:32 hat Jack Schwartz geschrieben:
> Hi Kevin.
> 
> On 2018-01-15 07:54, Kevin Wolf wrote:
> > Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
> > > Properly account for the possibility of multiboot kernels with a zero
> > > bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
> > > kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
> > > header field.
> > > 
> > > Do some cleanup to multiboot.c as well:
> > > - Remove some unused variables.
> > > - Use more intuitive header names when displaying fields in messages.
> > > - Change fprintf(stderr...) to error_report
> > There are some conflicts with Anatol's (CCed) multiboot series:
> > https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html
> > 
> > None if these should be hard to resolve, but it would be good if you
> > could agree with each other whose patch series should come first, and
> > then the other one should be rebased on top of that.
> > 
> > > Testing:
> > >    1) Ran the "make check" test suite.
> > >    2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
> > >       grub multiboot.elf test "kernel" by modifying source.)  Verified
> > >       with gdb that new code that reads addresses/offsets from multiboot
> > >       header was accessed.
> > >    3) Booted multiboot kernel with non-zero bss_end_addr.
> > >    4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
> > >    5) Code has soaked in an internal repo for two months.
> > Can you integrate your test kernel from 2) in tests/multiboot/ so we can
> > keep this as a regression test?
> If need be, would you be willing to accept updated versions of these patches
> (with another review, of course) without the test file?  I will deliver the
> test file later once I get company approvals.  I don't want the test file to
> continue holding everything up in the meantime.

Sure, let's move forward with what we have now. Please keep me CCed when
you send a new version and I'll give it a review and hopeuflly get it
merged.

Kevin

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Jack Schwartz]

Hi Kevin and everyone.

On 2018-03-05 00:13, Kevin Wolf wrote:
> Am 02.03.2018 um 20:32 hat Jack Schwartz geschrieben:
>> Hi Kevin.
>>
>> On 2018-01-15 07:54, Kevin Wolf wrote:
>>> Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
>>>> Properly account for the possibility of multiboot kernels with a zero
>>>> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
>>>> kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
>>>> header field.
>>>>
>>>> Do some cleanup to multiboot.c as well:
>>>> - Remove some unused variables.
>>>> - Use more intuitive header names when displaying fields in messages.
>>>> - Change fprintf(stderr...) to error_report
>>> There are some conflicts with Anatol's (CCed) multiboot series:
>>> https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html
>>>
>>> None if these should be hard to resolve, but it would be good if you
>>> could agree with each other whose patch series should come first, and
>>> then the other one should be rebased on top of that.
>>>
>>>> Testing:
>>>>     1) Ran the "make check" test suite.
>>>>     2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
>>>>        grub multiboot.elf test "kernel" by modifying source.)  Verified
>>>>        with gdb that new code that reads addresses/offsets from multiboot
>>>>        header was accessed.
>>>>     3) Booted multiboot kernel with non-zero bss_end_addr.
>>>>     4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
>>>>     5) Code has soaked in an internal repo for two months.
>>> Can you integrate your test kernel from 2) in tests/multiboot/ so we can
>>> keep this as a regression test?
>> If need be, would you be willing to accept updated versions of these patches
>> (with another review, of course) without the test file?  I will deliver the
>> test file later once I get company approvals.  I don't want the test file to
>> continue holding everything up in the meantime.
> Sure, let's move forward with what we have now. Please keep me CCed when
> you send a new version and I'll give it a review and hopeuflly get it
> merged.
>
> Kevin
Thanks, Kevin.

Patches have not changed, and I verified they still work on a current 
repo.  (Multiboot.c has had a one-line change regarding a header file, 
so I rebuilt and re-tested to make sure.)

Links again, for your reference:

1/4 multiboot: bss_end_addr can be zero
http://patchwork.ozlabs.org/patch/852049/

2/4 multiboot: Remove unused variables from multiboot.c
http://patchwork.ozlabs.org/patch/852045/

3/4 multiboot: Use header names when displaying fields
http://patchwork.ozlabs.org/patch/852046/

4/4 multiboot: fprintf(stderr...) -> error_report()
http://patchwork.ozlabs.org/patch/852051/

     Thanks,
     Jack

Re: [Qemu-devel] [PATCH QEMU v1 1/4] multiboot: bss_end_addr can be zero

[P J P]

+-- On Thu, 21 Dec 2017, Jack Schwartz wrote --+
| The multiboot spec (https://www.gnu.org/software/grub/manual/multiboot/),
| section 3.1.3, allows for bss_end_addr to be zero.
| 
| A zero bss_end_addr signifies there is no .bss section.
| 
| Suggested-by: Daniel Kiper <daniel.kiper@oracle.com>
| Signed-off-by: Jack Schwartz <jack.schwartz@oracle.com>
| Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
| ---
|  hw/i386/multiboot.c | 18 ++++++++++--------
|  1 file changed, 10 insertions(+), 8 deletions(-)
| 
| diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
| index c7b70c9..ff2733d 100644
| --- a/hw/i386/multiboot.c
| +++ b/hw/i386/multiboot.c
| @@ -233,12 +233,6 @@ int load_multiboot(FWCfgState *fw_cfg,
|          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);
| @@ -249,8 +243,16 @@ int load_multiboot(FWCfgState *fw_cfg,
|                  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;
| +            mb_load_size = kernel_file_size - mb_kernel_text_offset;
| +        }
| +        if (mh_bss_end_addr) {
| +            if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
| +                fprintf(stderr, "invalid mh_bss_end_addr address\n");
| +                exit(1);
| +            }
| +            mb_kernel_size = mh_bss_end_addr - mh_load_addr;
| +        } else {
| +            mb_kernel_size = mb_load_size;
|          }
|  
|          /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.

Looks good.
Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>

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

Re: [Qemu-devel] [PATCH QEMU v1 2/4] multiboot: Remove unused variables from multiboot.c

[P J P]

+-- On Thu, 21 Dec 2017, Jack Schwartz wrote --+
| Remove unused variables: mh_mode_type, mh_width, mh_height, mh_depth
| 
| Signed-off-by: Jack Schwartz <jack.schwartz@oracle.com>
| Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
| ---
|  hw/i386/multiboot.c | 6 ------
|  1 file changed, 6 deletions(-)
| 
| diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
| index ff2733d..964feaf 100644
| --- a/hw/i386/multiboot.c
| +++ b/hw/i386/multiboot.c
| @@ -255,12 +255,6 @@ int load_multiboot(FWCfgState *fw_cfg,
|              mb_kernel_size = mb_load_size;
|          }
|  
| -        /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
| -        uint32_t mh_mode_type = ldl_p(header+i+32);
| -        uint32_t mh_width = ldl_p(header+i+36);
| -        uint32_t mh_height = ldl_p(header+i+40);
| -        uint32_t mh_depth = ldl_p(header+i+44); */
| -

+1 They are anyway comments.
Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>

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

Re: [Qemu-devel] [PATCH QEMU v1 3/4] multiboot: Use header names when displaying fields

[P J P]

+-- On Thu, 21 Dec 2017, Jack Schwartz wrote --+
| Refer to field names when displaying fields in printf and debug statements.

I wonder if it's required; Having variable names is helpful while going 
through code.

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

Re: [Qemu-devel] [PATCH QEMU v1 4/4] multiboot: fprintf(stderr...) -> error_report()

[P J P]

+-- On Thu, 21 Dec 2017, Jack Schwartz wrote --+
| Change all fprintf(stderr...) calls in hw/i386/multiboot.c to call
| error_report() instead, including the mb_debug macro.  Remove the "\n"
| from strings passed to all modified calls, since error_report() appends
| one.
| 
| Signed-off-by: Jack Schwartz <jack.schwartz@oracle.com>
| Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
| ---
|  hw/i386/multiboot.c | 55 ++++++++++++++++++++++++++++-------------------------
|  1 file changed, 29 insertions(+), 26 deletions(-)
| 
| diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
| index 818728b..d9a0a95 100644
| --- a/hw/i386/multiboot.c
| +++ b/hw/i386/multiboot.c
| @@ -31,12 +31,13 @@
|  #include "hw/loader.h"
|  #include "elf.h"
|  #include "sysemu/sysemu.h"
| +#include "qemu/error-report.h"
|  
|  /* Show multiboot debug output */
|  //#define DEBUG_MULTIBOOT
|  
|  #ifdef DEBUG_MULTIBOOT
| -#define mb_debug(a...) fprintf(stderr, ## a)
| +#define mb_debug(a...) error_report(a)
|  #else
|  #define mb_debug(a...)
|  #endif
| @@ -137,7 +138,7 @@ static void mb_add_mod(MultibootState *s,
|      stl_p(p + MB_MOD_END,     end);
|      stl_p(p + MB_MOD_CMDLINE, cmdline_phys);
|  
| -    mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx"\n",
| +    mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx,
|               s->mb_mods_count, start, end);
|  
|      s->mb_mods_count++;
| @@ -179,12 +180,12 @@ int load_multiboot(FWCfgState *fw_cfg,
|      if (!is_multiboot)
|          return 0; /* no multiboot */
|  
| -    mb_debug("qemu: I believe we found a multiboot image!\n");
| +    mb_debug("qemu: I believe we found a multiboot image!");
|      memset(bootinfo, 0, sizeof(bootinfo));
|      memset(&mbs, 0, sizeof(mbs));
|  
|      if (flags & 0x00000004) { /* MULTIBOOT_HEADER_HAS_VBE */
| -        fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
| +        error_report("qemu: multiboot knows VBE. we don't.");
|      }
|      if (!(flags & 0x00010000)) { /* MULTIBOOT_HEADER_HAS_ADDR */
|          uint64_t elf_entry;
| @@ -193,7 +194,7 @@ int load_multiboot(FWCfgState *fw_cfg,
|          fclose(f);
|  
|          if (((struct elf64_hdr*)header)->e_machine == EM_X86_64) {
| -            fprintf(stderr, "Cannot load x86-64 image, give a 32bit one.\n");
| +            error_report("Cannot load x86-64 image, give a 32bit one.");
|              exit(1);
|          }
|  
| @@ -201,7 +202,7 @@ int load_multiboot(FWCfgState *fw_cfg,
|                                 &elf_low, &elf_high, 0, I386_ELF_MACHINE,
|                                 0, 0);
|          if (kernel_size < 0) {
| -            fprintf(stderr, "Error while loading elf kernel\n");
| +            error_report("Error while loading elf kernel");
|              exit(1);
|          }
|          mh_load_addr = elf_low;
| @@ -210,12 +211,13 @@ int load_multiboot(FWCfgState *fw_cfg,
|  
|          mbs.mb_buf = g_malloc(mb_kernel_size);
|          if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
| -            fprintf(stderr, "Error while fetching elf kernel from rom\n");
| +            error_report("Error while fetching elf kernel from rom");
|              exit(1);
|          }
|  
| -        mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
| -                  mb_kernel_size, (size_t)mh_entry_addr);
| +        mb_debug("qemu: loading multiboot-elf kernel "
| +                 "(%#x bytes) with entry %#zx",
| +                 mb_kernel_size, (size_t)mh_entry_addr);
|      } else {
|          /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
|          uint32_t mh_header_addr = ldl_p(header+i+12);
| @@ -224,7 +226,7 @@ int load_multiboot(FWCfgState *fw_cfg,
|  
|          mh_load_addr = ldl_p(header+i+16);
|          if (mh_header_addr < mh_load_addr) {
| -            fprintf(stderr, "invalid load_addr address\n");
| +            error_report("invalid load_addr address");
|              exit(1);
|          }
|  
| @@ -234,20 +236,20 @@ int load_multiboot(FWCfgState *fw_cfg,
|  
|          if (mh_load_end_addr) {
|              if (mh_load_end_addr < mh_load_addr) {
| -                fprintf(stderr, "invalid load_end_addr address\n");
| +                error_report("invalid load_end_addr address");
|                  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");
| +                error_report("invalid kernel_file_size");
|                  exit(1);
|              }
|              mb_load_size = kernel_file_size - mb_kernel_text_offset;
|          }
|          if (mh_bss_end_addr) {
|              if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
| -                fprintf(stderr, "invalid bss_end_addr address\n");
| +                error_report("invalid bss_end_addr address");
|                  exit(1);
|              }
|              mb_kernel_size = mh_bss_end_addr - mh_load_addr;
| @@ -255,17 +257,17 @@ int load_multiboot(FWCfgState *fw_cfg,
|              mb_kernel_size = mb_load_size;
|          }
|  
| -        mb_debug("multiboot: header_addr = %#x\n", mh_header_addr);
| -        mb_debug("multiboot: load_addr = %#x\n", mh_load_addr);
| -        mb_debug("multiboot: load_end_addr = %#x\n", mh_load_end_addr);
| -        mb_debug("multiboot: bss_end_addr = %#x\n", mh_bss_end_addr);
| -        mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x\n",
| +        mb_debug("multiboot: header_addr = %#x", mh_header_addr);
| +        mb_debug("multiboot: load_addr = %#x", mh_load_addr);
| +        mb_debug("multiboot: load_end_addr = %#x", mh_load_end_addr);
| +        mb_debug("multiboot: bss_end_addr = %#x", mh_bss_end_addr);
| +        mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x",
|                   mb_load_size, mh_load_addr);
|  
|          mbs.mb_buf = g_malloc(mb_kernel_size);
|          fseek(f, mb_kernel_text_offset, SEEK_SET);
|          if (fread(mbs.mb_buf, 1, mb_load_size, f) != mb_load_size) {
| -            fprintf(stderr, "fread() failed\n");
| +            error_report("fread() failed");
|              exit(1);
|          }
|          memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
| @@ -319,10 +321,10 @@ int load_multiboot(FWCfgState *fw_cfg,
|              hwaddr c = mb_add_cmdline(&mbs, tmpbuf);
|              if ((next_space = strchr(tmpbuf, ' ')))
|                  *next_space = '\0';
| -            mb_debug("multiboot loading module: %s\n", tmpbuf);
| +            mb_debug("multiboot loading module: %s", tmpbuf);
|              mb_mod_length = get_image_size(tmpbuf);
|              if (mb_mod_length < 0) {
| -                fprintf(stderr, "Failed to open file '%s'\n", tmpbuf);
| +                error_report("Failed to open file '%s'", tmpbuf);
|                  exit(1);
|              }
|  
| @@ -333,7 +335,7 @@ int load_multiboot(FWCfgState *fw_cfg,
|              mb_add_mod(&mbs, mbs.mb_buf_phys + offs,
|                         mbs.mb_buf_phys + offs + mb_mod_length, c);
|  
| -            mb_debug("mod_start: %p\nmod_end:   %p\n  cmdline: "TARGET_FMT_plx"\n",
| +            mb_debug("mod_start: %p\nmod_end:   %p\n  cmdline: "TARGET_FMT_plx,
|                       (char *)mbs.mb_buf + offs,
|                       (char *)mbs.mb_buf + offs + mb_mod_length, c);
|              initrd_filename = next_initrd+1;
| @@ -361,10 +363,11 @@ int load_multiboot(FWCfgState *fw_cfg,
|      stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
|      stl_p(bootinfo + MBI_MMAP_ADDR,   ADDR_E820_MAP);
|  
| -    mb_debug("multiboot: entry_addr = %#x\n", mh_entry_addr);
| -    mb_debug("           mb_buf_phys   = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
| -    mb_debug("           mod_start     = "TARGET_FMT_plx"\n", mbs.mb_buf_phys + mbs.offset_mods);
| -    mb_debug("           mb_mods_count = %d\n", mbs.mb_mods_count);
| +    mb_debug("multiboot: entry_addr = %#x", mh_entry_addr);
| +    mb_debug("           mb_buf_phys   = "TARGET_FMT_plx, mbs.mb_buf_phys);
| +    mb_debug("           mod_start     = "TARGET_FMT_plx,
| +             mbs.mb_buf_phys + mbs.offset_mods);
| +    mb_debug("           mb_mods_count = %d", mbs.mb_mods_count);
|  
|      /* save bootinfo off the stack */
|      mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
| 

Looks okay.

It maybe good to have version of error_report() which calls exit(1) too. I 
guess err(3) family of functions isn't used because they are nonstandard.

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

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Kevin Wolf]

Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
> Properly account for the possibility of multiboot kernels with a zero
> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
> kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
> header field.
> 
> Do some cleanup to multiboot.c as well:
> - Remove some unused variables.
> - Use more intuitive header names when displaying fields in messages.
> - Change fprintf(stderr...) to error_report
> 
> Testing:
>   1) Ran the "make check" test suite.
>   2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
>      grub multiboot.elf test "kernel" by modifying source.)  Verified
>      with gdb that new code that reads addresses/offsets from multiboot
>      header was accessed.
>   3) Booted multiboot kernel with non-zero bss_end_addr.
>   4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
>   5) Code has soaked in an internal repo for two months.
> 
> 	Thanks,
> 	Jack

Thanks, applied to my multiboot branch.

Kevin

[Qemu-devel] CVE-2018-7550 (was: multiboot: bss_end_addr can be zero / cleanup)

[Kevin Wolf]

Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
> Properly account for the possibility of multiboot kernels with a zero
> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
> kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
> header field.
> 
> Do some cleanup to multiboot.c as well:
> - Remove some unused variables.
> - Use more intuitive header names when displaying fields in messages.
> - Change fprintf(stderr...) to error_report

[ Cc: qemu-stable ]

This series happens to fix CVE-2018-7550.
http://www.openwall.com/lists/oss-security/2018/03/08/4

Just a shame that we weren't told before merging it so that the
appropriate tags could have been set in the commit message (and all of
the problems could have been addressed; I'm going to send another
Multiboot series now).

Kevin

Re: [Qemu-devel] CVE-2018-7550 (was: multiboot: bss_end_addr can be zero / cleanup)

[Konrad Rzeszutek Wilk]

On March 14, 2018 1:23:51 PM EDT, Kevin Wolf <kwolf@redhat.com> wrote:
>Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
>> Properly account for the possibility of multiboot kernels with a zero
>> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
>> kernels without a bss section, by allowing a zeroed bss_end_addr
>multiboot
>> header field.
>> 
>> Do some cleanup to multiboot.c as well:
>> - Remove some unused variables.
>> - Use more intuitive header names when displaying fields in messages.
>> - Change fprintf(stderr...) to error_report
>
>[ Cc: qemu-stable ]
>
>This series happens to fix CVE-2018-7550.
>http://www.openwall.com/lists/oss-security/2018/03/08/4
>
>Just a shame that we weren't told before merging it so that the
>appropriate tags could have been set in the commit message (and all of
>the problems could have been addressed; I'm going to send another
>Multiboot series now).

Huh?

You mean the CVE tags that were created in 2018 for a patch posted in 2017?

Or that the reporter of the security issue didn't point to this particular patch?

Irrespective of that,  is there a write-up  of how security process works at QEMU?

 That is what is the usual embargo period, the list of security folks, how one can become one, what are the responsibilities, how changes to process are being carried out (and discussed), what breath of testing and PoC work is done , how security fixes are being reviewed, etc?

Thanks!

>
>Kevin

Re: [Qemu-devel] CVE-2018-7550 (was: multiboot: bss_end_addr can be zero / cleanup)

[Kevin Wolf]

Am 14.03.2018 um 18:35 hat Konrad Rzeszutek Wilk geschrieben:
> On March 14, 2018 1:23:51 PM EDT, Kevin Wolf <kwolf@redhat.com> wrote:
> >Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
> >> Properly account for the possibility of multiboot kernels with a zero
> >> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
> >> kernels without a bss section, by allowing a zeroed bss_end_addr
> >multiboot
> >> header field.
> >> 
> >> Do some cleanup to multiboot.c as well:
> >> - Remove some unused variables.
> >> - Use more intuitive header names when displaying fields in messages.
> >> - Change fprintf(stderr...) to error_report
> >
> >[ Cc: qemu-stable ]
> >
> >This series happens to fix CVE-2018-7550.
> >http://www.openwall.com/lists/oss-security/2018/03/08/4
> >
> >Just a shame that we weren't told before merging it so that the
> >appropriate tags could have been set in the commit message (and all of
> >the problems could have been addressed; I'm going to send another
> >Multiboot series now).
> 
> Huh?
> 
> You mean the CVE tags that were created in 2018 for a patch posted in
> 2017?

Well, it seems to me that this patch was created for a different
purpose, but it happens to fix the bug for which this CVE was assigned
now. It's not your or Jack's fault, that's just how things go sometimes.

I think PJP knew that this CVE was coming before the patches were merged
into master, so if he had told us, we could have had a better commit
message. But either way, it's not a disaster to have a suboptimal commit
message.

> Or that the reporter of the security issue didn't point to this particular patch?
> 
> Irrespective of that,  is there a write-up  of how security process
> works at QEMU?
> 
> That is what is the usual embargo period, the list of security folks,
> how one can become one, what are the responsibilities, how changes to
> process are being carried out (and discussed), what breath of testing
> and PoC work is done , how security fixes are being reviewed, etc?

I don't think a problem like this would be embargoed at all. Anyway,
have a look here:

https://wiki.qemu.org/SecurityProcess

Kevin

Re: [Qemu-devel] CVE-2018-7550 (was: multiboot: bss_end_addr can be zero / cleanup)

[P J P]

+-- On Wed, 14 Mar 2018, Kevin Wolf wrote --+
| Well, it seems to me that this patch was created for a different
| purpose, but it happens to fix the bug for which this CVE was assigned
| now.

  Right. I had sent another patch to fix it, there Jack mentioned about his 
series from before.
 
| I think PJP knew that this CVE was coming before the patches were merged
| into master, so if he had told us, we could have had a better commit
| message.

  For non-embargoed issues we request CVE from Mitre after a proposed patch is 
reviewed and acked for upstream QEMU. Nevertheless, going forward I'll mention 
the CVE-ID here on the list, as soon as it is assigned.

| I don't think a problem like this would be embargoed at all.

  True.

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