| Message ID | 20190515132925.48867-1-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/fb-helper: Fix drm_fb_helper_hotplug_event() NULL ptr argument | expand |
On Wed, May 15, 2019 at 03:29:25PM +0200, Noralf Trønnes wrote: > drm_fb_helper_hotplug_event() should tolerate the fb_helper argument being > NULL. Commit 03a9606e7fee ("drm/fb-helper: Avoid race with DRM userspace") > introduced a fb_helper dereference before the NULL check. > Fixup by moving the dereference after the NULL check. > > Fixes: 03a9606e7fee ("drm/fb-helper: Avoid race with DRM userspace") > Reported-by: kbuild test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> Ah the classic "I spotted a deref before your NULL check, I'm going to optimize this all away because you got it wrong" nonsense from gcc. I thought the kernel uses a special compile flag to avoid this optimization ... Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_fb_helper.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index c259a28522f8..a52b48fafbd7 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -3031,7 +3031,6 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); > */ > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > { > - struct drm_device *dev = fb_helper->dev; > int err = 0; > > if (!drm_fbdev_emulation || !fb_helper) > @@ -3044,13 +3043,13 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > return err; > } > > - if (!fb_helper->fb || !drm_master_internal_acquire(dev)) { > + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { > fb_helper->delayed_hotplug = true; > mutex_unlock(&fb_helper->lock); > return err; > } > > - drm_master_internal_release(dev); > + drm_master_internal_release(fb_helper->dev); > > DRM_DEBUG_KMS("\n"); > > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, May 15, 2019 at 03:40:14PM +0200, Daniel Vetter wrote: > On Wed, May 15, 2019 at 03:29:25PM +0200, Noralf Trønnes wrote: > > drm_fb_helper_hotplug_event() should tolerate the fb_helper argument being > > NULL. Commit 03a9606e7fee ("drm/fb-helper: Avoid race with DRM userspace") > > introduced a fb_helper dereference before the NULL check. > > Fixup by moving the dereference after the NULL check. > > > > Fixes: 03a9606e7fee ("drm/fb-helper: Avoid race with DRM userspace") > > Reported-by: kbuild test robot <lkp@intel.com> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > Ah the classic "I spotted a deref before your NULL check, I'm going to > optimize this all away because you got it wrong" nonsense from gcc. I > thought the kernel uses a special compile flag to avoid this optimization > ... This is just a normal NULL dereference bug. You're thinking of the old tun.c vulnerability. That was back in the day before we started using -fno-delete-null-pointer-checks. What happened there was the code should have NULL dereferenced and Oopsed but GCC optimized it away and it ended up being a privilege escalation bug instead. regards, dan carpenter
On Wed, May 15, 2019 at 04:55:16PM +0300, Dan Carpenter wrote: > On Wed, May 15, 2019 at 03:40:14PM +0200, Daniel Vetter wrote: > > On Wed, May 15, 2019 at 03:29:25PM +0200, Noralf Trønnes wrote: > > > drm_fb_helper_hotplug_event() should tolerate the fb_helper argument being > > > NULL. Commit 03a9606e7fee ("drm/fb-helper: Avoid race with DRM userspace") > > > introduced a fb_helper dereference before the NULL check. > > > Fixup by moving the dereference after the NULL check. > > > > > > Fixes: 03a9606e7fee ("drm/fb-helper: Avoid race with DRM userspace") > > > Reported-by: kbuild test robot <lkp@intel.com> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > > > Ah the classic "I spotted a deref before your NULL check, I'm going to > > optimize this all away because you got it wrong" nonsense from gcc. I > > thought the kernel uses a special compile flag to avoid this optimization > > ... > > This is just a normal NULL dereference bug. > > You're thinking of the old tun.c vulnerability. That was back in the > day before we started using -fno-delete-null-pointer-checks. What > happened there was the code should have NULL dereferenced and Oopsed but > GCC optimized it away and it ended up being a privilege escalation bug > instead. Hm right, I got confused. -Daniel
Den 15.05.2019 15.40, skrev Daniel Vetter: > On Wed, May 15, 2019 at 03:29:25PM +0200, Noralf Trønnes wrote: >> drm_fb_helper_hotplug_event() should tolerate the fb_helper argument being >> NULL. Commit 03a9606e7fee ("drm/fb-helper: Avoid race with DRM userspace") >> introduced a fb_helper dereference before the NULL check. >> Fixup by moving the dereference after the NULL check. >> >> Fixes: 03a9606e7fee ("drm/fb-helper: Avoid race with DRM userspace") >> Reported-by: kbuild test robot <lkp@intel.com> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Thanks Dan and Daniel, applied to drm-misc-next. Noralf.
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index c259a28522f8..a52b48fafbd7 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -3031,7 +3031,6 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); */ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) { - struct drm_device *dev = fb_helper->dev; int err = 0; if (!drm_fbdev_emulation || !fb_helper) @@ -3044,13 +3043,13 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) return err; } - if (!fb_helper->fb || !drm_master_internal_acquire(dev)) { + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { fb_helper->delayed_hotplug = true; mutex_unlock(&fb_helper->lock); return err; } - drm_master_internal_release(dev); + drm_master_internal_release(fb_helper->dev); DRM_DEBUG_KMS("\n");
drm_fb_helper_hotplug_event() should tolerate the fb_helper argument being NULL. Commit 03a9606e7fee ("drm/fb-helper: Avoid race with DRM userspace") introduced a fb_helper dereference before the NULL check. Fixup by moving the dereference after the NULL check. Fixes: 03a9606e7fee ("drm/fb-helper: Avoid race with DRM userspace") Reported-by: kbuild test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/drm_fb_helper.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)