{xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

{xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 999 bytes --]

Hi,

I have a machine that allocate vesa LFB above 4GB, as reported by UEFI
GOP. At 0x4000000000 to be specific.
vga_console_info.u.vesa_lfb.lfb_base is a 32bit field, so it gets
truncated, leading to all kind of memory corruptions when something
writes there.
If that would be only about Xen, that wouldn't be that bad, but
unfortunately exactly the same structure is used as an interface for
dom0 start info (at least PV one).
My only idea is to introduce yet another entry in *_vga_console_info.u
union (efi_lfb64?) with a 64bit lfb_base field. And mark it in
video_type (XEN_VGATYPE_EFI_LFB64?). But I'm not sure how non-patched
Linux (or other supported OSes) would respond to this. xen_init_vga() in
Linux doesn't seem to bail on unknown video_type, so it may be fragile.

Any better ideas?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] {xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 999 bytes --]

Hi,

I have a machine that allocate vesa LFB above 4GB, as reported by UEFI
GOP. At 0x4000000000 to be specific.
vga_console_info.u.vesa_lfb.lfb_base is a 32bit field, so it gets
truncated, leading to all kind of memory corruptions when something
writes there.
If that would be only about Xen, that wouldn't be that bad, but
unfortunately exactly the same structure is used as an interface for
dom0 start info (at least PV one).
My only idea is to introduce yet another entry in *_vga_console_info.u
union (efi_lfb64?) with a 64bit lfb_base field. And mark it in
video_type (XEN_VGATYPE_EFI_LFB64?). But I'm not sure how non-patched
Linux (or other supported OSes) would respond to this. xen_init_vga() in
Linux doesn't seem to bail on unknown video_type, so it may be fragile.

Any better ideas?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: {xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

[Juergen Gross]

On 05/05/2019 15:27, Marek Marczykowski-Górecki wrote:
> Hi,
> 
> I have a machine that allocate vesa LFB above 4GB, as reported by UEFI
> GOP. At 0x4000000000 to be specific.
> vga_console_info.u.vesa_lfb.lfb_base is a 32bit field, so it gets
> truncated, leading to all kind of memory corruptions when something
> writes there.
> If that would be only about Xen, that wouldn't be that bad, but
> unfortunately exactly the same structure is used as an interface for
> dom0 start info (at least PV one).
> My only idea is to introduce yet another entry in *_vga_console_info.u
> union (efi_lfb64?) with a 64bit lfb_base field. And mark it in
> video_type (XEN_VGATYPE_EFI_LFB64?). But I'm not sure how non-patched
> Linux (or other supported OSes) would respond to this. xen_init_vga() in
> Linux doesn't seem to bail on unknown video_type, so it may be fragile.
> 
> Any better ideas?

In Linux kernel the screen_info structure has ext_lfb_base for that
purpose (it contains the upper 32 bits of lfb_base).

We could add a similar member to Xen's dom0_vga_console_info.u.vesa_lfb
and let the kernel detect its presence by using the value of
start_info.console.dom0.info_size - this wouldn't require a new video
type and old kernels would run as today. The same scheme is used for
gbl_caps and mode_attrs already.

Releated to that: I assume the comment in xen.h regarding lfb_base and
lfb_size being in units of 64kB is wrong and should be removed?


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] {xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

[Juergen Gross]

On 05/05/2019 15:27, Marek Marczykowski-Górecki wrote:
> Hi,
> 
> I have a machine that allocate vesa LFB above 4GB, as reported by UEFI
> GOP. At 0x4000000000 to be specific.
> vga_console_info.u.vesa_lfb.lfb_base is a 32bit field, so it gets
> truncated, leading to all kind of memory corruptions when something
> writes there.
> If that would be only about Xen, that wouldn't be that bad, but
> unfortunately exactly the same structure is used as an interface for
> dom0 start info (at least PV one).
> My only idea is to introduce yet another entry in *_vga_console_info.u
> union (efi_lfb64?) with a 64bit lfb_base field. And mark it in
> video_type (XEN_VGATYPE_EFI_LFB64?). But I'm not sure how non-patched
> Linux (or other supported OSes) would respond to this. xen_init_vga() in
> Linux doesn't seem to bail on unknown video_type, so it may be fragile.
> 
> Any better ideas?

In Linux kernel the screen_info structure has ext_lfb_base for that
purpose (it contains the upper 32 bits of lfb_base).

We could add a similar member to Xen's dom0_vga_console_info.u.vesa_lfb
and let the kernel detect its presence by using the value of
start_info.console.dom0.info_size - this wouldn't require a new video
type and old kernels would run as today. The same scheme is used for
gbl_caps and mode_attrs already.

Releated to that: I assume the comment in xen.h regarding lfb_base and
lfb_size being in units of 64kB is wrong and should be removed?


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: {xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

[Jan Beulich]

>>> On 06.05.19 at 10:04, <jgross@suse.com> wrote:
> On 05/05/2019 15:27, Marek Marczykowski-Górecki wrote:
>> Hi,
>> 
>> I have a machine that allocate vesa LFB above 4GB, as reported by UEFI
>> GOP. At 0x4000000000 to be specific.
>> vga_console_info.u.vesa_lfb.lfb_base is a 32bit field, so it gets
>> truncated, leading to all kind of memory corruptions when something
>> writes there.
>> If that would be only about Xen, that wouldn't be that bad, but
>> unfortunately exactly the same structure is used as an interface for
>> dom0 start info (at least PV one).
>> My only idea is to introduce yet another entry in *_vga_console_info.u
>> union (efi_lfb64?) with a 64bit lfb_base field. And mark it in
>> video_type (XEN_VGATYPE_EFI_LFB64?). But I'm not sure how non-patched
>> Linux (or other supported OSes) would respond to this. xen_init_vga() in
>> Linux doesn't seem to bail on unknown video_type, so it may be fragile.
>> 
>> Any better ideas?
> 
> In Linux kernel the screen_info structure has ext_lfb_base for that
> purpose (it contains the upper 32 bits of lfb_base).
> 
> We could add a similar member to Xen's dom0_vga_console_info.u.vesa_lfb
> and let the kernel detect its presence by using the value of
> start_info.console.dom0.info_size - this wouldn't require a new video
> type and old kernels would run as today. The same scheme is used for
> gbl_caps and mode_attrs already.

+1

> Releated to that: I assume the comment in xen.h regarding lfb_base and
> lfb_size being in units of 64kB is wrong and should be removed?

The remark applies to lfb_size only, and there it's correct: The
BIOS interface used hands back such a value and
efi_arch_video_init() adjusts the incoming byte-granular value
accordingly (albeit it's debatable whether rounding up is
appropriate).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] {xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

[Jan Beulich]

>>> On 06.05.19 at 10:04, <jgross@suse.com> wrote:
> On 05/05/2019 15:27, Marek Marczykowski-Górecki wrote:
>> Hi,
>> 
>> I have a machine that allocate vesa LFB above 4GB, as reported by UEFI
>> GOP. At 0x4000000000 to be specific.
>> vga_console_info.u.vesa_lfb.lfb_base is a 32bit field, so it gets
>> truncated, leading to all kind of memory corruptions when something
>> writes there.
>> If that would be only about Xen, that wouldn't be that bad, but
>> unfortunately exactly the same structure is used as an interface for
>> dom0 start info (at least PV one).
>> My only idea is to introduce yet another entry in *_vga_console_info.u
>> union (efi_lfb64?) with a 64bit lfb_base field. And mark it in
>> video_type (XEN_VGATYPE_EFI_LFB64?). But I'm not sure how non-patched
>> Linux (or other supported OSes) would respond to this. xen_init_vga() in
>> Linux doesn't seem to bail on unknown video_type, so it may be fragile.
>> 
>> Any better ideas?
> 
> In Linux kernel the screen_info structure has ext_lfb_base for that
> purpose (it contains the upper 32 bits of lfb_base).
> 
> We could add a similar member to Xen's dom0_vga_console_info.u.vesa_lfb
> and let the kernel detect its presence by using the value of
> start_info.console.dom0.info_size - this wouldn't require a new video
> type and old kernels would run as today. The same scheme is used for
> gbl_caps and mode_attrs already.

+1

> Releated to that: I assume the comment in xen.h regarding lfb_base and
> lfb_size being in units of 64kB is wrong and should be removed?

The remark applies to lfb_size only, and there it's correct: The
BIOS interface used hands back such a value and
efi_arch_video_init() adjusts the incoming byte-granular value
accordingly (albeit it's debatable whether rounding up is
appropriate).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: {xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 2660 bytes --]

On Mon, May 06, 2019 at 03:57:06AM -0600, Jan Beulich wrote:
> >>> On 06.05.19 at 10:04, <jgross@suse.com> wrote:
> > On 05/05/2019 15:27, Marek Marczykowski-Górecki wrote:
> >> Hi,
> >> 
> >> I have a machine that allocate vesa LFB above 4GB, as reported by UEFI
> >> GOP. At 0x4000000000 to be specific.
> >> vga_console_info.u.vesa_lfb.lfb_base is a 32bit field, so it gets
> >> truncated, leading to all kind of memory corruptions when something
> >> writes there.
> >> If that would be only about Xen, that wouldn't be that bad, but
> >> unfortunately exactly the same structure is used as an interface for
> >> dom0 start info (at least PV one).
> >> My only idea is to introduce yet another entry in *_vga_console_info.u
> >> union (efi_lfb64?) with a 64bit lfb_base field. And mark it in
> >> video_type (XEN_VGATYPE_EFI_LFB64?). But I'm not sure how non-patched
> >> Linux (or other supported OSes) would respond to this. xen_init_vga() in
> >> Linux doesn't seem to bail on unknown video_type, so it may be fragile.
> >> 
> >> Any better ideas?
> > 
> > In Linux kernel the screen_info structure has ext_lfb_base for that
> > purpose (it contains the upper 32 bits of lfb_base).
> > 
> > We could add a similar member to Xen's dom0_vga_console_info.u.vesa_lfb
> > and let the kernel detect its presence by using the value of
> > start_info.console.dom0.info_size - this wouldn't require a new video
> > type and old kernels would run as today. The same scheme is used for
> > gbl_caps and mode_attrs already.
> 
> +1

Makes sense.
That said, in Linux, VIDEO_CAPABILITY_64BIT_BASE is 2, same as COMPAT
bit Xen use in gbl_caps - which is later copied to
screen_info->capabilities by Linux... Another interaction is that, all
extra fields (gbl_caps, mode_attrs) are skipped with
XEN_VGATYPE_EFI_LFB. It will look a little confusing that bit 2 means
totally different things depending on video type. And to be honest, I'm
not sure if Linux wouldn't interpret COMPAT bit wrong here.

BTW another problem I have on this machine is the framebuffer size. It's
3840 x 2160, which is larger than max resolution hardcoded in
drivers/video/lfb.c (1920 x 1200). efi_find_gop_mode() chooses the
largest one, ignoring this limit. On one hand, it should take the limit
into account, but on another, increasing the limit looks quite harmless
(other than the console is quite slow) and not changing the mode during
boot looks better.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] {xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 2660 bytes --]

On Mon, May 06, 2019 at 03:57:06AM -0600, Jan Beulich wrote:
> >>> On 06.05.19 at 10:04, <jgross@suse.com> wrote:
> > On 05/05/2019 15:27, Marek Marczykowski-Górecki wrote:
> >> Hi,
> >> 
> >> I have a machine that allocate vesa LFB above 4GB, as reported by UEFI
> >> GOP. At 0x4000000000 to be specific.
> >> vga_console_info.u.vesa_lfb.lfb_base is a 32bit field, so it gets
> >> truncated, leading to all kind of memory corruptions when something
> >> writes there.
> >> If that would be only about Xen, that wouldn't be that bad, but
> >> unfortunately exactly the same structure is used as an interface for
> >> dom0 start info (at least PV one).
> >> My only idea is to introduce yet another entry in *_vga_console_info.u
> >> union (efi_lfb64?) with a 64bit lfb_base field. And mark it in
> >> video_type (XEN_VGATYPE_EFI_LFB64?). But I'm not sure how non-patched
> >> Linux (or other supported OSes) would respond to this. xen_init_vga() in
> >> Linux doesn't seem to bail on unknown video_type, so it may be fragile.
> >> 
> >> Any better ideas?
> > 
> > In Linux kernel the screen_info structure has ext_lfb_base for that
> > purpose (it contains the upper 32 bits of lfb_base).
> > 
> > We could add a similar member to Xen's dom0_vga_console_info.u.vesa_lfb
> > and let the kernel detect its presence by using the value of
> > start_info.console.dom0.info_size - this wouldn't require a new video
> > type and old kernels would run as today. The same scheme is used for
> > gbl_caps and mode_attrs already.
> 
> +1

Makes sense.
That said, in Linux, VIDEO_CAPABILITY_64BIT_BASE is 2, same as COMPAT
bit Xen use in gbl_caps - which is later copied to
screen_info->capabilities by Linux... Another interaction is that, all
extra fields (gbl_caps, mode_attrs) are skipped with
XEN_VGATYPE_EFI_LFB. It will look a little confusing that bit 2 means
totally different things depending on video type. And to be honest, I'm
not sure if Linux wouldn't interpret COMPAT bit wrong here.

BTW another problem I have on this machine is the framebuffer size. It's
3840 x 2160, which is larger than max resolution hardcoded in
drivers/video/lfb.c (1920 x 1200). efi_find_gop_mode() chooses the
largest one, ignoring this limit. On one hand, it should take the limit
into account, but on another, increasing the limit looks quite harmless
(other than the console is quite slow) and not changing the mode during
boot looks better.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: {xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

[Jan Beulich]

>>> On 06.05.19 at 12:29, <marmarek@invisiblethingslab.com> wrote:
> BTW another problem I have on this machine is the framebuffer size. It's
> 3840 x 2160, which is larger than max resolution hardcoded in
> drivers/video/lfb.c (1920 x 1200). efi_find_gop_mode() chooses the
> largest one, ignoring this limit. On one hand, it should take the limit
> into account, but on another, increasing the limit looks quite harmless
> (other than the console is quite slow) and not changing the mode during
> boot looks better.

Iirc in a prior discussion we had decided to drop the limit altogether
(and in fact all the MAX_* values living there), as there was nothing
found actually requiring there to be a limit. This probably was in the
context of some (perhaps draft / RFC) patch, so I'd guess whoever
it was then lost interest and didn't send a full patch as agreed upon
during that discussion.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] {xen, dom0}_vga_console_info.u.vesa_lfb.lfb_base field too small

[Jan Beulich]

>>> On 06.05.19 at 12:29, <marmarek@invisiblethingslab.com> wrote:
> BTW another problem I have on this machine is the framebuffer size. It's
> 3840 x 2160, which is larger than max resolution hardcoded in
> drivers/video/lfb.c (1920 x 1200). efi_find_gop_mode() chooses the
> largest one, ignoring this limit. On one hand, it should take the limit
> into account, but on another, increasing the limit looks quite harmless
> (other than the console is quite slow) and not changing the mode during
> boot looks better.

Iirc in a prior discussion we had decided to drop the limit altogether
(and in fact all the MAX_* values living there), as there was nothing
found actually requiring there to be a limit. This probably was in the
context of some (perhaps draft / RFC) patch, so I'd guess whoever
it was then lost interest and didn't send a full patch as agreed upon
during that discussion.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel