[PATCH] drm/fb_helper: Allow leaking fbdev smem_start

Fri Sep 21 14:07:40 UTC 2018

Hi,

On Thu, Sep 20, 2018 at 03:04:12PM +0200, Neil Armstrong wrote:
> Since "drm/fb: Stop leaking physical address", the default behaviour of
> the DRM fbdev emulation is to set the smem_base to 0 and pass the new
> FBINFO_HIDE_SMEM_START flag.
> 
> The main reason is to avoid leaking physical addresse to user-space, and
> it follows a general move over the kernel code to avoid user-space to
> manipulate physical addresses and then use some other mechanisms like
> dma-buf to transfer physical buffer handles over multiple subsystems.
> 
> But, a lot of devices depends on closed sources binaries to enable
> OpenGL hardware acceleration that uses this smem_start value to
> pass physical addresses to out-of-tree modules in order to render
> into these physical adresses. These should use dma-buf buffers allocated
> from the DRM display device instead and stop relying on fbdev overallocation
> to gather DMA memory (some HW vendors delivers GBM and Wayland capable
> binaries, but older unsupported devices won't have these new binaries
> and are doomed until an Open Source solution like Lima finalizes).
> 
> Since these devices heavily depends on this kind of software and because
> the smem_start population was available for years, it's a breakage to
> stop leaking smem_start without any alternative solutions.
> 
> This patch adds a Kconfig depending on the EXPERT config and an unsafe
> kernel module parameter tainting the kernel when enabled.
> 
> A clear comment and Kconfig help text was added to clarify why and when
> this patch should be reverted, but in the meantime it's a necessary
> feature to keep.
> 
> Cc: Dave Airlie <airlied at gmail.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com>
> Cc: Noralf Trønnes <noralf at tronnes.org>
> Cc: Maxime Ripard <maxime.ripard at bootlin.com>
> Cc: Eric Anholt <eric at anholt.net>
> Cc: Lucas Stach <l.stach at pengutronix.de>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Ben Skeggs <skeggsb at gmail.com>
> Cc: Christian König <christian.koenig at amd.com>
> Signed-off-by: Neil Armstrong <narmstrong at baylibre.com>
> ---
>  drivers/gpu/drm/Kconfig         | 15 +++++++++++++++
>  drivers/gpu/drm/drm_fb_helper.c | 33 +++++++++++++++++++++++++++++++--
>  2 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 8871b3f..b07c298 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -110,6 +110,21 @@ config DRM_FBDEV_OVERALLOC
>  	  is 100. Typical values for double buffering will be 200,
>  	  triple buffering 300.
>  
> +config DRM_FBDEV_LEAK_PHYS_SMEM
> +	bool "Shamelessly allow leaking of fbdev physical address (DANGEROUS)"
> +	depends on DRM_FBDEV_EMULATION && EXPERT
> +	default n
> +	help
> +	  In order to keep user-space compatibility, we want in certain
> +	  use-cases to keep leaking the fbdev physical address to the
> +	  user-space program handling the fbdev buffer.
> +	  This option is a shame and should be removed as soon as possible
> +	  and be considered as a broken and legacy behaviour from a modern
> +	  fbdev device driver.
> +
> +	  If in doubt, say "N" or spread the word to your closed source
> +	  library vendor.
> +
>  config DRM_LOAD_EDID_FIRMWARE
>  	bool "Allow to specify an EDID data set instead of probing for it"
>  	depends on DRM
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index bf2190c..a354944 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -56,6 +56,25 @@ MODULE_PARM_DESC(drm_fbdev_overalloc,
>  		 "Overallocation of the fbdev buffer (%) [default="
>  		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
>  
> +/*
> + * In order to keep user-space compatibility, we want in certain use-cases
> + * to keep leaking the fbdev physical address to the user-space program
> + * handling the fbdev buffer.
> + * This is a bad habit essentially kept into closed source opengl driver
> + * that should really be moved into open-source upstream projects instead
> + * of using legacy physical addresses in user space to communicate with
> + * other out-of-tree kernel modules.
> + *
> + * This module_param *should* be removed as soon as possible and be
> + * considered as a broken and legacy behaviour from a modern fbdev device.
> + */
> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> +static bool drm_leak_fbdev_smem = false;
> +module_param_unsafe(drm_leak_fbdev_smem, bool, 0600);
> +MODULE_PARM_DESC(fbdev_emulation,
> +		 "Allow unsafe leaking fbdev physical smem address [default=false]");
> +#endif
> +
>  static LIST_HEAD(kernel_fb_helper_list);
>  static DEFINE_MUTEX(kernel_fb_helper_lock);
>  
> @@ -2673,8 +2692,12 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper,
>  
>  	info = fb_helper->fbdev;
>  	info->var.pixclock = 0;
> -	/* don't leak any physical addresses to userspace */
> -	info->flags |= FBINFO_HIDE_SMEM_START;
> +	/* Shamelessly allow physical address leaking to userspace */
> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> +	if (!drm_leak_fbdev_smem)
> +#endif

You can use the IS_ENABLED directy within the if statement. It would
make the code a bit nicer.

> +		/* don't leak any physical addresses to userspace */
> +		info->flags |= FBINFO_HIDE_SMEM_START;
>  
>  	/* Need to drop locks to avoid recursive deadlock in
>  	 * register_framebuffer. This is ok because the only thing left to do is
> @@ -3083,6 +3106,12 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
>  	fbi->fbops = &drm_fbdev_fb_ops;
>  	fbi->screen_size = fb->height * fb->pitches[0];
>  	fbi->fix.smem_len = fbi->screen_size;
> +	/* Shamelessly leak the physical address to user-space */
> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> +	if (drm_leak_fbdev_smem)
> +		fbi->fix.smem_start =
> +			page_to_phys(virt_to_page(fbi->screen_buffer));
> +#endif

fbi->screen_buffer is only set..

>  	fbi->screen_buffer = buffer->vaddr;

.. at the next line. Moving that line before the if statement works.

With that fixed, and the IS_ENABLED in the if

Reviewed-by: Maxime Ripard <maxime.ripard at bootlin.com>
Tested-by: Maxime Ripard <maxime.ripard at bootlin.com>

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180921/cfd199f7/attachment.sig>