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 <address@hidden>
> Reviewed-by: Markus Armbruster <address@hidden>
> ---
> 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é <address@hidden>