Re: [edk2-devel] [PATCH 1/1] OvmfPkg/QemuVideoDxe: purge VbeShim

["Laszlo Ersek" <lersek@redhat.com>]

On 2/13/24 15:02, Gerd Hoffmann wrote:
> On Tue, Feb 13, 2024 at 11:44:17AM +0100, Laszlo Ersek wrote:
>> Hi Gerd,
>>
>> On 2/13/24 09:59, Gerd Hoffmann wrote:
>>> Removing CSM support broke VbeShim.
>>>
>>> I see ASSERTS being triggered, both in vbeshim code and elsewhere.
>>> Didn't investigate in detail, but apparently the real mode address space
>>> is not considered being special any more, which invalidates some
>>> assumptions of the vbeshim code.
>>>
>>> The guest os which depends on vbeshim for video support is -- according
>>> to the comments -- Windows 2008 R2.  Which went EOL in January 2020,
>>> more than four years ago.
>>>
>>> Time to retire VbeShim.  RIP.
>>
>> I don't understand this, and/or disagree.
>>
>> (1) Please collect more information about the ASSERT()s (the failure mode you are seeing).
>> Because, (a) in the code that's being removed in this patch, I don't see any ASSERT() that should fire, and (b) I cannot reproduce the issue. (In fact my log contains "InstallVbeShim: VBE shim installed".)
> 
> Fair enough.  The ASSERT() I see firing is this:
> 
> ASSERT /home/kraxel/projects/edk2/OvmfPkg/QemuVideoDxe/VbeShim.c(142): Int0x10->Segment == 0x0000

Something nasty is going on here;

    138     //
    139     // We managed to allocate the page at zero. SVN r14218 guarantees that it
    140     // is NUL-filled.
    141     //
    142     ASSERT (Int0x10->Segment == 0x0000);

"SVN r14218" is git commit d436d5ca0936 -- "Guarantee that free memory in the 4K page starting at address 0 is always cleared to 0".

> 
> It reproduces for me when booting with a bunch of hot-pluggable CPUs (qemu -smp 2,maxcpus=32).
> 
> I bisected this,

Thank you!

> and it seems indeed totally unrelated to CSM removal:
> 
>    725acd0b9cc047b20a1b9379e46f05d3ebcf485e is the first bad commit
>    commit 725acd0b9cc047b20a1b9379e46f05d3ebcf485e
>    Author: Dun Tan <dun.tan@intel.com>
>    Date:   Fri Dec 1 10:53:32 2023 +0800
> 
>        UefiCpuPkg: Avoid assuming only one smmbasehob
> 
> New theory: This is a NULL pointer dereference corrupting the zero page.

Interesting; this commit is indeed related to having a multitude of (possible?) CPUs.

> 
> I still think it makes sense to retire VbeShim, but of course with a
> different commit message and also the leftover function declaration
> removed.

I agree, although in this case, the VBE shim has unexpectedly caught an independent bug for us ;)

> 
> Can we unmap the zero page to avoid NULL pointer dereferences going
> unnoticed (after removing VbeShim)?  IIRC I've seen an config option
> for that a while back, but can't find it now in the source tree ...

Yes. See "PcdNullPointerDetectionPropertyMask" in "MdeModulePkg/MdeModulePkg.dec".

See NULL_DETECTION_NONSTOP_MODE in CpuDxe (and PiSmmCpuDxeSmm). In CpuDxe, there's a fault handler that's supposed to dump the context to the serial port. From looking only briefly, I'm not sure where the actual unmapping happens (so that we get a page fault in the first place).

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115400): https://edk2.groups.io/g/devel/message/115400
Mute This Topic: https://groups.io/mt/104329097/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-