drm/i915/fbdev: Hide smem_start from userspace
diff mbox series

Message ID 20191113171944.19308-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/fbdev: Hide smem_start from userspace
Related show

Commit Message

Chris Wilson Nov. 13, 2019, 5:19 p.m. UTC
Do not leak our internal kernel address for random userspace to abuse.
Daniel added the support to fbdev to filter out the physical addresses
being exposed by fbdev, put that to use to protect ourselves.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112256
Fixes: 5f889b9a61dd ("drm/i915: Disregard drm_mode_config.fb_base")
References: da6c7707caf3 ("fbdev: Add FBINFO_HIDE_SMEM_START flag")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Ville Syrjälä Nov. 14, 2019, 10:53 a.m. UTC | #1
On Wed, Nov 13, 2019 at 05:19:44PM +0000, Chris Wilson wrote:
> Do not leak our internal kernel address for random userspace to abuse.
> Daniel added the support to fbdev to filter out the physical addresses
> being exposed by fbdev, put that to use to protect ourselves.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112256
> Fixes: 5f889b9a61dd ("drm/i915: Disregard drm_mode_config.fb_base")
> References: da6c7707caf3 ("fbdev: Add FBINFO_HIDE_SMEM_START flag")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 3d1061470e76..bff311561597 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -226,8 +226,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  		goto out_unpin;
>  	}
>  
> -	ifbdev->helper.fb = &ifbdev->fb->base;
> -
> +	/* don't leak any physical addresses to userspace */
> +	info->flags |= FBINFO_HIDE_SMEM_START;

Doesn't the fb helper already do this?

Looks like it tries. Though I have no idea why it does
it in initial_config() instead of fill_fb_info().

>  	info->fbops = &intelfb_ops;
>  
>  	/* setup aperture base/size for vesafb takeover */
> @@ -247,6 +247,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	info->fix.smem_start = (unsigned long)info->screen_base;

Isn't screen_base the virtual address? Why do we put that into
smem_start?

>  	info->fix.smem_len = info->screen_size;
>  
> +	ifbdev->helper.fb = &ifbdev->fb->base;
>  	drm_fb_helper_fill_info(info, &ifbdev->helper, sizes);
>  
>  	/* If the object is shmemfs backed, it will have given us zeroed pages.
> -- 
> 2.24.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Nov. 14, 2019, 10:57 a.m. UTC | #2
Quoting Ville Syrjälä (2019-11-14 10:53:11)
> On Wed, Nov 13, 2019 at 05:19:44PM +0000, Chris Wilson wrote:
> > Do not leak our internal kernel address for random userspace to abuse.
> > Daniel added the support to fbdev to filter out the physical addresses
> > being exposed by fbdev, put that to use to protect ourselves.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112256
> > Fixes: 5f889b9a61dd ("drm/i915: Disregard drm_mode_config.fb_base")
> > References: da6c7707caf3 ("fbdev: Add FBINFO_HIDE_SMEM_START flag")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fbdev.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > index 3d1061470e76..bff311561597 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > @@ -226,8 +226,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >               goto out_unpin;
> >       }
> >  
> > -     ifbdev->helper.fb = &ifbdev->fb->base;
> > -
> > +     /* don't leak any physical addresses to userspace */
> > +     info->flags |= FBINFO_HIDE_SMEM_START;
> 
> Doesn't the fb helper already do this?
> 
> Looks like it tries. Though I have no idea why it does
> it in initial_config() instead of fill_fb_info().

I wasn't even sure we took that path.

> >       info->fbops = &intelfb_ops;
> >  
> >       /* setup aperture base/size for vesafb takeover */
> > @@ -247,6 +247,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >       info->fix.smem_start = (unsigned long)info->screen_base;
> 
> Isn't screen_base the virtual address? Why do we put that into
> smem_start?

I made the mistake of thinking it was only internally used. So I take
that as an implicit r-b for the fix and the igt proving that I was
dumb...
https://patchwork.freedesktop.org/series/69416/
https://patchwork.freedesktop.org/series/69421/
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 3d1061470e76..bff311561597 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -226,8 +226,8 @@  static int intelfb_create(struct drm_fb_helper *helper,
 		goto out_unpin;
 	}
 
-	ifbdev->helper.fb = &ifbdev->fb->base;
-
+	/* don't leak any physical addresses to userspace */
+	info->flags |= FBINFO_HIDE_SMEM_START;
 	info->fbops = &intelfb_ops;
 
 	/* setup aperture base/size for vesafb takeover */
@@ -247,6 +247,7 @@  static int intelfb_create(struct drm_fb_helper *helper,
 	info->fix.smem_start = (unsigned long)info->screen_base;
 	info->fix.smem_len = info->screen_size;
 
+	ifbdev->helper.fb = &ifbdev->fb->base;
 	drm_fb_helper_fill_info(info, &ifbdev->helper, sizes);
 
 	/* If the object is shmemfs backed, it will have given us zeroed pages.