[Qemu-devel] [PATCH v2 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[Qemu-devel] [PATCH v2 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[Li Qiang]

And also do some code cleanup.
A lot of thanks to Markus's review and advice.

v2: fix some small issue per Markus's review.

Li Qiang (3):
  fw_cfg: fix -boot bootsplash error checking
  fw_cfg: fix -boot reboot-timeout error checking
  fw_cfg: make qemu_extra_params_fw locally

 hw/nvram/fw_cfg.c       | 63 +++++++++++++++++++++----------------------------
 include/sysemu/sysemu.h |  1 -
 vl.c                    |  4 ++--
 3 files changed, 29 insertions(+), 39 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/3] fw_cfg: fix -boot bootsplash error checking

[Li Qiang]

fw_cfg_bootsplash() gets option parameter "splash-time"
with qemu_opt_get(), then converts it to an integer by hand.
It neglects to check that conversion for errors. This is
needlessly complicated and error-prone. But as "splash-time
not specified" is not the same as "splash-time=T" for any T,
we need use qemu_opt_get() to check if splash time exists.
This patch also make the qemu exit when finding or loading
splash file failed.

Signed-off-by: Li Qiang <liq3ea@gmail.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
v1->v2: drop error_report when read_splashfile() fails

 hw/nvram/fw_cfg.c | 35 +++++++++++++----------------------
 vl.c              |  2 +-
 2 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 946f765f7f..83d66818f6 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -118,47 +118,38 @@ error:
 
 static void fw_cfg_bootsplash(FWCfgState *s)
 {
-    int boot_splash_time = -1;
     const char *boot_splash_filename = NULL;
-    char *p;
+    const char *boot_splash_time = NULL;
     char *filename, *file_data;
     gsize file_size;
     int file_type;
-    const char *temp;
 
     /* get user configuration */
     QemuOptsList *plist = qemu_find_opts("boot-opts");
     QemuOpts *opts = QTAILQ_FIRST(&plist->head);
-    if (opts != NULL) {
-        temp = qemu_opt_get(opts, "splash");
-        if (temp != NULL) {
-            boot_splash_filename = temp;
-        }
-        temp = qemu_opt_get(opts, "splash-time");
-        if (temp != NULL) {
-            p = (char *)temp;
-            boot_splash_time = strtol(p, &p, 10);
-        }
-    }
+    boot_splash_filename = qemu_opt_get(opts, "splash");
+    boot_splash_time = qemu_opt_get(opts, "splash-time");
 
     /* insert splash time if user configurated */
-    if (boot_splash_time >= 0) {
+    if (boot_splash_time) {
+        int64_t bst_val = qemu_opt_get_number(opts, "splash-time", -1);
         /* validate the input */
-        if (boot_splash_time > 0xffff) {
-            error_report("splash time is big than 65535, force it to 65535.");
-            boot_splash_time = 0xffff;
+        if (bst_val < 0 || bst_val > 0xffff) {
+            error_report("splash-time is invalid,"
+                         "it should be a value between 0 and 65535");
+            exit(1);
         }
         /* use little endian format */
-        qemu_extra_params_fw[0] = (uint8_t)(boot_splash_time & 0xff);
-        qemu_extra_params_fw[1] = (uint8_t)((boot_splash_time >> 8) & 0xff);
+        qemu_extra_params_fw[0] = (uint8_t)(bst_val & 0xff);
+        qemu_extra_params_fw[1] = (uint8_t)((bst_val >> 8) & 0xff);
         fw_cfg_add_file(s, "etc/boot-menu-wait", qemu_extra_params_fw, 2);
     }
 
     /* insert splash file if user configurated */
-    if (boot_splash_filename != NULL) {
+    if (boot_splash_filename) {
         filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, boot_splash_filename);
         if (filename == NULL) {
-            error_report("failed to find file '%s'.", boot_splash_filename);
+            error_report("failed to find file '%s'", boot_splash_filename);
             return;
         }
 
diff --git a/vl.c b/vl.c
index fa25d1ae2d..96ac0ddcf6 100644
--- a/vl.c
+++ b/vl.c
@@ -336,7 +336,7 @@ static QemuOptsList qemu_boot_opts = {
             .type = QEMU_OPT_STRING,
         }, {
             .name = "splash-time",
-            .type = QEMU_OPT_STRING,
+            .type = QEMU_OPT_NUMBER,
         }, {
             .name = "reboot-timeout",
             .type = QEMU_OPT_STRING,
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 1/3] fw_cfg: fix -boot bootsplash error checking

[Philippe Mathieu-Daudé]

On 11/21/18 6:10 AM, Li Qiang wrote:
> fw_cfg_bootsplash() gets option parameter "splash-time"
> with qemu_opt_get(), then converts it to an integer by hand.
> It neglects to check that conversion for errors. This is
> needlessly complicated and error-prone. But as "splash-time
> not specified" is not the same as "splash-time=T" for any T,
> we need use qemu_opt_get() to check if splash time exists.
> This patch also make the qemu exit when finding or loading
> splash file failed.
> 
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
> v1->v2: drop error_report when read_splashfile() fails
> 
>  hw/nvram/fw_cfg.c | 35 +++++++++++++----------------------
>  vl.c              |  2 +-
>  2 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 946f765f7f..83d66818f6 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -118,47 +118,38 @@ error:
>  
>  static void fw_cfg_bootsplash(FWCfgState *s)
>  {
> -    int boot_splash_time = -1;
>      const char *boot_splash_filename = NULL;
> -    char *p;
> +    const char *boot_splash_time = NULL;
>      char *filename, *file_data;
>      gsize file_size;
>      int file_type;
> -    const char *temp;
>  
>      /* get user configuration */
>      QemuOptsList *plist = qemu_find_opts("boot-opts");
>      QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> -    if (opts != NULL) {
> -        temp = qemu_opt_get(opts, "splash");
> -        if (temp != NULL) {
> -            boot_splash_filename = temp;
> -        }
> -        temp = qemu_opt_get(opts, "splash-time");
> -        if (temp != NULL) {
> -            p = (char *)temp;
> -            boot_splash_time = strtol(p, &p, 10);
> -        }
> -    }
> +    boot_splash_filename = qemu_opt_get(opts, "splash");
> +    boot_splash_time = qemu_opt_get(opts, "splash-time");
>  
>      /* insert splash time if user configurated */
> -    if (boot_splash_time >= 0) {
> +    if (boot_splash_time) {
> +        int64_t bst_val = qemu_opt_get_number(opts, "splash-time", -1);
>          /* validate the input */
> -        if (boot_splash_time > 0xffff) {
> -            error_report("splash time is big than 65535, force it to 65535.");
> -            boot_splash_time = 0xffff;
> +        if (bst_val < 0 || bst_val > 0xffff) {
> +            error_report("splash-time is invalid,"
> +                         "it should be a value between 0 and 65535");
> +            exit(1);
>          }
>          /* use little endian format */
> -        qemu_extra_params_fw[0] = (uint8_t)(boot_splash_time & 0xff);
> -        qemu_extra_params_fw[1] = (uint8_t)((boot_splash_time >> 8) & 0xff);
> +        qemu_extra_params_fw[0] = (uint8_t)(bst_val & 0xff);
> +        qemu_extra_params_fw[1] = (uint8_t)((bst_val >> 8) & 0xff);
>          fw_cfg_add_file(s, "etc/boot-menu-wait", qemu_extra_params_fw, 2);
>      }
>  
>      /* insert splash file if user configurated */
> -    if (boot_splash_filename != NULL) {
> +    if (boot_splash_filename) {
>          filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, boot_splash_filename);
>          if (filename == NULL) {
> -            error_report("failed to find file '%s'.", boot_splash_filename);
> +            error_report("failed to find file '%s'", boot_splash_filename);
>              return;
>          }
>  
> diff --git a/vl.c b/vl.c
> index fa25d1ae2d..96ac0ddcf6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -336,7 +336,7 @@ static QemuOptsList qemu_boot_opts = {
>              .type = QEMU_OPT_STRING,
>          }, {
>              .name = "splash-time",
> -            .type = QEMU_OPT_STRING,
> +            .type = QEMU_OPT_NUMBER,
>          }, {
>              .name = "reboot-timeout",
>              .type = QEMU_OPT_STRING,
> 

[Qemu-devel] [PATCH v2 2/3] fw_cfg: fix -boot reboot-timeout error checking

[Li Qiang]

fw_cfg_reboot() gets option parameter "reboot-timeout" with
qemu_opt_get(), then converts it to an integer by hand. It neglects to
check that conversion for errors, and fails to reject negative values.
Positive values above the limit get reported and replaced by the limit.
This patch checks for conversion errors properly, and reject all values
outside 0...0xffff.

Signed-off-by: Li Qiang <liq3ea@gmail.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
v1->v2: commit typo fix

 hw/nvram/fw_cfg.c | 27 +++++++++++++--------------
 vl.c              |  2 +-
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 83d66818f6..aafa96721f 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -177,26 +177,25 @@ static void fw_cfg_bootsplash(FWCfgState *s)
 
 static void fw_cfg_reboot(FWCfgState *s)
 {
-    int reboot_timeout = -1;
-    char *p;
-    const char *temp;
+    const char *reboot_timeout = NULL;
+    int64_t rt_val = -1;
 
     /* get user configuration */
     QemuOptsList *plist = qemu_find_opts("boot-opts");
     QemuOpts *opts = QTAILQ_FIRST(&plist->head);
-    if (opts != NULL) {
-        temp = qemu_opt_get(opts, "reboot-timeout");
-        if (temp != NULL) {
-            p = (char *)temp;
-            reboot_timeout = strtol(p, &p, 10);
+    reboot_timeout = qemu_opt_get(opts, "reboot-timeout");
+
+    if (reboot_timeout) {
+        rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
+        /* validate the input */
+        if (rt_val < 0 || rt_val > 0xffff) {
+            error_report("reboot timeout is invalid,"
+                         "it should be a value between 0 and 65535");
+            exit(1);
         }
     }
-    /* validate the input */
-    if (reboot_timeout > 0xffff) {
-        error_report("reboot timeout is larger than 65535, force it to 65535.");
-        reboot_timeout = 0xffff;
-    }
-    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4);
+
+    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_val, 4), 4);
 }
 
 static void fw_cfg_write(FWCfgState *s, uint8_t value)
diff --git a/vl.c b/vl.c
index 96ac0ddcf6..38a1759461 100644
--- a/vl.c
+++ b/vl.c
@@ -339,7 +339,7 @@ static QemuOptsList qemu_boot_opts = {
             .type = QEMU_OPT_NUMBER,
         }, {
             .name = "reboot-timeout",
-            .type = QEMU_OPT_STRING,
+            .type = QEMU_OPT_NUMBER,
         }, {
             .name = "strict",
             .type = QEMU_OPT_BOOL,
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 2/3] fw_cfg: fix -boot reboot-timeout error checking

[Philippe Mathieu-Daudé]

On 11/21/18 6:10 AM, Li Qiang wrote:
> fw_cfg_reboot() gets option parameter "reboot-timeout" with
> qemu_opt_get(), then converts it to an integer by hand. It neglects to
> check that conversion for errors, and fails to reject negative values.
> Positive values above the limit get reported and replaced by the limit.
> This patch checks for conversion errors properly, and reject all values
> outside 0...0xffff.
> 
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> v1->v2: commit typo fix
> 
>  hw/nvram/fw_cfg.c | 27 +++++++++++++--------------
>  vl.c              |  2 +-
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 83d66818f6..aafa96721f 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -177,26 +177,25 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>  
>  static void fw_cfg_reboot(FWCfgState *s)
>  {
> -    int reboot_timeout = -1;
> -    char *p;
> -    const char *temp;
> +    const char *reboot_timeout = NULL;
> +    int64_t rt_val = -1;
>  
>      /* get user configuration */
>      QemuOptsList *plist = qemu_find_opts("boot-opts");
>      QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> -    if (opts != NULL) {
> -        temp = qemu_opt_get(opts, "reboot-timeout");
> -        if (temp != NULL) {
> -            p = (char *)temp;
> -            reboot_timeout = strtol(p, &p, 10);
> +    reboot_timeout = qemu_opt_get(opts, "reboot-timeout");
> +
> +    if (reboot_timeout) {
> +        rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> +        /* validate the input */
> +        if (rt_val < 0 || rt_val > 0xffff) {
> +            error_report("reboot timeout is invalid,"
> +                         "it should be a value between 0 and 65535");
> +            exit(1);
>          }
>      }
> -    /* validate the input */
> -    if (reboot_timeout > 0xffff) {
> -        error_report("reboot timeout is larger than 65535, force it to 65535.");
> -        reboot_timeout = 0xffff;
> -    }
> -    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4);
> +
> +    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_val, 4), 4);
>  }
>  
>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
> diff --git a/vl.c b/vl.c
> index 96ac0ddcf6..38a1759461 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -339,7 +339,7 @@ static QemuOptsList qemu_boot_opts = {
>              .type = QEMU_OPT_NUMBER,
>          }, {
>              .name = "reboot-timeout",
> -            .type = QEMU_OPT_STRING,
> +            .type = QEMU_OPT_NUMBER,
>          }, {
>              .name = "strict",
>              .type = QEMU_OPT_BOOL,
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[Qemu-devel] [PATCH v2 3/3] fw_cfg: make qemu_extra_params_fw locally

[Li Qiang]

qemu_extra_params_fw[] has external linkage, but is used
only in fw_cfg_bootsplash(), it makes sense to make it
locally.

Signed-off-by: Li Qiang <liq3ea@gmail.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/nvram/fw_cfg.c       | 1 +
 include/sysemu/sysemu.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index aafa96721f..a71f043b5d 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -120,6 +120,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
 {
     const char *boot_splash_filename = NULL;
     const char *boot_splash_time = NULL;
+    uint8_t qemu_extra_params_fw[2];
     char *filename, *file_data;
     gsize file_size;
     int file_type;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 8d6095d98b..f8608d24d9 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -133,7 +133,6 @@ extern uint8_t *boot_splash_filedata;
 extern size_t boot_splash_filedata_size;
 extern bool enable_mlock;
 extern bool enable_cpu_pm;
-extern uint8_t qemu_extra_params_fw[2];
 extern QEMUClockType rtc_clock;
 extern const char *mem_path;
 extern int mem_prealloc;
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 3/3] fw_cfg: make qemu_extra_params_fw locally

[Philippe Mathieu-Daudé]

Hi Li,

On 11/21/18 6:10 AM, Li Qiang wrote:
> qemu_extra_params_fw[] has external linkage, but is used
> only in fw_cfg_bootsplash(), it makes sense to make it
> locally.
> 
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/nvram/fw_cfg.c       | 1 +
>  include/sysemu/sysemu.h | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index aafa96721f..a71f043b5d 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -120,6 +120,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>  {
>      const char *boot_splash_filename = NULL;
>      const char *boot_splash_time = NULL;
> +    uint8_t qemu_extra_params_fw[2];
>      char *filename, *file_data;
>      gsize file_size;
>      int file_type;
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 8d6095d98b..f8608d24d9 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -133,7 +133,6 @@ extern uint8_t *boot_splash_filedata;
>  extern size_t boot_splash_filedata_size;
>  extern bool enable_mlock;
>  extern bool enable_cpu_pm;
> -extern uint8_t qemu_extra_params_fw[2];
>  extern QEMUClockType rtc_clock;
>  extern const char *mem_path;
>  extern int mem_prealloc;
> 

You forgot to remove it from vl.c:

$ git grep qemu_extra_params_fw
vl.c:194:uint8_t qemu_extra_params_fw[2];

I'll simply squash this in your patch when applying:

-- >8 --
--- a/vl.c
+++ b/vl.c
@@ -191,7 +191,6 @@ int boot_menu;
 bool boot_strict;
 uint8_t *boot_splash_filedata;
 size_t boot_splash_filedata_size;
-uint8_t qemu_extra_params_fw[2];

 int icount_align_option;

---

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[Gerd Hoffmann]

On Tue, Nov 20, 2018 at 09:10:23PM -0800, Li Qiang wrote:
> And also do some code cleanup.
> A lot of thanks to Markus's review and advice.
> 
> v2: fix some small issue per Markus's review.
> 
> Li Qiang (3):
>   fw_cfg: fix -boot bootsplash error checking
>   fw_cfg: fix -boot reboot-timeout error checking
>   fw_cfg: make qemu_extra_params_fw locally

Series is
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[Philippe Mathieu-Daudé]

On 11/21/18 6:10 AM, Li Qiang wrote:
> And also do some code cleanup.
> A lot of thanks to Markus's review and advice.
> 
> v2: fix some small issue per Markus's review.
> 
> Li Qiang (3):
>   fw_cfg: fix -boot bootsplash error checking
>   fw_cfg: fix -boot reboot-timeout error checking
>   fw_cfg: make qemu_extra_params_fw locally
> 
>  hw/nvram/fw_cfg.c       | 63 +++++++++++++++++++++----------------------------
>  include/sysemu/sysemu.h |  1 -
>  vl.c                    |  4 ++--
>  3 files changed, 29 insertions(+), 39 deletions(-)
> 

Thanks, I've queued this series for when 4.0 opens up.

Regards,

Phil.