Re: [PATCH 30/30] fbdev: Make support for userspace interfaces configurable

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 11.06.23 um 18:37 schrieb Sam Ravnborg:
Hi Thomas,

On Mon, Jun 05, 2023 at 04:48:12PM +0200, Thomas Zimmermann wrote:
Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev
device optional. If the new option has not been selected, fbdev
does not create a files in devfs or sysfs.
s/ a//

Most modern Linux systems run a DRM-based graphics stack that uses
the kernel's framebuffer console, but has otherwise deprecated fbdev
support. Yet fbdev userspace interfaces are still present.

The option makes it possible to use the fbdev subsystem as console
implementation without support for userspace. This closes potential
entry points to manipulate kernel or I/O memory via framebuffers. It
also prevents the execution of driver code via ioctl or sysfs, both
of which might allow malicious software to exploit bugs in the fbdev
code.

A small number of fbdev drivers require struct fbinfo.dev to be
initialized, usually for the support of sysfs interface. Make these
drivers depend on FB_DEVICE. They can later be fixed if necessary.
Should that be a TODO in gpu/todo.rst?
Otherwise the amount of people knowing about this
is very close to 1.
As an alternative add a TODO to each Kconfig file.


Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
---
  drivers/staging/fbtft/Kconfig            |  1 +
  drivers/video/fbdev/Kconfig              | 12 +++++++++
  drivers/video/fbdev/core/Makefile        |  7 +++---
  drivers/video/fbdev/core/fb_internal.h   | 32 ++++++++++++++++++++++++
  drivers/video/fbdev/omap2/omapfb/Kconfig |  2 +-
  include/linux/fb.h                       |  2 ++
  6 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index 4d29e8c1014e..5dda3c65a38e 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -2,6 +2,7 @@
  menuconfig FB_TFT
  	tristate "Support for small TFT LCD display modules"
  	depends on FB && SPI
+	depends on FB_DEVICE
  	depends on GPIOLIB || COMPILE_TEST
  	select FB_SYS_FILLRECT
  	select FB_SYS_COPYAREA
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 6df9bd09454a..48d9a14f889c 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -57,6 +57,15 @@ config FIRMWARE_EDID
  	  combination with certain motherboards and monitors are known to
  	  suffer from this problem.
+config FB_DEVICE
+        bool "Provide legacy /dev/fb* device"
+        depends on FB
+        help
+	  Say Y here if you want the legacy /dev/fb* device file. It's
+	  only required if you have userspace programs that depend on
+	  fbdev for graphics output. This does not effect the framebuffer
+	  console.
tabs to spaces to indent the above correct.

+
  config FB_DDC
  	tristate
  	depends on FB
@@ -1545,6 +1554,7 @@ config FB_3DFX_I2C
  config FB_VOODOO1
  	tristate "3Dfx Voodoo Graphics (sst1) support"
  	depends on FB && PCI
+	depends on FB_DEVICE
  	select FB_CFB_FILLRECT
  	select FB_CFB_COPYAREA
  	select FB_CFB_IMAGEBLIT
@@ -1862,6 +1872,7 @@ config FB_SH_MOBILE_LCDC
  	tristate "SuperH Mobile LCDC framebuffer support"
  	depends on FB && HAVE_CLK && HAS_IOMEM
  	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
+	depends on FB_DEVICE
  	select FB_SYS_FILLRECT
  	select FB_SYS_COPYAREA
  	select FB_SYS_IMAGEBLIT
@@ -1930,6 +1941,7 @@ config FB_SMSCUFX
  config FB_UDL
  	tristate "Displaylink USB Framebuffer support"
  	depends on FB && USB
+	depends on FB_DEVICE
  	select FB_MODE_HELPERS
  	select FB_SYS_FILLRECT
  	select FB_SYS_COPYAREA
diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
index 125d24f50c36..d5e8772620f8 100644
--- a/drivers/video/fbdev/core/Makefile
+++ b/drivers/video/fbdev/core/Makefile
@@ -2,12 +2,13 @@
  obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
  obj-$(CONFIG_FB)                  += fb.o
  fb-y                              := fb_backlight.o \
-                                     fb_devfs.o \
                                       fb_info.o \
-                                     fb_procfs.o \
-                                     fbmem.o fbmon.o fbcmap.o fbsysfs.o \
+                                     fbmem.o fbmon.o fbcmap.o \
                                       modedb.o fbcvt.o fb_cmdline.o fb_io_fops.o
  fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
+fb-$(CONFIG_FB_DEVICE)            += fb_devfs.o \
+                                     fb_procfs.o \
+                                     fbsysfs.o
Maybe change this to one line to avoid '\'?

ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
  fb-y				  += fbcon.o bitblit.o softcursor.o
diff --git a/drivers/video/fbdev/core/fb_internal.h b/drivers/video/fbdev/core/fb_internal.h
index 0b43c0cd5096..b8a28466db79 100644
--- a/drivers/video/fbdev/core/fb_internal.h
+++ b/drivers/video/fbdev/core/fb_internal.h
@@ -3,12 +3,22 @@
  #ifndef _FB_INTERNAL_H
  #define _FB_INTERNAL_H
+#include <linux/device.h>
  #include <linux/fb.h>
  #include <linux/mutex.h>
/* fb_devfs.c */
+#if defined(CONFIG_FB_DEVICE)
  int fb_register_chrdev(void);
  void fb_unregister_chrdev(void);
+#else
+static inline int fb_register_chrdev(void)
+{
+	return 0;
+}
+static inline void fb_unregister_chrdev(void)
+{ }
+#endif
/* fbmem.c */
  extern struct class *fb_class;
@@ -19,11 +29,33 @@ struct fb_info *get_fb_info(unsigned int idx);
  void put_fb_info(struct fb_info *fb_info);
/* fb_procfs.c */
+#if defined(CONFIG_FB_DEVICE)
  int fb_init_procfs(void);
  void fb_cleanup_procfs(void);
+#else
+static inline int fb_init_procfs(void)
+{
+	return 0;
+}
+static inline void fb_cleanup_procfs(void)
+{ }
+#endif
/* fbsysfs.c */
+#if defined(CONFIG_FB_DEVICE)
  int fb_device_create(struct fb_info *fb_info);
  void fb_device_destroy(struct fb_info *fb_info);
+#else
+static inline int fb_device_create(struct fb_info *fb_info)
+{
+	get_device(fb_info->device); // as in device_add()
+
+	return 0;
+}
+static inline void fb_device_destroy(struct fb_info *fb_info)
+{
+	put_device(fb_info->device); // as in device_del()
+}
+#endif
I do not see why fb_device_{create,destroy} needs to call
{get,put}_device - and it is not explained.
A short explanation in the commit maybe?

Ok, I'll do that.

This get_device() is normally done within device_create() from within register_framebuffer(). The put is within unregister_frambuffer(). Without, someone could attempt to unplug the Linux hardware device and our pointer would go stale. So we have to hold the reference within fbdev.

Best regards
Thomas


With my comments addressed:
Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>

Note: I do not engage in the thread about the best Kconfig
solution - I trust the involved people will find a good solution.

	Sam

#endif
diff --git a/drivers/video/fbdev/omap2/omapfb/Kconfig b/drivers/video/fbdev/omap2/omapfb/Kconfig
index 69f9cb03507e..21069fdb7cc2 100644
--- a/drivers/video/fbdev/omap2/omapfb/Kconfig
+++ b/drivers/video/fbdev/omap2/omapfb/Kconfig
@@ -5,9 +5,9 @@ config OMAP2_VRFB
  menuconfig FB_OMAP2
  	tristate "OMAP2+ frame buffer support"
  	depends on FB
+	depends on FB_DEVICE
  	depends on DRM_OMAP = n
  	depends on GPIOLIB
-
  	select FB_OMAP2_DSS
  	select OMAP2_VRFB if ARCH_OMAP2 || ARCH_OMAP3
  	select FB_CFB_FILLRECT
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 541a0e3ce21f..40ed1028160c 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -481,7 +481,9 @@ struct fb_info {
const struct fb_ops *fbops;
  	struct device *device;		/* This is the parent */
+#if defined(CONFIG_FB_DEVICE)
  	struct device *dev;		/* This is this fb device */
+#endif
  	int class_flag;                    /* private sysfs flags */
  #ifdef CONFIG_FB_TILEBLITTING
  	struct fb_tile_ops *tileops;    /* Tile Blitting */
--
2.40.1

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux