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

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Tue, Feb 13, 2024 at 2:56 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 13 Feb 2024 at 15:55, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > On Tue, Feb 13, 2024 at 2:02 PM Gerd Hoffmann <kraxel@redhat.com> 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
> > >
> > > It reproduces for me when booting with a bunch of hot-pluggable CPUs (qemu -smp 2,maxcpus=32).
> > >
> > > I bisected this, 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.
> >
> > Tangentially related: Maybe it's time to unmap the NULL page? :)
> >
>
>
> That would break many 3rd party GPU and network drivers.

Do they need the zero page to be writable, at least?
The fact that it's 2024 and most of EFI still has the zero page mapped
*and* writable is, uh, annoying to say the least. Even
write-protecting would be a great option.

IMO there's currently little incentive in fixing those 3rd party
blobs, because everything Just Works(tm), except when it doesn't. Like
it seems to have happened this time around.

(and to compound the problem, there's no sort of UBSAN or ASAN support
to catch these issues either. So you virtually can't find NULL pointer
derefs, unless the compiler can see it and trap)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115386): https://edk2.groups.io/g/devel/message/115386
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]
-=-=-=-=-=-=-=-=-=-=-=-