[PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered

The efifb and simplefb drivers just render to a pre-allocated frame buffer
and rely on the display hardware being initialized before the kernel boots.

But if another driver already probed correctly and registered a fbdev, the
generic drivers shouldn't be probed since an actual driver for the display
hardware is already present.

This is more likely to occur after commit d391c5827107 ("drivers/firmware:
move x86 Generic System Framebuffers support") since the "efi-framebuffer"
and "simple-framebuffer" platform devices are registered at a later time.

Link: https://lore.kernel.org/r/20211110200253.rfudkt3edbd3nsyj@lahvuun/
Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers support")
Reported-by: Ilya Trukhanov <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Vetter <redacted>
---

Changes in v2:
- Add a Link: tag with a reference to the bug report (Thorsten Leemhuis).
- Add a comment explaining why the probe fails earlier (Daniel Vetter).
- Add a Fixes: tag for stable to pick the fix (Daniel Vetter).
- Add Daniel Vetter's Reviewed-by: tag.
- Improve the commit message and mention the culprit commit

 drivers/video/fbdev/efifb.c    | 11 +++++++++++
 drivers/video/fbdev/simplefb.c | 11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git drivers/video/fbdev/efifb.c drivers/video/fbdev/efifb.c
index edca3703b964..ea42ba6445b2 100644
--- drivers/video/fbdev/efifb.c
+++ drivers/video/fbdev/efifb.c

@@ -351,6 +351,17 @@ static int efifb_probe(struct platform_device *dev)
 	char *option = NULL;
 	efi_memory_desc_t md;
 
+	/*
+	 * Generic drivers must not be registered if a framebuffer exists.
+	 * If a native driver was probed, the display hardware was already
+	 * taken and attempting to use the system framebuffer is dangerous.
+	 */
+	if (num_registered_fb > 0) {
+		dev_err(&dev->dev,
+			"efifb: a framebuffer is already registered\n");
+		return -EINVAL;
+	}
+
 	if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled)
 		return -ENODEV;
 

diff --git drivers/video/fbdev/simplefb.c drivers/video/fbdev/simplefb.c
index 62f0ded70681..b63074fd892e 100644
--- drivers/video/fbdev/simplefb.c
+++ drivers/video/fbdev/simplefb.c

@@ -407,6 +407,17 @@ static int simplefb_probe(struct platform_device *pdev)
 	struct simplefb_par *par;
 	struct resource *mem;
 
+	/*
+	 * Generic drivers must not be registered if a framebuffer exists.
+	 * If a native driver was probed, the display hardware was already
+	 * taken and attempting to use the system framebuffer is dangerous.
+	 */
+	if (num_registered_fb > 0) {
+		dev_err(&pdev->dev,
+			"simplefb: a framebuffer is already registered\n");
+		return -EINVAL;
+	}
+
 	if (fb_get_options("simplefb", NULL))
 		return -ENODEV;
 

-- 
2.33.1

On Thu, Nov 11, 2021 at 12:11:20PM +0100, Javier Martinez Canillas wrote:

The efifb and simplefb drivers just render to a pre-allocated frame buffer
and rely on the display hardware being initialized before the kernel boots.

But if another driver already probed correctly and registered a fbdev, the
generic drivers shouldn't be probed since an actual driver for the display
hardware is already present.

This is more likely to occur after commit d391c5827107 ("drivers/firmware:
move x86 Generic System Framebuffers support") since the "efi-framebuffer"
and "simple-framebuffer" platform devices are registered at a later time.

Link: https://lore.kernel.org/r/20211110200253.rfudkt3edbd3nsyj@lahvuun/
Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers support")
Reported-by: Ilya Trukhanov <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Vetter <redacted>
---

Changes in v2:
- Add a Link: tag with a reference to the bug report (Thorsten Leemhuis).
- Add a comment explaining why the probe fails earlier (Daniel Vetter).
- Add a Fixes: tag for stable to pick the fix (Daniel Vetter).

That does not mean that it will make it into the stable tree.  Please
read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

thanks,

greg k-h

On Thu, Nov 11, 2021 at 12:39:28PM +0100, Greg Kroah-Hartman wrote:

On Thu, Nov 11, 2021 at 12:11:20PM +0100, Javier Martinez Canillas wrote:

quoted

The efifb and simplefb drivers just render to a pre-allocated frame buffer
and rely on the display hardware being initialized before the kernel boots.

But if another driver already probed correctly and registered a fbdev, the
generic drivers shouldn't be probed since an actual driver for the display
hardware is already present.

This is more likely to occur after commit d391c5827107 ("drivers/firmware:
move x86 Generic System Framebuffers support") since the "efi-framebuffer"
and "simple-framebuffer" platform devices are registered at a later time.

Link: https://lore.kernel.org/r/20211110200253.rfudkt3edbd3nsyj@lahvuun/
Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers support")
Reported-by: Ilya Trukhanov <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Vetter <redacted>
---

Changes in v2:
- Add a Link: tag with a reference to the bug report (Thorsten Leemhuis).
- Add a comment explaining why the probe fails earlier (Daniel Vetter).
- Add a Fixes: tag for stable to pick the fix (Daniel Vetter).

That does not mean that it will make it into the stable tree.  Please
read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

Defacto your auto-picker is aggressive enough that just Fixes: is actually
good enough to get it into stable :-)

But yeah explicit cc: stable can't hurt.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Hi Javier,

On Thu, Nov 11, 2021 at 12:13 PM Javier Martinez Canillas
[off-list ref] wrote:

Who says this registered fbdev is driving the same hardware as efifb?
This might be e.g. a small external display connected to i2c or spi.

Likewise.

+               dev_err(&pdev->dev,
+                       "simplefb: a framebuffer is already registered\n");
+               return -EINVAL;
+       }
+
        if (fb_get_options("simplefb", NULL))
                return -ENODEV;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Hello Geert,

On 11/15/21 17:20, Geert Uytterhoeven wrote:

[snip]

quoted

@@ -351,6 +351,17 @@ static int efifb_probe(struct platform_device *dev)
        char *option = NULL;
        efi_memory_desc_t md;

+       /*
+        * Generic drivers must not be registered if a framebuffer exists.
+        * If a native driver was probed, the display hardware was already
+        * taken and attempting to use the system framebuffer is dangerous.
+        */
+       if (num_registered_fb > 0) {

Who says this registered fbdev is driving the same hardware as efifb?
This might be e.g. a small external display connected to i2c or spi.

quoted

+               dev_err(&dev->dev,
+                       "efifb: a framebuffer is already registered\n");
+               return -EINVAL;
+       }
+

That's true, although I wonder if the {efi,simple}fb drivers should even be
probed in that case. As I see it, these are always a best effort that are
only useful for earlycon or if there isn't another display driver supported.

Since there may be other conditions needed in order for these to work. For
example, when using the u-boot EFI stub in most cases the unused clocks and
power domains can't be gated or otherwise the firmware frame buffer could go
away (e.g: will need to boot with "clk_ignore_unused" and "pd_ignore_unused").

Same for the simplefb driver, if the DT node is missing resources that are
needed by the display controller to continue working (clocks, regulators,
power domains), the firmware setup framebuffer will go away at some point.

So this is already a fragile solution and $SUBJECT doesn't make things worse
IMO. Since not having something like this can lead to issues as reported by:

https://lore.kernel.org/all/20211110200253.rfudkt3edbd3nsyj@lahvuun/

We could probably do some smarter here by providing a function that checks
if the registered fbdev drivers matches the aperture base. But I'm unsure
if that's worth it. After all, fbdev drivers are likely to be disabled by
most distros soon now that we have the simpledrm driver.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Hi Javier,

On Tue, Nov 16, 2021 at 10:30 AM Javier Martinez Canillas
[off-list ref] wrote:

On 11/15/21 17:20, Geert Uytterhoeven wrote:

quoted

quoted

@@ -351,6 +351,17 @@ static int efifb_probe(struct platform_device *dev)
        char *option = NULL;
        efi_memory_desc_t md;

+       /*
+        * Generic drivers must not be registered if a framebuffer exists.
+        * If a native driver was probed, the display hardware was already
+        * taken and attempting to use the system framebuffer is dangerous.
+        */
+       if (num_registered_fb > 0) {

Who says this registered fbdev is driving the same hardware as efifb?
This might be e.g. a small external display connected to i2c or spi.

quoted

+               dev_err(&dev->dev,
+                       "efifb: a framebuffer is already registered\n");
+               return -EINVAL;
+       }
+

That's true, although I wonder if the {efi,simple}fb drivers should even be
probed in that case. As I see it, these are always a best effort that are
only useful for earlycon or if there isn't another display driver supported.

Since there may be other conditions needed in order for these to work. For
example, when using the u-boot EFI stub in most cases the unused clocks and
power domains can't be gated or otherwise the firmware frame buffer could go
away (e.g: will need to boot with "clk_ignore_unused" and "pd_ignore_unused").

Same for the simplefb driver, if the DT node is missing resources that are
needed by the display controller to continue working (clocks, regulators,
power domains), the firmware setup framebuffer will go away at some point.

So this is already a fragile solution and $SUBJECT doesn't make things worse
IMO. Since not having something like this can lead to issues as reported by:

https://lore.kernel.org/all/20211110200253.rfudkt3edbd3nsyj@lahvuun/

We could probably do some smarter here by providing a function that checks
if the registered fbdev drivers matches the aperture base. But I'm unsure
if that's worth it. After all, fbdev drivers are likely to be disabled by
most distros soon now that we have the simpledrm driver.

Checking the aperture base is what was done in all other cases of
preventing generic (fbdev) drivers from stepping on specific drivers'
toes...

But as you're only impacting efifb and simplefb, thus not crippling
generic fbdev support, I don't care that much.


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Hello Geert,

On 11/16/21 10:43, Geert Uytterhoeven wrote:

[snip]

quoted

So this is already a fragile solution and $SUBJECT doesn't make things worse
IMO. Since not having something like this can lead to issues as reported by:

https://lore.kernel.org/all/20211110200253.rfudkt3edbd3nsyj@lahvuun/

We could probably do some smarter here by providing a function that checks
if the registered fbdev drivers matches the aperture base. But I'm unsure
if that's worth it. After all, fbdev drivers are likely to be disabled by
most distros soon now that we have the simpledrm driver.

Checking the aperture base is what was done in all other cases of
preventing generic (fbdev) drivers from stepping on specific drivers'
toes...

Ok, I can re-spin the patch checking if the aperture ranges overlap. There's
an apertures_overlap() function in drivers/video/fbdev/core/fbmem.c that can
be exported for fbdev drivers to use.

Another option is to just say that DRM drivers should be built as a module if
the {efi,simple}fb driver are built-in.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

On 11/16/21 11:01, Javier Martinez Canillas wrote:

Hello Geert,

On 11/16/21 10:43, Geert Uytterhoeven wrote:

[snip]

quoted

quoted

So this is already a fragile solution and $SUBJECT doesn't make things worse
IMO. Since not having something like this can lead to issues as reported by:

https://lore.kernel.org/all/20211110200253.rfudkt3edbd3nsyj@lahvuun/

We could probably do some smarter here by providing a function that checks
if the registered fbdev drivers matches the aperture base. But I'm unsure
if that's worth it. After all, fbdev drivers are likely to be disabled by
most distros soon now that we have the simpledrm driver.

Checking the aperture base is what was done in all other cases of
preventing generic (fbdev) drivers from stepping on specific drivers'
toes...

Ok, I can re-spin the patch checking if the aperture ranges overlap. There's
an apertures_overlap() function in drivers/video/fbdev/core/fbmem.c that can
be exported for fbdev drivers to use.

So I tried the following patch [0]. But when testing on a VM, the efifb driver
is probed even after the virtio_gpu driver has already been probed. Being a DRM
driver, it doesn't use the fbdev infra and AFAIU doesn't reserve any apertures.

When the {efi,simple}fb drivers check if there's an aperture already reserved
using the fb_aperture_registered() helper, this just returns false even when a
driver for the same hardware was already registered. The kernel log says:

[    0.891512] checking generic (0 0) vs hw (c0000000 1d5000)

That is because when DRM_FBDEV_EMULATION=y, the virtio_gpu driver registers an
fbdev but without any aperture set.

I discussed this with Thomas and even though $SUBJECT is just a workaround, it
seems that is the best we can do as an heuristic to prevent the generic fbdev
drivers to be probed after a native DRM driver.

[0]:
From d962c20bc9fd90c2525d79b69e632d99e8050fc5 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Thu, 11 Nov 2021 00:55:06 +0100
Subject: [PATCH v4] fbdev: Prevent probing generic drivers if a FB is already
 registered

The efifb and simplefb drivers just render to a pre-allocated frame buffer
and rely on the display hardware being initialized before the kernel boots.

But if another driver already probed correctly and registered a fbdev, the
generic drivers shouldn't be probed since an actual driver for the display
hardware is already present.

This is more likely to occur after commit d391c5827107 ("drivers/firmware:
move x86 Generic System Framebuffers support") since the "efi-framebuffer"
and "simple-framebuffer" platform devices are registered at a later time.

Link: https://lore.kernel.org/r/20211110200253.rfudkt3edbd3nsyj@lahvuun/
Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers support")
Reported-by: Ilya Trukhanov <redacted>
Cc: <redacted> # 5.15.x
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v4:
- Only fail to probe if a registered fbdev has overlapping aperture (Geert).

Changes in v3:
- Cc [off-list ref] since a Fixes: tag is not enough (gregkh).

Changes in v2:
- Add a Link: tag with a reference to the bug report (Thorsten Leemhuis).
- Add a comment explaining why the probe fails earlier (Daniel Vetter).
- Add a Fixes: tag for stable to pick the fix (Daniel Vetter).
- Add Daniel Vetter's Reviewed-by: tag.
- Improve the commit message and mention the culprit commit

 drivers/video/fbdev/core/fbmem.c | 16 ++++++++++++++++
 drivers/video/fbdev/efifb.c      | 11 +++++++++++
 drivers/video/fbdev/simplefb.c   | 11 +++++++++++
 include/linux/fb.h               |  1 +
 4 files changed, 39 insertions(+)

diff --git drivers/video/fbdev/core/fbmem.c drivers/video/fbdev/core/fbmem.c
index 826175ad88a2..9906b83132cb 100644
--- drivers/video/fbdev/core/fbmem.c
+++ drivers/video/fbdev/core/fbmem.c

@@ -1546,6 +1546,22 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena,
 	return false;
 }
 
+bool fb_aperture_registered(struct apertures_struct *a)
+{
+	int i;
+
+	for_each_registered_fb(i) {
+		struct apertures_struct *gen_aper;
+
+		gen_aper = registered_fb[i]->apertures;
+		if (fb_do_apertures_overlap(gen_aper, a))
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(fb_aperture_registered);
+
 static void do_unregister_framebuffer(struct fb_info *fb_info);
 
 #define VGA_FB_PHYS 0xA0000

diff --git drivers/video/fbdev/efifb.c drivers/video/fbdev/efifb.c
index edca3703b964..1ad6698b2e05 100644
--- drivers/video/fbdev/efifb.c
+++ drivers/video/fbdev/efifb.c

@@ -457,6 +457,17 @@ static int efifb_probe(struct platform_device *dev)
 	info->apertures->ranges[0].base = efifb_fix.smem_start;
 	info->apertures->ranges[0].size = size_remap;
 
+	/*
+	 * Generic drivers must not be registered if a framebuffer exists.
+	 * If a native driver was probed, the display hardware was already
+	 * taken and attempting to use the system framebuffer is dangerous.
+	 */
+	if (fb_aperture_registered(info->apertures)) {
+		dev_err(&dev->dev,
+			"efifb: a framebuffer is already registered\n");
+		return -EINVAL;
+	}
+
 	if (efi_enabled(EFI_MEMMAP) &&
 	    !efi_mem_desc_lookup(efifb_fix.smem_start, &md)) {
 		if ((efifb_fix.smem_start + efifb_fix.smem_len) >

diff --git drivers/video/fbdev/simplefb.c drivers/video/fbdev/simplefb.c
index 62f0ded70681..3ad0f538ca91 100644
--- drivers/video/fbdev/simplefb.c
+++ drivers/video/fbdev/simplefb.c

@@ -456,6 +456,17 @@ static int simplefb_probe(struct platform_device *pdev)
 	info->apertures->ranges[0].base = info->fix.smem_start;
 	info->apertures->ranges[0].size = info->fix.smem_len;
 
+	/*
+	 * Generic drivers must not be registered if a framebuffer exists.
+	 * If a native driver was probed, the display hardware was already
+	 * taken and attempting to use the system framebuffer is dangerous.
+	 */
+	if (fb_aperture_registered(info->apertures)) {
+		dev_err(&pdev->dev,
+			"simplefb: a framebuffer is already registered\n");
+		return -EINVAL;
+	}
+
 	info->fbops = &simplefb_ops;
 	info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
 	info->screen_base = ioremap_wc(info->fix.smem_start,

diff --git include/linux/fb.h include/linux/fb.h
index 6f3db99ab990..f1fbdb39932b 100644
--- include/linux/fb.h
+++ include/linux/fb.h

@@ -604,6 +604,7 @@ extern ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
 			    size_t count, loff_t *ppos);
 
 /* drivers/video/fbmem.c */
+extern bool fb_aperture_registered(struct apertures_struct *a);
 extern int register_framebuffer(struct fb_info *fb_info);
 extern void unregister_framebuffer(struct fb_info *fb_info);
 extern int remove_conflicting_pci_framebuffers(struct pci_dev *pdev,

-- 
2.33.1

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat