Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Laszlo Ersek]

On 10/25/19 18:57, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
> to only allow the range 0..65535; however both qemu and libvirt document
> the special value -1  to mean don't reboot.
> Allow it again.
>
> Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout error 
> checking")
> RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  hw/nvram/fw_cfg.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 7dc3ac378e..1a9ec44232 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
>
>      if (reboot_timeout) {
>          rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> +
>          /* validate the input */
> -        if (rt_val < 0 || rt_val > 0xffff) {
> +        if (rt_val < -1 || rt_val > 0xffff) {
>              error_report("reboot timeout is invalid,"
> -                         "it should be a value between 0 and 65535");
> +                         "it should be a value between -1 and 65535");
>              exit(1);
>          }
>      }
>

Ouch.

Here's the prototype of qemu_opt_get_number():

> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t 
> defval);

So, when we call it, here's what we actually do:

        rt_val = (int64_t)qemu_opt_get_number(opts, "reboot-timeout", 
(uint64_t)-1);
                 ^^^^^^^^^                                            ^^^^^^^^^^

The conversion to uint64_t is fine.

The conversion to int64_t is not great:

> Otherwise, the new type is signed and the value cannot be represented
> in it; either the result is implementation-defined or an
> implementation-defined signal is raised.

I guess we're exploiting two's complement, as the implementation-defined
result. Not great. :)

Here's what I'd prefer:

> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 7dc3ac378ee0..16413550a1da 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -237,7 +237,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>  static void fw_cfg_reboot(FWCfgState *s)
>  {
>      const char *reboot_timeout = NULL;
> -    int64_t rt_val = -1;
> +    uint64_t rt_val = -1;
>      uint32_t rt_le32;
>
>      /* get user configuration */
> @@ -248,9 +248,9 @@ static void fw_cfg_reboot(FWCfgState *s)
>      if (reboot_timeout) {
>          rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
>          /* validate the input */
> -        if (rt_val < 0 || rt_val > 0xffff) {
> +        if (rt_val > 0xffff && rt_val != (uint64_t)-1) {
>              error_report("reboot timeout is invalid,"
> -                         "it should be a value between 0 and 65535");
> +                         "it should be a value between -1 and 65535");
>              exit(1);
>          }
>      }

(

The trick is that strtoull(), in

  qemu_opt_get_number()
    qemu_opt_get_number_helper()
      parse_option_number()
        qemu_strtou64()
          strtoull()

turns "-1" into (uint64_t)-1, which counts as a valid conversion, per
spec:

> If the subject sequence has the expected form and the value of /base/
> is zero, the sequence of characters starting with the first digit is
> interpreted as an integer constant according to the rules of 6.4.4.1.
> If the subject sequence has the expected form and the value of /base/
> is between 2 and 36, it is used as the base for conversion, ascribing
> to each letter its value as given above. If the subject sequence
> begins with a minus sign, the value resulting from the conversion is
> negated (in the return type). A pointer to the final string is stored
> in the object pointed to by /endptr/, provided that /endptr/ is not a
> null pointer.

)

I don't insist though; if Phil is OK with the posted patch, I won't try
to block it.

Thanks
Laszlo