Re: [PATCH v2 18/19] cutils: Improve qemu_strtod* error paths

[Eric Blake]

On Thu, May 11, 2023 at 09:10:32PM -0500, Eric Blake wrote:
> 
> Previous patches changed all integral qemu_strto*() error paths to
> guarantee that *value is never left uninitialized.  Do likewise for
> qemu_strtod.  Also, tighten qemu_strtod_finite() to never return a
> non-finite value (prior to this patch, we were rejecting "inf" with
> -EINVAL and unspecified result 0.0, but failing "9e999" with -ERANGE
> and HUGE_VAL - which is infinite on IEEE machines - despite our
> function claiming to recognize only finite values).
> 
> Auditing callers, we have no external callers of qemu_strtod, and
> among the callers of qemu_strtod_finite:
> 
> - qapi/qobject-input-visitor.c:qobject_input_type_number_keyval() and
>   qapi/string-input-visitor.c:parse_type_number() which reject all
>   errors (does not matter what we store)
> 
> - utils/cutils.c:do_strtosz() incorrectly assumes that *endptr points
>   to '.' on all failures (that is, it is not distinguishing between
>   EINVAL and ERANGE; and therefore still does the WRONG THING for
>   "9.9e999".  The change here does not entirely fix that (a later
>   patch will tackle this more systematically), but at least the value
>   of endptr is now less likely to be out of bounds on overflow
> 
> - our testsuite, which we can update to match what we document
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
> 
> ---

cc'ing qemu-stable for comments, since this patch fixes a sanitizer
issue flagged in https://gitlab.com/qemu-project/qemu/-/issues/1629.
While we decided the read-out-of-bounds in qemu_strtosz() is probably
not CVE-worthy (if you have control over the command line or QMP, you
can probably do much worse than cause a size parser to segfault), it
does raise the question of whether this patch is useful for
qemu-stable.  What's more, taking this patch in isolation without all
the prerequisite patches is probably sufficient to prevent the
read-out-of-bounds, but still leaves qemu_strtosz() with some odd
corner cases; while taking the whole series is a much bigger
committment but easier to analyze as a unit given that a lot of thte
series is testsuite unit test additions.  But either way, we'd need
reviews on this series if anyone thinks it warrants backports to
stable releases.

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