Bug 1749022

Upstream blurb:

[Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options
http://mid.mail-archive.com/20180416111743.8473-1-berrange@redhat.com

Commit 950c4e6c94b1 seems to have introduced a regression up-stream.

That was first fixed surgically in commit 6e3ad3f0e31b ("i386: fix
regression parsing multiboot initrd modules", 2018-07-17).

Then things got cleaned up more extensively in the two commits right
after:
- f8da93a0ffa0 ("i386: only parse the initrd_filename once for multiboot
  modules", 2018-07-17)
- 0c2f6e7ee995 ("opts: remove redundant check for NULL parameter",
  2018-07-17)

So in total we're looking at six commits:

1 20efc49ed625 accel: use g_strsplit for parsing accelerator names
2 e652714f98f2 opts: don't silently truncate long parameter keys
3 950c4e6c94b1 opts: don't silently truncate long option values
---
4 6e3ad3f0e31b i386: fix regression parsing multiboot initrd modules
5 f8da93a0ffa0 i386: only parse the initrd_filename once for multiboot
               modules
6 0c2f6e7ee995 opts: remove redundant check for NULL parameter

All of these are part of v3.0.0.

We certainly need #3 and #4.

I'm not sure about #1 and #2, but I'd wager those are pre-requisites --
at least "semantically", if not technically -- for #3.

#5 looks like an independent fix, just so #6 can undo #4. I'd exclude #5
and #6 from the backport, simply because I really don't want to touch
"multiboot". (My use case, BZ#1748180, has nothing to do with it.) But,
the backport targets 8.2.0, so people familiar with this part of QEMU
could disagree, and want to go for a complete backport of the second
series as well.

Note that in the upstream posting of the second series,

  [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules
  http://mid.mail-archive.com/20180514171913.17664-1-berrange@redhat.com

Dan confirmed, at

  http://mid.mail-archive.com/20180521085627.GD23090@redhat.com

that:

> I wanted to do a really simple fix that addressed the crasher
> regression, so that aspect of the fix wasn't obscured by the bigger
> refactoring I do in 2 & 3

I take this as evidence that *not* backporting #5 and #6 would be valid.

Dan, what's your opinion? Thanks.

(In reply to Laszlo Ersek from comment #2)

> 1 20efc49ed625 accel: use g_strsplit for parsing accelerator names
> 2 e652714f98f2 opts: don't silently truncate long parameter keys
> 3 950c4e6c94b1 opts: don't silently truncate long option values
> ---
> 4 6e3ad3f0e31b i386: fix regression parsing multiboot initrd modules
> 5 f8da93a0ffa0 i386: only parse the initrd_filename once for multiboot
>                modules
> 6 0c2f6e7ee995 opts: remove redundant check for NULL parameter

I did a mock backport (just with git-cherry-pick) of #1 through #4, on
top of "qemu-kvm-2.12.0-86.module+el8.1.0+4146+4ed2d185", to see if I'd
run into conflicts. git-cherry-pick resolved everything automatically,
the below is just what git-range-diff tells me after the fact.

#1 has a context difference in "accel/accel.c", function
configure_accelerator():

>     @@ -37,7 +39,7 @@
>           bool accel_initialised = false;
>           bool init_failed = false;
>      @@
>     -         accel = "tcg";
>     +         accel = "kvm:tcg";
>           }
>
>      -    p = accel;

It's due to downstream commit 8b53513834e6 ("Use kvm by default",
2018-04-24). Harmless.

#4 has a context difference in "util/qemu-option.c", function
get_opt_value():

>     @@ -46,5 +48,5 @@
>      +        *value = NULL;
>      +    }
>           while (1) {
>     -         offset = qemu_strchrnul(p, ',');
>     -         length = offset - p;
>     +         offset = strchr(p, ',');
>     +         if (!offset) {


That's because upstream has commit 5c99fa375da1 ("cutils: Provide
strchrnul", 2018-06-29), part of v3.0.0, but downstream lacks it.
Harmless -- the upstream commit only refactors get_opt_value().

Given Laszlo already tried the backport & showed it is simple, there's no reason to reject the request.

Hi Dan,

(In reply to Daniel Berrangé from comment #7)
> Given Laszlo already tried the backport & showed it is simple, there's
> no reason to reject the request.

my original needinfo for you was from comment 2 -- can you please read
that, and give me your opinion?

To reiterate, there are multiple subsets, of the full 6-patch set, that
are plausible for backporting. Full set:

  #1 20efc49ed625 accel: use g_strsplit for parsing accelerator names
  #2 e652714f98f2 opts: don't silently truncate long parameter keys
  #3 950c4e6c94b1 opts: don't silently truncate long option values
  ---
  #4 6e3ad3f0e31b i386: fix regression parsing multiboot initrd modules
  #5 f8da93a0ffa0 i386: only parse the initrd_filename once for
                  multiboot modules
  #6 0c2f6e7ee995 opts: remove redundant check for NULL parameter

Options (subsets) for backporting:

- #3 and #4 (this would be my preference -- it's minimal!)
- #1 through #4 (which is what I tried)
- #1 through #6 (which I would prefer not doing, but you could disagree)

My goal is to figure out the desired scope of the backport in the RHBZ,
so that, when I post the series to rhvirt-patches (after Brewing,
smoke-testing etc), it *not* be rejected just because I should have
backported #5 and #6 too.

Thanks!

(In reply to Laszlo Ersek from comment #10)

> Options (subsets) for backporting:
> 
> - #3 and #4 (this would be my preference -- it's minimal!)

Skipping leading patches in a upstream series is a red flag to me - it makes the backport unnecessarily diverge from the upstream series

> - #1 through #4 (which is what I tried)
> - #1 through #6 (which I would prefer not doing, but you could disagree)

Personally my preference is to backport full patch series unless conflicts make it problematic. Both of these options satisfy the bug report cleanly though, so I don't object to either

Hmm, okay, thanks. I guess I'll try #1 through #6 then -- we're only at 8.1, and if backporting f8da93a0ffa0 (#5) becomes necessary later (possibly out of order), it could get complicated. If I run into a conflict with #5 now, then I'll post #1-#4 only. Thanks!

Created attachment 1614501 [details]
ISO image with OVMF's EnrollDefaultKeys.efi built at edk2-stable201908

Verification steps for QE:

*** Setup (needs to be done only once):

(01) Download and decompress the attached file, to
     "EnrollDefaultKeys.X64.iso".

     This ISO image contains an "EnrollDefaultKeys.efi" binary, built
     from upstream edk2 at edk2-stable201908.

(02) Generate a new X509 certificate, to be enrolled as Platform Key /
     first Key Exchange Key:

     openssl req \
       -x509 \
       -newkey rsa:2048 \
       -outform PEM \
       -keyout PkKek1.private.key \
       -out PkKek1.pem

     (The details that OpenSSL asks for, interactively, are not
     important; enter whatever you like. A passphrase is required.)

(03) Slightly reformat the just created "PkKek1" file, for passing on
     the QEMU command line:

     sed \
       -e 's/^-----BEGIN CERTIFICATE-----$/4e32566d-8e9e-4f52-81d3-5bb9715f9727:/' \
       -e '/^-----END CERTIFICATE-----$/d' \
       PkKek1.pem \
     > PkKek1.oemstr

*** Test (used for reproducing the bug, and verifying the fix):

(04) Launch qemu-kvm as follows:

     /usr/libexec/qemu-kvm \
       -machine q35,smm=on,accel=tcg \
       \
       -nographic \
       \
       -fw_cfg name=opt/ovmf/PcdResizeXterm,string=y \
       \
       -global driver=cfi.pflash01,property=secure,value=on \
       -drive if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/edk2/ovmf/OVMF_CODE.secboot.fd \
       -drive if=pflash,unit=1,format=raw,snapshot=on,file=/usr/share/edk2/ovmf/OVMF_VARS.fd \
       \
       -global isa-debugcon.iobase=0x402 \
       -debugcon file:debug.log \
       \
       -drive id=shell-iso,if=none,format=raw,readonly=on,file=/usr/share/edk2/ovmf/UefiShell.iso \
       -drive id=enroll-iso,if=none,format=raw,readonly=on,file=EnrollDefaultKeys.X64.iso \
       \
       -device virtio-scsi-pci,id=scsi0 \
       -device scsi-cd,bus=scsi0.0,lun=1,drive=shell-iso,bootindex=0 \
       -device scsi-cd,bus=scsi0.0,lun=2,drive=enroll-iso \
       \
       -smbios type=11,value="$(< PkKek1.oemstr)"

(05) In the UEFI shell, enter the following commands:

     connect -r
     map -u
     fs1:
     dir

     Verify that the DIR command lists the following:

> 09/12/2019  11:48              31,616  EnrollDefaultKeys.efi

     (The goal with the DIR command is to ensure that, in the current
     working directory, we're looking at the "EnrollDefaultKeys.efi"
     file from the attachment -- the drive named "enroll-iso" --, and
     not from the system-wide edk2-ovmf package named (i.e., the drive
     named "shell-iso").)

(06) Run the following command:

     EnrollDefaultKeys

(07) Power down the VM with the following UEFI shell command:

     reset -s

*** Actual result:

Step (06) prints:

> info: SetupMode=1 SecureBoot=0 SecureBootEnable=0 CustomMode=0 VendorKeys=1
> error: invalid base64 string after app prefix 4E32566D-8E9E-4F52-81D3-5BB9715F9727

*** Expected result:

Step (06) prints:

> info: SetupMode=1 SecureBoot=0 SecureBootEnable=0 CustomMode=0 VendorKeys=1
> info: SetupMode=0 SecureBoot=1 SecureBootEnable=1 CustomMode=0 VendorKeys=0
> info: success

Note to QE:

Dependent on the certificate generated in comment#14 step (02), the
error message ("Actual result") from comment#14 step (06) may be

> error: EnrollListOfCerts("KEK", 8BE4DF61-93CA-11D2-AA0D-00E098032B8C): Invalid Parameter

instead of

> error: invalid base64 string after app prefix 4E32566D-8E9E-4F52-81D3-5BB9715F9727

This "EnrollListOfCerts" error message is just another symptom of the
same issue, and it confirms the bug just the same.

Qunfang Zhang,

can you please grant qa_ack+ for this RHBZ?

Please see the reproduction and verification steps in comment#14 and comment#15.

Thanks,
Laszlo

According to Comment 14, reproduced it on qemu-kvm-2.12.0-88.module+el8.1.0+4233+bc44be3f

After step 6, 
print:
FS1:\> EnrollDefaultKeys
info: SetupMode=1 SecureBoot=0 SecureBootEnable=0 CustomMode=0 VendorKeys=1
error: invalid base64 string after app prefix 4E32566D-8E9E-4F52-81D3-5BB9715F9727


Retested on qemu-kvm-2.12.0-89.module+el8.2.0+4436+f3a2188d, not hit this issue. So set status to VERIFIED.


Versions:
kernel-4.18.0-147.6.el8.x86_64
qemu-kvm-2.12.0-89.module+el8.2.0+4436+f3a2188d
edk2-ovmf-20190308git89910a39dcfd-6.el8.noarch


After step 6,
print:
FS1:\> EnrollDefaultKeys
info: SetupMode=1 SecureBoot=0 SecureBootEnable=0 CustomMode=0 VendorKeys=1
info: SetupMode=0 SecureBoot=1 SecureBootEnable=1 CustomMode=0 VendorKeys=0
info: success

QEMU has been recently split into sub-components and as a one-time operation to avoid breakage of tools, we are setting the QEMU sub-component of this BZ to "General". Please review and change the sub-component if necessary the next time you review this BZ. Thanks

IMO "QMP Monitor and CLI" is a better fit, as the issue was that long option values passed on the command line got truncated.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2020:1587