[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsin
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options |
Date: |
Mon, 16 Apr 2018 20:13:42 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Daniel P. Berrangé <address@hidden> writes:
> On Mon, Apr 16, 2018 at 06:30:45PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <address@hidden> 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.
- [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options, Daniel P . Berrangé, 2018/04/16
- [Qemu-devel] [PATCH 1/3] accel: use g_strsplit for parsing accelerator names, Daniel P . Berrangé, 2018/04/16
- [Qemu-devel] [PATCH 2/3] opts: don't silently truncate long parameter keys, Daniel P . Berrangé, 2018/04/16
- [Qemu-devel] [PATCH 3/3] opts: don't silently truncate long option values, Daniel P . Berrangé, 2018/04/16
- Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options, Paolo Bonzini, 2018/04/16
- Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options, Markus Armbruster, 2018/04/16