Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options

From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	qemu-devel@nongnu.org, Marcel Apfelbaum <marcel@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options
Date: Mon, 16 Apr 2018 19:42:00 +0100	[thread overview]
Message-ID: <20180416184200.GB14488@redhat.com> (raw)
In-Reply-To: <87lgdnkoix.fsf@dusky.pond.sub.org>

On Mon, Apr 16, 2018 at 08:13:42PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Apr 16, 2018 at 06:30:45PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > A user trying out SMBIOS "OEM strings" feature reported that the data
> >> > they are exposing to the guest was truncated at 1023 bytes, which breaks
> >> > the app consuming in the guest. After searching for the cause I
> >> > eventually found that the QemuOpts parsing is using fixed length 1024
> >> > byte array for option values and 128 byte array for key names.
> >> >
> >> > We can certainly debate whether it is sane to have such long command
> >> > line argument values (it is not sane), but if the OS was capable of
> >> > exec'ing QEMU with such an ARGV array, there is little good reason for
> >> > imposing an artificial length restriction when parsing it. Even worse is
> >> > that we silently truncate without reporting an error when hitting limits
> >> > resulting in a semantically incorrect behaviour, possibly even leading
> >> > to security flaws depending on the data that was truncated.
> >> >
> >> > Thus this patch series removes the artificial length limits by killing
> >> > the fixed length buffers.
> >> >
> >> > Separately I intend to make it possible to read "OEM strings" data from
> >> > a file, to avoid need to have long command line args.
> >> 
> >> Too bad I haven't been able to complete my quest to kill QemuOpts.
> >> 
> >> As far as I know, keyval.c's only arbitrary limit is the length of a key
> >> fragment (the things separated by '.').
> >
> > Looks like that's the same scenario I tried to address in patch 2. The
> > 'key' part in QemuOpts has the same 128 byte limit as in the keyval.c
> > code. I fear that could be hit with -blockdev when setting params on
> > very deeply nested block backends.  On the plus side  keyval.c actually
> > reports an error when it hits its 128 byte limit, instead of silently
> > carrying on as if all was well like QemuOpts did :-)
> 
> In keyval.c, the key (things like "a.b.c") can be arbitrarily long
> (well, until g_malloc() throws in the towel), but each key fragment
> ("a", "b" and "c") is limited to 128 bytes.
> 
> If key length was limited there, I would've asked you to fix it there,
> too.

Agreed, if only fragments are limited, that's fine because we know that
no code ever declares a key long enough to exceed the individual fragment
size.

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 :|