efifb: ignore frame buffer with physical address 0

[PATCH] efifb: ignore frame buffer with physical address 0

Posted by Roger Pau Monne 2 years ago

On one of my boxes when the HDMI cable is not plugged in the
FrameBufferBase of the EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE structure is
set to 0 by the firmware (while some of the other fields looking
plausible).

Such (bogus address) ends up mapped in vesa_init(), and since it
overlaps with a RAM region the whole system goes down pretty badly,
see:

(XEN) vesafb: framebuffer at 0x0000000000000000, mapped to 0xffff82c000201000, using 35209k, total 35209k
(XEN) vesafb: mode is 0x37557x32, linelength=960, font 8x16
(XEN) vesafb: Truecolor: size=8:8:8:8, shift=24:0:8:16
(XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) �ERROR: Class:0; Subclass:0; Operation: 0
ERROR: No ConOut
ERROR: No ConIn

Do like Linux and prevent using the EFI Frame Buffer if the base
address is 0.  This is inline with the logic in Linuxes
fb_base_is_valid() function at drivers/video/fbdev/efifb.c v6.0.9.

See also Linux commit 133bb070e94ab41d750c6f2160c8843e46f11b78 for
further reference.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Other options would be doing the check in vesa_init(), but that would
also then apply to other framebuffers and won't be strictly limited to
the EFI fb.

We could also check in vesa_init() whether the framebuffer overlaps
with any RAM region, but I think that should be in addition to the
change done here.
---
 xen/arch/x86/efi/efi-boot.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index e82ac9daa7..a68091d82a 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -552,7 +552,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
         bpp  = 0;
         break;
     }
-    if ( bpp > 0 )
+    if ( bpp > 0 && gop->Mode->FrameBufferBase )
     {
         vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
         vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
-- 
2.37.3

Re: [PATCH] efifb: ignore frame buffer with physical address 0

Posted by Jan Beulich 2 years ago

On 18.11.2022 13:39, Roger Pau Monne wrote:
> On one of my boxes when the HDMI cable is not plugged in the
> FrameBufferBase of the EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE structure is
> set to 0 by the firmware (while some of the other fields looking
> plausible).
> 
> Such (bogus address) ends up mapped in vesa_init(), and since it
> overlaps with a RAM region the whole system goes down pretty badly,
> see:
> 
> (XEN) vesafb: framebuffer at 0x0000000000000000, mapped to 0xffff82c000201000, using 35209k, total 35209k
> (XEN) vesafb: mode is 0x37557x32, linelength=960, font 8x16

Interesting mode - should we check for non-zero values there as well,
perhaps?

> (XEN) vesafb: Truecolor: size=8:8:8:8, shift=24:0:8:16
> (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) �ERROR: Class:0; Subclass:0; Operation: 0
> ERROR: No ConOut
> ERROR: No ConIn
> 
> Do like Linux and prevent using the EFI Frame Buffer if the base
> address is 0.  This is inline with the logic in Linuxes
> fb_base_is_valid() function at drivers/video/fbdev/efifb.c v6.0.9.
> 
> See also Linux commit 133bb070e94ab41d750c6f2160c8843e46f11b78 for
> further reference.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Other options would be doing the check in vesa_init(), but that would
> also then apply to other framebuffers and won't be strictly limited to
> the EFI fb.

Well, zero is wrong uniformly, so it wouldn't seem unreasonable to
put the check there. But I'm happy to keep it in EFI code for now.

> We could also check in vesa_init() whether the framebuffer overlaps
> with any RAM region, but I think that should be in addition to the
> change done here.

Indeed.

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -552,7 +552,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>          bpp  = 0;
>          break;
>      }
> -    if ( bpp > 0 )
> +    if ( bpp > 0 && gop->Mode->FrameBufferBase )
>      {
>          vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
>          vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */

A few lines up from here, just out of patch context, there is a
PrintErr() which imo is bogus/misleading when also encountering a
zero fb base. I'd like to suggest that you put the new check early
in the function (perhaps extended by a zero check of other
applicable fields, as per above), returning right away alongside
another new PrintErr().

Jan

Re: [PATCH] efifb: ignore frame buffer with physical address 0

Posted by Roger Pau Monné 2 years ago

On Fri, Nov 18, 2022 at 02:04:40PM +0100, Jan Beulich wrote:
> On 18.11.2022 13:39, Roger Pau Monne wrote:
> > On one of my boxes when the HDMI cable is not plugged in the
> > FrameBufferBase of the EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE structure is
> > set to 0 by the firmware (while some of the other fields looking
> > plausible).
> > 
> > Such (bogus address) ends up mapped in vesa_init(), and since it
> > overlaps with a RAM region the whole system goes down pretty badly,
> > see:
> > 
> > (XEN) vesafb: framebuffer at 0x0000000000000000, mapped to 0xffff82c000201000, using 35209k, total 35209k
> > (XEN) vesafb: mode is 0x37557x32, linelength=960, font 8x16
> 
> Interesting mode - should we check for non-zero values there as well,
> perhaps?

We could, yes, I went for what Linux currently does, but a height or
width of 0 is also likely wrong. We already check for bpp != 0.

> > (XEN) vesafb: Truecolor: size=8:8:8:8, shift=24:0:8:16
> > (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) �ERROR: Class:0; Subclass:0; Operation: 0
> > ERROR: No ConOut
> > ERROR: No ConIn
> > 
> > Do like Linux and prevent using the EFI Frame Buffer if the base
> > address is 0.  This is inline with the logic in Linuxes
> > fb_base_is_valid() function at drivers/video/fbdev/efifb.c v6.0.9.
> > 
> > See also Linux commit 133bb070e94ab41d750c6f2160c8843e46f11b78 for
> > further reference.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Other options would be doing the check in vesa_init(), but that would
> > also then apply to other framebuffers and won't be strictly limited to
> > the EFI fb.
> 
> Well, zero is wrong uniformly, so it wouldn't seem unreasonable to
> put the check there. But I'm happy to keep it in EFI code for now.
> 
> > We could also check in vesa_init() whether the framebuffer overlaps
> > with any RAM region, but I think that should be in addition to the
> > change done here.
> 
> Indeed.
> 
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -552,7 +552,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> >          bpp  = 0;
> >          break;
> >      }
> > -    if ( bpp > 0 )
> > +    if ( bpp > 0 && gop->Mode->FrameBufferBase )
> >      {
> >          vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
> >          vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
> 
> A few lines up from here, just out of patch context, there is a
> PrintErr() which imo is bogus/misleading when also encountering a
> zero fb base. I'd like to suggest that you put the new check early
> in the function (perhaps extended by a zero check of other
> applicable fields, as per above), returning right away alongside
> another new PrintErr().

Would you be fine with the new message being "Invalid Frame Buffer
configuration found"?

Thanks, Roger.

Re: [PATCH] efifb: ignore frame buffer with physical address 0

Posted by Jan Beulich 2 years ago

On 18.11.2022 14:44, Roger Pau Monné wrote:
> On Fri, Nov 18, 2022 at 02:04:40PM +0100, Jan Beulich wrote:
>> On 18.11.2022 13:39, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/efi/efi-boot.h
>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>> @@ -552,7 +552,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>>>          bpp  = 0;
>>>          break;
>>>      }
>>> -    if ( bpp > 0 )
>>> +    if ( bpp > 0 && gop->Mode->FrameBufferBase )
>>>      {
>>>          vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
>>>          vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
>>
>> A few lines up from here, just out of patch context, there is a
>> PrintErr() which imo is bogus/misleading when also encountering a
>> zero fb base. I'd like to suggest that you put the new check early
>> in the function (perhaps extended by a zero check of other
>> applicable fields, as per above), returning right away alongside
>> another new PrintErr().
> 
> Would you be fine with the new message being "Invalid Frame Buffer
> configuration found"?

Yeah, that'll probably do.

Jan