| Message ID | 1396691102-22904-9-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Sat, Apr 5, 2014 at 11:44 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > The context_dtor callback is only called once we've successfully loaded > the driver, which means dev->dev_private is set up. The check is hence > pointless. > > Also dev->dev_private is deref already above, so compilers are free > to elide it anyway. Are you sure compilers can assume "*ptr" implies "ptr != NULL"? I doubt that and depending on CONFIG_DEFAULT_MMAP_MIN_ADDR I think you can even build user-space that can successfully mmap(MAP_FIXED) at address 0. Anyhow, I guess no-one cares besides me, so patch looks good :) Thanks David
On Mon, Apr 7, 2014 at 5:51 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > On Sat, Apr 5, 2014 at 11:44 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> The context_dtor callback is only called once we've successfully loaded >> the driver, which means dev->dev_private is set up. The check is hence >> pointless. >> >> Also dev->dev_private is deref already above, so compilers are free >> to elide it anyway. > > Are you sure compilers can assume "*ptr" implies "ptr != NULL"? I > doubt that and depending on CONFIG_DEFAULT_MMAP_MIN_ADDR I think you > can even build user-space that can successfully mmap(MAP_FIXED) at > address 0. Anyhow, I guess no-one cares besides me, so patch looks > good :) Yeah, my understand has been that every time you deref a pointer somewhere the compiler is allowed to presume that the pointer isn't NULL. Which makes mmap(MAP_FIXED) at address NULL such a dangerous thing and iirc there's been patches floating around to severely restrict that to make exploiting such bugs much harder. Iirc it's only emulators like dosemu who really need to be able to map something at NULL. Since if gcc drops the NULL check the last line of defense (namely Oopsing on the NULL deref) can be disabled by userspace. The usual exploit is to put a real data structure at NULL and use that (thorugh vtables if possible) to take over the kernel. I'm not always entirely sure on what the precise rules are really in detail, but since coverity screamed at me about this here I've figured coverity is probably right ;-) -Daniel
diff --git a/drivers/gpu/drm/via/via_mm.c b/drivers/gpu/drm/via/via_mm.c index 927889105483..d70b1e1544bf 100644 --- a/drivers/gpu/drm/via/via_mm.c +++ b/drivers/gpu/drm/via/via_mm.c @@ -79,7 +79,7 @@ int via_final_context(struct drm_device *dev, int context) /* Linux specific until context tracking code gets ported to BSD */ /* Last context, perform cleanup */ - if (list_is_singular(&dev->ctxlist) && dev->dev_private) { + if (list_is_singular(&dev->ctxlist)) { DRM_DEBUG("Last Context\n"); drm_irq_uninstall(dev); via_cleanup_futex(dev_priv);
The context_dtor callback is only called once we've successfully loaded the driver, which means dev->dev_private is set up. The check is hence pointless. Also dev->dev_private is deref already above, so compilers are free to elide it anyway. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/via/via_mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)