[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 15/30] opts: don't silently truncate long option
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PULL 15/30] opts: don't silently truncate long option values |
Date: |
Mon, 14 May 2018 17:23:37 +0100 |
User-agent: |
Mutt/1.9.5 (2018-04-13) |
On Mon, May 14, 2018 at 05:19:04PM +0100, Peter Maydell wrote:
> 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)...
Oh, real bug ! This should have been
if (value) {
*value = NULL;
}
because multiboot.c passes in NULL for this parameter.
Unless we decide to rewrite multiboot.c to avoid that instead,
since all other callers pass non-NULL.
>
> > + 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
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
- [Qemu-devel] [PULL 13/30] accel: use g_strsplit for parsing accelerator names, (continued)
- [Qemu-devel] [PULL 13/30] accel: use g_strsplit for parsing accelerator names, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 14/30] opts: don't silently truncate long parameter keys, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 17/30] qemu-options: Mark -virtioconsole as deprecated, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 18/30] qemu-options: Remove remainders of the -tdf option, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 19/30] qemu-options: Bail out on unsupported options instead of silently ignoring them, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 20/30] qemu-options: Remove deprecated -no-kvm-pit-reinjection, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 16/30] target/i386: sev: fix memory leaks, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 15/30] opts: don't silently truncate long option values, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 21/30] qemu-options: Remove deprecated -no-kvm-irqchip, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 25/30] configure: Really use local libfdt if the system one is too old, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 22/30] qemu-doc: provide details of supported build platforms, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 23/30] glib: bump min required glib library version to 2.42, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 27/30] shippable: Remove Debian 8 libfdt kludge, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 26/30] configure: Display if libfdt is from system or git, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 24/30] i386/kvm: add support for Hyper-V reenlightenment MSRs, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 28/30] build: Silence dtc directory creation, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 29/30] pc-dimm: fix error messages if no slots were defined, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 30/30] rename included C files to foo.inc.c, remove osdep.h, Paolo Bonzini, 2018/05/08