From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qt1-f193.google.com (mail-qt1-f193.google.com [209.85.160.193]) by mx.groups.io with SMTP id smtpd.web10.876.1587011671503195268 for ; Wed, 15 Apr 2020 21:34:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=VS7ovtwS; spf=pass (domain: gmail.com, ip: 209.85.160.193, mailfrom: hqm03ster@gmail.com) Received: by mail-qt1-f193.google.com with SMTP id c16so9675360qtv.1; Wed, 15 Apr 2020 21:34:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=QTjLpcwdoLQZOb+qTdh/A5lGACctspOpVK63giqxhtM=; b=VS7ovtwSAPwm/erVEArjbjitvtUlwRiroXIQI8GrQETeLOT2fWBNw6HcssNU6sFdnV igmo8dMUz+z6NWTCoZ3cuTPdQx05M1X0r2DNxeqB2jHSRU1vbTbEjsV/TQtCbmrxRveG TaUcBKVhzzdIVHd7IVPS67iLQ+LTyNBVB6nn/3KrjwL+TBR3NEhAPFCq4pNq7skbqrcZ reXXZ4aCDyBrv7jwNDuowV7WpbvQEtBnHE66p6qtAYNqd5gc96SSSLIWm7EUeEgv6xhu z2Ep9lOX90zdlgtvLguVJ5EnkaVicGRtaEKlKSOurOJcwHs3OjgeChCvZqNDp/fVAeYL pWIA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=QTjLpcwdoLQZOb+qTdh/A5lGACctspOpVK63giqxhtM=; b=ffQPYIQF5Hl1bJ0ohdW9e3VRutnnd0vRpWsp9GvgMupP+TmqefsSAKhzT5F/s/OGFh ZsW4RL3QIccVP8G7hiksscTUR1ePP2SKyfiJXDiD8CRBgLrTXd6lCt/u6YNzSKmKr6rv Mdma0zrukSmcu7pz3+jHiv71zrrTKhaMQ63A31SPXVJZOuuvUIp9TFJRRi8tYNKqD3b5 qOX+DJUCD9fmzrjUsH0oVJ9wt3SziyjhNiXgwuG3FgF5X7aLFlgyh2mPY0wwk3YVHmJj k5b3vDQdL94RbG67W+oBXKgi94ALu5l6awAnxYXed3lZWF2D3+RC1Gs6LGF3hZUWovE5 yTqA== X-Gm-Message-State: AGi0PuYTUJibdk3KHTMJZkwT6KlWRmdry4taovOP1e1UCu6HnbqsEtVT Cd4afcewrkD5yCSX4HMsjfYorXMZhMZR2KJtGm4= X-Google-Smtp-Source: APiQypJP7wngR55WKIV1un5OXko8VxihuR2EQjFH3y1VRNexDW4IYAxxka9MfEqQgUbECBcVkZR38brTYELcGuSbE44= X-Received: by 2002:ac8:7246:: with SMTP id l6mr24499713qtp.298.1587011670671; Wed, 15 Apr 2020 21:34:30 -0700 (PDT) MIME-Version: 1.0 References: <623b1855-285c-cce3-c806-c17e5fd217ea@redhat.com> <5211.1586899245384995995@groups.io> In-Reply-To: From: Hou Qiming Date: Thu, 16 Apr 2020 12:38:19 +0800 Message-ID: Subject: Re: [edk2-discuss] Load Option passing. Either bugs or my confusion. To: Laszlo Ersek Cc: valerij zaporogeci , discuss@edk2.groups.io, Gerd Hoffmann , "Marcel Apfelbaum (GMail address)" , edk2-devel-groups-io , qemu devel list Content-Type: multipart/alternative; boundary="000000000000e2864d05a360f2c6" --000000000000e2864d05a360f2c6 Content-Type: text/plain; charset="UTF-8" Very good point, I did neglect ramfb resolution changes... But there is one important thing: it *can* cause a QEMU crash, a potentially exploitable one, not always a guest crash. That's what motivated my heavy-handed approach since allowing resolution change would have necessitated a good deal of security checks. It has crashed my host *kernel* quite a few times. The point is, while the QemuRamfbDxe driver may behave properly, nothing prevents the guest from writing garbage or *malicious* values to the ramfb config space. Then the values are sent to the display component without any sanity check. For some GUI frontends, this means allocating an OpenGL texture with guest-supplied dimensions and uploading guest memory content to it, which means that guest memory content goes straight into a *kernel driver*, *completely unchecked*. Some integer overflow and a lenient GPU driver later, and the guest escapes straight to kernel. The proper way to enable ramfb resolution change again is adding sanity checks for ramfb resolution / pointer / etc. on the QEMU side. We have to make sure it doesn't exceed what the host GPU driver supports. Maybe clamp both width and height to between 1 and 2048? We also need to validate that OpenGL texture dimension update succeeds. Note that OpenGL is not obliged to validate anything and everything has to be checked on the QEMU side. Qiming On Wed, Apr 15, 2020 at 11:05 PM Laszlo Ersek wrote: > (CC Gerd, Qiming, Marcel, qemu-devel for ramfb:) > > On 04/14/20 23:20, valerij zaporogeci wrote: > > [snip] > > > There is a Boot Manager UI display problem, I don't know if this is > > qemu problem, but with the ARM (both 32 and 64 bits, the qemu version > > is 4.2.0, the OVMF is fresh), and using "ramfb" device, the Boot > > Manager has troubles with drawing - it's interfase looks entirely > > broken, like this (I'll try to attach the screenshot). UEFI shell > > doesn't have this problem. switching to "serial" (which is -serial vc) > > doesn't produce it too. Only when ramfb is chosen, the Boot Manager UI > > gets smeared. But it takes input and presumable works properly, except > > displaying things. qemu writes these messages in the command prompt: > > ramfb_fw_cfg_write: 640x480 @ 0x4bd00000 > > ramfb_fw_cfg_write: resolution locked, change rejected > > ramfb_fw_cfg_write: 800x600 @ 0x4bd00000 > > ramfb_fw_cfg_write: resolution locked, change rejected > > Gerd contributed the OVMF QemuRamfbDxe driver in edk2 commit > 1d25ff51af5c ("OvmfPkg: add QemuRamfbDxe", 2018-06-14). Note the date: > June 2018. > > The then-latest (released) QEMU version was v2.12.0, and v2.12.1 / > v3.0.0 were in the making. > > At that time, the resolution change definitely worked -- note my > "Tested-by" on the edk2 commit message. > > > Running "git blame" on the QEMU source, I now find commit a9e0cb67b7f4 > ("hw/display/ramfb: lock guest resolution after it's set", 2019-05-24). > > Again, note the date: May 2019 (and this commit was released with QEMU > v4.1.0). > > So I would say that the symptom you see is a QEMU v4.1.0 regression. The > QemuRamfbGraphicsOutputSetMode() function in the OVMF ramfb driver > certainly needs the QemuFwCfgWriteBytes() call to work, for changing the > resolution. > > > Now, I'm not familiar with the reasons behind QEMU commit a9e0cb67b7f4. > It says it intends to "prevent[] a crash when the guest writes garbage > to the configuration space (e.g. when rebooting)". > > But I don't understand why locking the resolution was necessary for > preventing "a crash": > > (1) Registering a device reset handler in QEMU seems sufficient, so that > QEMU forget about the currently shared RAMFB area at platform reset. > > (2) The crash in question is apparently not a *QEMU* crash -- which > might otherwise justify a heavy-handed approach. Instead, it is a > *guest* crash. See the references below: > > (2a) > http://mid.mail-archive.com/CABSdmrmU7FK90Bupq_ySowcc9Uk=8nQxNLHgzvDsNYdp_QLogA@mail.gmail.com > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg02299.html > > (2b) https://github.com/intel/gvt-linux/issues/23#issuecomment-483651476 > > Therefore, I don't think that locking the resolution was justified! > > Importantly: > > - The QemuRamfbDxe driver allocates the framebuffer in *reserved* > memory, therefore any well-behaving OS will *never* touch the > framebuffer. > > - The QemuRamfbDxe driver allocates the framebuffer memory only once, > namely for such a resolution that needs the largest amount of > framebuffer memory. Therefore, framebuffer re-allocations in the guest > driver -- and thereby guest RAM *re-mapping* in QEMU -- are *not* > necessary, upon resolution change. > > The ramfb device reset handler in QEMU is justified (for unmapping / > forgetting the previously shared RAMFB area). > > The resolution locking is *NOT* justified, and it breaks the OVMF > driver. I suggest backing out the resolution locking from QEMU. > > Reference (2a) above indicates 'It could be a misguided attempt to > "resize ramfb" by the guest Intel driver'. If that is the case, then > please fix the Intel guest driver, without regressing the QEMU device > model. > > I'm sad that the QEMU device model change was not regression-tested > against the *upstream* OVMF driver (which, by then, had been upstream > for almost a year). > > Laszlo > > --000000000000e2864d05a360f2c6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Very good point, I did neglect ramfb resolution chang= es... But there is one important thing: it *can* cause a QEMU crash, a pote= ntially exploitable one, not always a guest crash. That's what motivate= d my heavy-handed approach since allowing resolution change would have nece= ssitated a good deal of security checks. It has crashed my host *kernel* qu= ite a few times.

The point is, while=20 the QemuRamfbDxe driver may behave properly, nothing prevents the guest fro= m writing garbage or *malicious* values to the ramfb config space. Then the= values are sent to the display component without any sanity check. For som= e GUI frontends, this means allocating an OpenGL texture with guest-supplie= d dimensions and uploading guest memory content to it, which means that gue= st memory content goes straight into a *kernel driver*, *completely uncheck= ed*. Some integer overflow and a lenient GPU driver later, and the guest es= capes straight to kernel.

The proper way to enable= ramfb resolution change again is adding sanity checks for ramfb resolution= / pointer / etc. on the QEMU side. We have to make sure it doesn't exc= eed what the host GPU driver supports. Maybe clamp both width and height to between 1 and 2048? We also need to validate that OpenGL texture dimension update succeeds. No= te that OpenGL is not obliged to validate anything and everything has to be= checked on the QEMU side.

Qiming


On Wed, Apr 15, 2020 at 11:05 PM Laszlo Ersek <lersek@redhat.com> wrote:
(CC Gerd, Qiming, Marcel, qemu-dev= el for ramfb:)

On 04/14/20 23:20, valerij zaporogeci wrote:

[snip]

> There is a Boot Manager UI display problem, I don't know if this i= s
> qemu problem, but with the ARM (both 32 and 64 bits, the qemu version<= br> > is 4.2.0, the OVMF is fresh), and using "ramfb" device, the = Boot
> Manager has troubles with drawing - it's interfase looks entirely<= br> > broken, like this (I'll try to attach the screenshot). UEFI shell<= br> > doesn't have this problem. switching to "serial" (which = is -serial vc)
> doesn't produce it too. Only when ramfb is chosen, the Boot Manage= r UI
> gets smeared. But it takes input and presumable works properly, except=
> displaying things. qemu writes these messages in the command prompt: > ramfb_fw_cfg_write: 640x480 @ 0x4bd00000
> ramfb_fw_cfg_write: resolution locked, change rejected
> ramfb_fw_cfg_write: 800x600 @ 0x4bd00000
> ramfb_fw_cfg_write: resolution locked, change rejected

Gerd contributed the OVMF QemuRamfbDxe driver in edk2 commit
1d25ff51af5c ("OvmfPkg: add QemuRamfbDxe", 2018-06-14). Note the = date:
June 2018.

The then-latest (released) QEMU version was v2.12.0, and v2.12.1 /
v3.0.0 were in the making.

At that time, the resolution change definitely worked -- note my
"Tested-by" on the edk2 commit message.


Running "git blame" on the QEMU source, I now find commit a9e0cb6= 7b7f4
("hw/display/ramfb: lock guest resolution after it's set", 20= 19-05-24).

Again, note the date: May 2019 (and this commit was released with QEMU
v4.1.0).

So I would say that the symptom you see is a QEMU v4.1.0 regression. The QemuRamfbGraphicsOutputSetMode() function in the OVMF ramfb driver
certainly needs the QemuFwCfgWriteBytes() call to work, for changing the resolution.


Now, I'm not familiar with the reasons behind QEMU commit a9e0cb67b7f4.=
It says it intends to "prevent[] a crash when the guest writes garbage=
to the configuration space (e.g. when rebooting)".

But I don't understand why locking the resolution was necessary for
preventing "a crash":

(1) Registering a device reset handler in QEMU seems sufficient, so that QEMU forget about the currently shared RAMFB area at platform reset.

(2) The crash in question is apparently not a *QEMU* crash -- which
might otherwise justify a heavy-handed approach. Instead, it is a
*guest* crash. See the references below:

(2a) http://mid.mail-archive.com/CABSdmrmU7FK90Bupq_ySowcc9Uk=3D8nQxNLHgzvDsNYd= p_QLogA@mail.gmail.com
=C2=A0 =C2=A0 =C2=A0https://lists= .gnu.org/archive/html/qemu-devel/2019-05/msg02299.html

(2b) https://github.com/intel/gvt= -linux/issues/23#issuecomment-483651476

Therefore, I don't think that locking the resolution was justified!

Importantly:

- The QemuRamfbDxe driver allocates the framebuffer in *reserved*
memory, therefore any well-behaving OS will *never* touch the
framebuffer.

- The QemuRamfbDxe driver allocates the framebuffer memory only once,
namely for such a resolution that needs the largest amount of
framebuffer memory. Therefore, framebuffer re-allocations in the guest
driver -- and thereby guest RAM *re-mapping* in QEMU -- are *not*
necessary, upon resolution change.

The ramfb device reset handler in QEMU is justified (for unmapping /
forgetting the previously shared RAMFB area).

The resolution locking is *NOT* justified, and it breaks the OVMF
driver. I suggest backing out the resolution locking from QEMU.

Reference (2a) above indicates 'It could be a misguided attempt to
"resize ramfb" by the guest Intel driver'. If that is the cas= e, then
please fix the Intel guest driver, without regressing the QEMU device
model.

I'm sad that the QEMU device model change was not regression-tested
against the *upstream* OVMF driver (which, by then, had been upstream
for almost a year).

Laszlo

--000000000000e2864d05a360f2c6--