Re: [Qemu-devel] [PULL 15/30] opts: don't silently truncate long option values

[Peter Maydell]

On 8 May 2018 at 23:14, Paolo Bonzini <address@hidden> wrote:
> From: Daniel P. Berrangé <address@hidden>
>
> The existing QemuOpts parsing code uses a fixed size 1024 byte buffer
> for storing the option values. If a value exceeded this size it was
> silently truncated and no error reported to the user. Long option values
> is not a common scenario, but it is conceivable that they will happen.
> eg if the user has a very deeply nested filesystem it would be possible
> to come up with a disk path that was > 1024 bytes. Most of the time if
> such data was silently truncated, the user would get an error about
> opening a non-existant disk. If they're unlucky though, QEMU might use a
> completely different disk image from another VM, which could be
> considered a security issue. Another example program was in using the
> -smbios command line arg with very large data blobs. In this case the
> silent truncation will be providing semantically incorrect data to the
> guest OS for SMBIOS tables.
>
> If the operating system didn't limit the user's argv when spawning QEMU,
> the code should honour whatever length arguments were given without
> imposing its own length restrictions. This patch thus changes the code
> to use a heap allocated buffer for storing the values during parsing,
> lifting the arbitrary length restriction.

Hi; Coverity doesn't like this change (CID1391003):

> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -70,25 +70,37 @@ static const char *get_opt_name(const char *p, char 
> **option, char delim)
>   * delimiter is fixed to be comma which starts a new option. To specify an
>   * option value that contains commas, double each comma.
>   */
> -const char *get_opt_value(char *buf, int buf_size, const char *p)
> +const char *get_opt_value(const char *p, char **value)
>  {
> -    char *q;
> +    size_t capacity = 0, length;
> +    const char *offset;
> +
> +    *value = NULL;

Here we write to *value, so value must be non-NULL, and
within the loop the only place we write to value it
can't become NULL either (g_renew can't fail)...

> +    while (1) {
> +        offset = strchr(p, ',');
> +        if (!offset) {
> +            offset = p + strlen(p);
> +        }
>
> -    q = buf;
> -    while (*p != '\0') {
> -        if (*p == ',') {
> -            if (*(p + 1) != ',')
> -                break;
> -            p++;
> +        length = offset - p;
> +        if (*offset != '\0' && *(offset + 1) == ',') {
> +            length++;
> +        }
> +        if (value) {

...so this check for whether value is NULL can never be true.

> +            *value = g_renew(char, *value, capacity + length + 1);
> +            strncpy(*value + capacity, p, length);
> +            (*value)[capacity + length] = '\0';
> +        }
> +        capacity += length;
> +        if (*offset == '\0' ||
> +            *(offset + 1) != ',') {
> +            break;
>          }
> -        if (q && (q - buf) < buf_size - 1)
> -            *q++ = *p;
> -        p++;
> +
> +        p += (offset - p) + 2;
>      }
> -    if (q)
> -        *q = '\0';
>
> -    return p;
> +    return offset;
>  }
>

thanks
-- PMM