fbcon: add lifetime refcount to opened frame buffers

This just adds the refcount and the new registration lock logic.  It
does not (for example) actually change the read/write/ioctl routines to
actually use the frame buffer that was opened: those function still end
up alway susing whatever the current frame buffer is at the time of the
call.

Without this, if something holds the frame buffer open over a
framebuffer switch, the close() operation after the switch will access a
fb_info that has been free'd by the unregistering of the old frame
buffer.

(The read/write/ioctl operations will normally not cause problems,
because they will - illogically - pick up the new fbcon instead.  But a
switch that happens just as one of those is going on might see problems
too, the window is just much smaller: one individual op rather than the
whole open-close sequence.)

This use-after-free is apparently fairly easily triggered by the Ubuntu
11.04 boot sequence.

Acked-by: Tim Gardner <tim.gardner@canonical.com>
Tested-by: Daniel J Blueman <daniel.blueman@gmail.com>
Tested-by: Anca Emanuel <anca.emanuel@gmail.com>
Cc: Bruno Prémont <bonbons@linux-vserver.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Andy Whitcroft <andy.whitcroft@canonical.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index e0c2284924b63b4a390aa8a4eb862fcfddac2609..eec14d2ca1c7671c5bc45d8d0b19a980c653860b 100644 (file)
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -42,9 +42,34 @@
 
 #define FBPIXMAPSIZE   (1024 * 8)
 
+static DEFINE_MUTEX(registration_lock);
 struct fb_info *registered_fb[FB_MAX] __read_mostly;
 int num_registered_fb __read_mostly;
 
+static struct fb_info *get_fb_info(unsigned int idx)
+{
+       struct fb_info *fb_info;
+
+       if (idx >= FB_MAX)
+               return ERR_PTR(-ENODEV);
+
+       mutex_lock(&registration_lock);
+       fb_info = registered_fb[idx];
+       if (fb_info)
+               atomic_inc(&fb_info->count);
+       mutex_unlock(&registration_lock);
+
+       return fb_info;
+}
+
+static void put_fb_info(struct fb_info *fb_info)
+{
+       if (!atomic_dec_and_test(&fb_info->count))
+               return;
+       if (fb_info->fbops->fb_destroy)
+               fb_info->fbops->fb_destroy(fb_info);
+}
+
 int lock_fb_info(struct fb_info *info)
 {
        mutex_lock(&info->lock);
@@ -647,6 +672,7 @@ int fb_show_logo(struct fb_info *info, int rotate) { return 0; }
 
 static void *fb_seq_start(struct seq_file *m, loff_t *pos)
 {
+       mutex_lock(&registration_lock);
        return (*pos < FB_MAX) ? pos : NULL;
 }
 
@@ -658,6 +684,7 @@ static void *fb_seq_next(struct seq_file *m, void *v, loff_t *pos)
 
 static void fb_seq_stop(struct seq_file *m, void *v)
 {
+       mutex_unlock(&registration_lock);
 }
 
 static int fb_seq_show(struct seq_file *m, void *v)
@@ -1361,14 +1388,16 @@ __releases(&info->lock)
        struct fb_info *info;
        int res = 0;
 
-       if (fbidx >= FB_MAX)
-               return -ENODEV;
-       info = registered_fb[fbidx];
-       if (!info)
+       info = get_fb_info(fbidx);
+       if (!info) {
                request_module("fb%d", fbidx);
-       info = registered_fb[fbidx];
-       if (!info)
-               return -ENODEV;
+               info = get_fb_info(fbidx);
+               if (!info)
+                       return -ENODEV;
+       }
+       if (IS_ERR(info))
+               return PTR_ERR(info);
+
        mutex_lock(&info->lock);
        if (!try_module_get(info->fbops->owner)) {
                res = -ENODEV;
@@ -1386,6 +1415,8 @@ __releases(&info->lock)
 #endif
 out:
        mutex_unlock(&info->lock);
+       if (res)
+               put_fb_info(info);
        return res;
 }
 
@@ -1401,6 +1432,7 @@ __releases(&info->lock)
                info->fbops->fb_release(info,1);
        module_put(info->fbops->owner);
        mutex_unlock(&info->lock);
+       put_fb_info(info);
        return 0;
 }
 
@@ -1542,11 +1574,13 @@ register_framebuffer(struct fb_info *fb_info)
        remove_conflicting_framebuffers(fb_info->apertures, fb_info->fix.id,
                                         fb_is_primary_device(fb_info));
 
+       mutex_lock(&registration_lock);
        num_registered_fb++;
        for (i = 0 ; i < FB_MAX; i++)
                if (!registered_fb[i])
                        break;
        fb_info->node = i;
+       atomic_set(&fb_info->count, 1);
        mutex_init(&fb_info->lock);
        mutex_init(&fb_info->mm_lock);
 
@@ -1583,6 +1617,7 @@ register_framebuffer(struct fb_info *fb_info)
        fb_var_to_videomode(&mode, &fb_info->var);
        fb_add_videomode(&mode, &fb_info->modelist);
        registered_fb[i] = fb_info;
+       mutex_unlock(&registration_lock);
 
        event.info = fb_info;
        if (!lock_fb_info(fb_info))
@@ -1616,6 +1651,7 @@ unregister_framebuffer(struct fb_info *fb_info)
        struct fb_event event;
        int i, ret = 0;
 
+       mutex_lock(&registration_lock);
        i = fb_info->node;
        if (!registered_fb[i]) {
                ret = -EINVAL;
@@ -1638,7 +1674,7 @@ unregister_framebuffer(struct fb_info *fb_info)
            (fb_info->pixmap.flags & FB_PIXMAP_DEFAULT))
                kfree(fb_info->pixmap.addr);
        fb_destroy_modelist(&fb_info->modelist);
-       registered_fb[i]=NULL;
+       registered_fb[i] = NULL;
        num_registered_fb--;
        fb_cleanup_device(fb_info);
        device_destroy(fb_class, MKDEV(FB_MAJOR, i));
@@ -1646,9 +1682,9 @@ unregister_framebuffer(struct fb_info *fb_info)
        fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
 
        /* this may free fb info */
-       if (fb_info->fbops->fb_destroy)
-               fb_info->fbops->fb_destroy(fb_info);
+       put_fb_info(fb_info);
 done:
+       mutex_unlock(&registration_lock);
        return ret;
 }
 

diff --git a/include/linux/fb.h b/include/linux/fb.h
index df728c1c29ed39c0b25a8ead2fcc0f89696c911f..6a8274877171aa7959cb95e5370ccc49a626d665 100644 (file)
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -832,6 +832,7 @@ struct fb_tile_ops {
 #define FBINFO_CAN_FORCE_OUTPUT     0x200000
 
 struct fb_info {
+       atomic_t count;
        int node;
        int flags;
        struct mutex lock;              /* Lock for open/release/ioctl funcs */