[PATCH 0/2] x86 Simplefb Fixes

[PATCH 0/2] x86 Simplefb Fixes

[David Herrmann]

Hi

Two fixes for the simplefb setup code in arch/x86/. One to support Matt's 64bit
EFI fb-base-address, and one to fix a longer standing verification bug.

Neither fix is critical. Additionally, most people still use efifb/vesafb
instead of simplefb, so probably does not affect any normal distribution.
However, SimpleDRM is close to being merged, so x86-sysfb might get into
some distros.

Thanks
David

David Herrmann (2):
  x86/sysfb: add support for 64bit EFI lfb_base
  x86/sysfb: fix lfb_size calculation

 arch/x86/kernel/sysfb_simplefb.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

-- 
2.10.2

[PATCH 1/2] x86/sysfb: add support for 64bit EFI lfb_base

[David Herrmann]

The screen_info object was extended to support 64bit lfb_base addresses
in:

    commit ae2ee627dc87a70910de91b791b3cd0e9c6facdd
    Author: Matt Fleming <matt.fleming@intel.com>
    Date:   Tue Aug 25 16:32:55 2015 +0100

        efifb: Add support for 64-bit frame buffer addresses

However, the x86 simple-framebuffer setup code never made use of it. Fix
it to properly assemble and verify the lfb_base before advertising
simple-framebuffer devices.

In particular, this means if VIDEO_CAPABILITY_64BIT_BASE is set, the
screen_info->ext_lfb_base field will contain the upper 32bit of the
actual lfb_base. Make sure the address is not 0 (i.e., unset), as well as
does not overflow the physical address type.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 arch/x86/kernel/sysfb_simplefb.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 764a29f..35b8641 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -67,6 +67,20 @@ __init int create_simplefb(const struct screen_info *si,
 	struct platform_device *pd;
 	struct resource res;
 	unsigned long len;
+	u64 base;
+
+	/*
+	 * If the 64BIT_BASE capability is set, ext_lfb_base will contain the
+	 * upper half of the base address. Assemble the address, then make sure
+	 * it is valid and we can actually access it.
+	 */
+	base = si->lfb_base;
+	if (si->capabilities & VIDEO_CAPABILITY_64BIT_BASE)
+		base |= (u64)si->ext_lfb_base << 32;
+	if (!base || (u64)(resource_size_t)base != base) {
+		printk(KERN_DEBUG "sysfb: inaccessible VRAM base\n");
+		return -EINVAL;
+	}
 
 	/* don't use lfb_size as it may contain the whole VMEM instead of only
 	 * the part that is occupied by the framebuffer */
@@ -81,8 +95,8 @@ __init int create_simplefb(const struct screen_info *si,
 	memset(&res, 0, sizeof(res));
 	res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
 	res.name = simplefb_resname;
-	res.start = si->lfb_base;
-	res.end = si->lfb_base + len - 1;
+	res.start = base;
+	res.end = res.start + len - 1;
 	if (res.end <= res.start)
 		return -EINVAL;
 
-- 
2.10.2

[PATCH 2/2] x86/sysfb: fix lfb_size calculation

[David Herrmann]

The screen_info.lfb_size field is shifted by 16 bits *only* in case of
VBE. This has historical reasons since VBE advertised it similarly.
However, in case of EFI framebuffers, the size is no longer shifted. Fix
the x86 simple-framebuffer setup code to use the correct size in the
non-VBE case.

While at it, avoid variable abbreviations and rename 'len' to 'length',
and use the correct types matching the screen_info definition.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 arch/x86/kernel/sysfb_simplefb.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 35b8641..85195d4 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -66,8 +66,8 @@ __init int create_simplefb(const struct screen_info *si,
 {
 	struct platform_device *pd;
 	struct resource res;
-	unsigned long len;
-	u64 base;
+	u64 base, size;
+	u32 length;
 
 	/*
 	 * If the 64BIT_BASE capability is set, ext_lfb_base will contain the
@@ -82,11 +82,20 @@ __init int create_simplefb(const struct screen_info *si,
 		return -EINVAL;
 	}
 
-	/* don't use lfb_size as it may contain the whole VMEM instead of only
-	 * the part that is occupied by the framebuffer */
-	len = mode->height * mode->stride;
-	len = PAGE_ALIGN(len);
-	if (len > (u64)si->lfb_size << 16) {
+	/*
+	 * Don't use lfb_size as IORESOURCE size, since it may contain the
+	 * entire VMEM, and thus require huge mappings. Use just the part we
+	 * need, that is, the part where the framebuffer is located. But verify
+	 * that it does not exceed the advertised VMEM.
+	 * Note that in case of VBE, the lfb_size is shifted by 16 bits for
+	 * historical reasons.
+	 */
+	size = si->lfb_size;
+	if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
+		size <<= 16;
+	length = mode->height * mode->stride;
+	length = PAGE_ALIGN(length);
+	if (length > size) {
 		printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n");
 		return -EINVAL;
 	}
@@ -96,7 +105,7 @@ __init int create_simplefb(const struct screen_info *si,
 	res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
 	res.name = simplefb_resname;
 	res.start = base;
-	res.end = res.start + len - 1;
+	res.end = res.start + length - 1;
 	if (res.end <= res.start)
 		return -EINVAL;
 
-- 
2.10.2

Re: [PATCH 1/2] x86/sysfb: add support for 64bit EFI lfb_base

[Tom Gundersen]

On Tue, Nov 15, 2016 at 1:01 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> The screen_info object was extended to support 64bit lfb_base addresses
> in:
>
>     commit ae2ee627dc87a70910de91b791b3cd0e9c6facdd
>     Author: Matt Fleming <matt.fleming@intel.com>
>     Date:   Tue Aug 25 16:32:55 2015 +0100
>
>         efifb: Add support for 64-bit frame buffer addresses
>
> However, the x86 simple-framebuffer setup code never made use of it. Fix
> it to properly assemble and verify the lfb_base before advertising
> simple-framebuffer devices.
>
> In particular, this means if VIDEO_CAPABILITY_64BIT_BASE is set, the
> screen_info->ext_lfb_base field will contain the upper 32bit of the
> actual lfb_base. Make sure the address is not 0 (i.e., unset), as well as
> does not overflow the physical address type.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Reviewed-by: Tom Gundersen <teg@jklm.no>

> ---
>  arch/x86/kernel/sysfb_simplefb.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
> index 764a29f..35b8641 100644
> --- a/arch/x86/kernel/sysfb_simplefb.c
> +++ b/arch/x86/kernel/sysfb_simplefb.c
> @@ -67,6 +67,20 @@ __init int create_simplefb(const struct screen_info *si,
>         struct platform_device *pd;
>         struct resource res;
>         unsigned long len;
> +       u64 base;
> +
> +       /*
> +        * If the 64BIT_BASE capability is set, ext_lfb_base will contain the
> +        * upper half of the base address. Assemble the address, then make sure
> +        * it is valid and we can actually access it.
> +        */
> +       base = si->lfb_base;
> +       if (si->capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> +               base |= (u64)si->ext_lfb_base << 32;
> +       if (!base || (u64)(resource_size_t)base != base) {
> +               printk(KERN_DEBUG "sysfb: inaccessible VRAM base\n");
> +               return -EINVAL;
> +       }
>
>         /* don't use lfb_size as it may contain the whole VMEM instead of only
>          * the part that is occupied by the framebuffer */
> @@ -81,8 +95,8 @@ __init int create_simplefb(const struct screen_info *si,
>         memset(&res, 0, sizeof(res));
>         res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>         res.name = simplefb_resname;
> -       res.start = si->lfb_base;
> -       res.end = si->lfb_base + len - 1;
> +       res.start = base;
> +       res.end = res.start + len - 1;
>         if (res.end <= res.start)
>                 return -EINVAL;
>
> --
> 2.10.2
>

Re: [PATCH 2/2] x86/sysfb: fix lfb_size calculation

[Tom Gundersen]

On Tue, Nov 15, 2016 at 1:01 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> The screen_info.lfb_size field is shifted by 16 bits *only* in case of
> VBE. This has historical reasons since VBE advertised it similarly.
> However, in case of EFI framebuffers, the size is no longer shifted. Fix
> the x86 simple-framebuffer setup code to use the correct size in the
> non-VBE case.
>
> While at it, avoid variable abbreviations and rename 'len' to 'length',
> and use the correct types matching the screen_info definition.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Reviewed-by: Tom Gundersen <teg@jklm.no>

> ---
>  arch/x86/kernel/sysfb_simplefb.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
> index 35b8641..85195d4 100644
> --- a/arch/x86/kernel/sysfb_simplefb.c
> +++ b/arch/x86/kernel/sysfb_simplefb.c
> @@ -66,8 +66,8 @@ __init int create_simplefb(const struct screen_info *si,
>  {
>         struct platform_device *pd;
>         struct resource res;
> -       unsigned long len;
> -       u64 base;
> +       u64 base, size;
> +       u32 length;
>
>         /*
>          * If the 64BIT_BASE capability is set, ext_lfb_base will contain the
> @@ -82,11 +82,20 @@ __init int create_simplefb(const struct screen_info *si,
>                 return -EINVAL;
>         }
>
> -       /* don't use lfb_size as it may contain the whole VMEM instead of only
> -        * the part that is occupied by the framebuffer */
> -       len = mode->height * mode->stride;
> -       len = PAGE_ALIGN(len);
> -       if (len > (u64)si->lfb_size << 16) {
> +       /*
> +        * Don't use lfb_size as IORESOURCE size, since it may contain the
> +        * entire VMEM, and thus require huge mappings. Use just the part we
> +        * need, that is, the part where the framebuffer is located. But verify
> +        * that it does not exceed the advertised VMEM.
> +        * Note that in case of VBE, the lfb_size is shifted by 16 bits for
> +        * historical reasons.
> +        */
> +       size = si->lfb_size;
> +       if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
> +               size <<= 16;
> +       length = mode->height * mode->stride;
> +       length = PAGE_ALIGN(length);
> +       if (length > size) {
>                 printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n");
>                 return -EINVAL;
>         }
> @@ -96,7 +105,7 @@ __init int create_simplefb(const struct screen_info *si,
>         res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>         res.name = simplefb_resname;
>         res.start = base;
> -       res.end = res.start + len - 1;
> +       res.end = res.start + length - 1;
>         if (res.end <= res.start)
>                 return -EINVAL;
>
> --
> 2.10.2
>

[tip:x86/urgent] x86/sysfb: Add support for 64bit EFI lfb_base

[tip-bot for David Herrmann]

Commit-ID:  9164b4ceb7b492a77c7fe770a4b9d1375c9cd45a
Gitweb:     http://git.kernel.org/tip/9164b4ceb7b492a77c7fe770a4b9d1375c9cd45a
Author:     David Herrmann <dh.herrmann@gmail.com>
AuthorDate: Tue, 15 Nov 2016 13:01:57 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 16 Nov 2016 09:38:22 +0100

x86/sysfb: Add support for 64bit EFI lfb_base

The screen_info object was extended to support 64-bit lfb_base addresses
in:

  ae2ee627dc87 ("efifb: Add support for 64-bit frame buffer addresses")

However, the x86 simple-framebuffer setup code never made use of it. Fix
it to properly assemble and verify the lfb_base before advertising
simple-framebuffer devices.

In particular, this means if VIDEO_CAPABILITY_64BIT_BASE is set, the
screen_info->ext_lfb_base field will contain the upper 32bit of the
actual lfb_base. Make sure the address is not 0 (i.e., unset), as well as
does not overflow the physical address type.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Tom Gundersen <teg@jklm.no>
Link: http://lkml.kernel.org/r/20161115120158.15388-2-dh.herrmann@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/sysfb_simplefb.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 764a29f..35b8641 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -67,6 +67,20 @@ __init int create_simplefb(const struct screen_info *si,
 	struct platform_device *pd;
 	struct resource res;
 	unsigned long len;
+	u64 base;
+
+	/*
+	 * If the 64BIT_BASE capability is set, ext_lfb_base will contain the
+	 * upper half of the base address. Assemble the address, then make sure
+	 * it is valid and we can actually access it.
+	 */
+	base = si->lfb_base;
+	if (si->capabilities & VIDEO_CAPABILITY_64BIT_BASE)
+		base |= (u64)si->ext_lfb_base << 32;
+	if (!base || (u64)(resource_size_t)base != base) {
+		printk(KERN_DEBUG "sysfb: inaccessible VRAM base\n");
+		return -EINVAL;
+	}
 
 	/* don't use lfb_size as it may contain the whole VMEM instead of only
 	 * the part that is occupied by the framebuffer */
@@ -81,8 +95,8 @@ __init int create_simplefb(const struct screen_info *si,
 	memset(&res, 0, sizeof(res));
 	res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
 	res.name = simplefb_resname;
-	res.start = si->lfb_base;
-	res.end = si->lfb_base + len - 1;
+	res.start = base;
+	res.end = res.start + len - 1;
 	if (res.end <= res.start)
 		return -EINVAL;
 

[tip:x86/urgent] x86/sysfb: Fix lfb_size calculation

[tip-bot for David Herrmann]

Commit-ID:  f96acec8c8020807429d21324547f4b904c37177
Gitweb:     http://git.kernel.org/tip/f96acec8c8020807429d21324547f4b904c37177
Author:     David Herrmann <dh.herrmann@gmail.com>
AuthorDate: Tue, 15 Nov 2016 13:01:58 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 16 Nov 2016 09:38:23 +0100

x86/sysfb: Fix lfb_size calculation

The screen_info.lfb_size field is shifted by 16 bits *only* in case of
VBE. This has historical reasons since VBE advertised it similarly.
However, in case of EFI framebuffers, the size is no longer shifted. Fix
the x86 simple-framebuffer setup code to use the correct size in the
non-VBE case.

While at it, avoid variable abbreviations and rename 'len' to 'length',
and use the correct types matching the screen_info definition.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Tom Gundersen <teg@jklm.no>
Link: http://lkml.kernel.org/r/20161115120158.15388-3-dh.herrmann@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/sysfb_simplefb.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 35b8641..85195d4 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -66,8 +66,8 @@ __init int create_simplefb(const struct screen_info *si,
 {
 	struct platform_device *pd;
 	struct resource res;
-	unsigned long len;
-	u64 base;
+	u64 base, size;
+	u32 length;
 
 	/*
 	 * If the 64BIT_BASE capability is set, ext_lfb_base will contain the
@@ -82,11 +82,20 @@ __init int create_simplefb(const struct screen_info *si,
 		return -EINVAL;
 	}
 
-	/* don't use lfb_size as it may contain the whole VMEM instead of only
-	 * the part that is occupied by the framebuffer */
-	len = mode->height * mode->stride;
-	len = PAGE_ALIGN(len);
-	if (len > (u64)si->lfb_size << 16) {
+	/*
+	 * Don't use lfb_size as IORESOURCE size, since it may contain the
+	 * entire VMEM, and thus require huge mappings. Use just the part we
+	 * need, that is, the part where the framebuffer is located. But verify
+	 * that it does not exceed the advertised VMEM.
+	 * Note that in case of VBE, the lfb_size is shifted by 16 bits for
+	 * historical reasons.
+	 */
+	size = si->lfb_size;
+	if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
+		size <<= 16;
+	length = mode->height * mode->stride;
+	length = PAGE_ALIGN(length);
+	if (length > size) {
 		printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n");
 		return -EINVAL;
 	}
@@ -96,7 +105,7 @@ __init int create_simplefb(const struct screen_info *si,
 	res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
 	res.name = simplefb_resname;
 	res.start = base;
-	res.end = res.start + len - 1;
+	res.end = res.start + length - 1;
 	if (res.end <= res.start)
 		return -EINVAL;