lkml.org 
[lkml]   [2011]   [Jan]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Subject[PATCH 1/2] fb: avoid oops when fb is removed by remove_conflicting_framebuffers
Date
Firmware framebuffers (which have the flag FBINFO_MISC_FIRMWARE) can be
removed and replaced by another native framebuffer, like what happens
when you boot with a vesa framebuffer and later loads a drm module with
modesetting enabled on x86.

When framebuffer which is going to replace the firmware framebuffer is
registered, unregister_framebuffer is called for the old firmware
framebuffer automatically inside remove_conflicting_framebuffers
function. The problem is that while this happens, the old framebuffer
can still be in use.

The first issue in this is that unregister_framebuffer can free/destroy
struct fb_info when calling fb_info->fbops->fb_destroy. The same struct
fb_info is given to file->private_data inside fb_open function, so if
there is an application that have old framebuffer open, and it closes it
after the framebuffer was replaced, fb_release will still get the old
struct fb_info from file->private_data. As it was freed, the kernel will
most likely oops accessing an already freed location inside fb_release.
To fix this, add a reference count to struct fb_info and use it, so the
struct will be only destroyed after the last user closes the framebuffer.

The second issue is that the file_operations framebuffer functions
reference registered_fb array without locking. This can cause a race and
oops if application using the old firmware framebuffer calls a
read/write/mmap/ioctl function while unregister_framebuffer is running,
in the window inside it where registered_fb for the old framebuffer is
set to NULL. To avoid this situation, do not get the framebuffer from
registered_fb array, instead get it from file->private_data which is set
on fb_open. While at it, also add a new fb_info state which is set when
the framebuffer is being unregistered, so that file_operations functions
can check and prevent access to framebuffer when possible, returning an
error if the framebuffer is being or already unregistered.

This addresses kernel.org bug #26232. A testcase is provided in the
ticket which triggers the problem on current kernels.

References: https://bugzilla.kernel.org/show_bug.cgi?id=26232
Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
---
drivers/video/fbmem.c | 170 ++++++++++++++++++++++++++++++++++-------------
drivers/video/fbsysfs.c | 2 +
include/linux/fb.h | 2 +
3 files changed, 127 insertions(+), 47 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 4ac1201..05d09ac 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -48,7 +48,7 @@ int num_registered_fb __read_mostly;
int lock_fb_info(struct fb_info *info)
{
mutex_lock(&info->lock);
- if (!info->fbops) {
+ if (unlikely(!info->fbops || info->state == FBINFO_STATE_EXITING)) {
mutex_unlock(&info->lock);
return 0;
}
@@ -56,6 +56,72 @@ int lock_fb_info(struct fb_info *info)
}
EXPORT_SYMBOL(lock_fb_info);

+static int fbops_lock_fb_info(struct fb_info *info)
+{
+ int err;
+
+ if (!info->screen_base)
+ return -ENODEV;
+
+ if (info->flags == FBINFO_MISC_FIRMWARE) {
+ err = lock_fb_info(info);
+ if (err) {
+ if (info->state == FBINFO_STATE_SUSPENDED)
+ err = -EPERM;
+ if (err < 1)
+ unlock_fb_info(info);
+ else
+ err = 0;
+ } else
+ err = -ENODEV;
+ return err;
+ }
+
+ if (info->state != FBINFO_STATE_RUNNING)
+ return -EPERM;
+
+ return 0;
+}
+
+static void fbops_unlock_fb_info(struct fb_info *info)
+{
+ if (info->flags == FBINFO_MISC_FIRMWARE)
+ unlock_fb_info(info);
+}
+
+static void fb_kref_init(struct fb_info *info)
+{
+ if (info->flags == FBINFO_MISC_FIRMWARE)
+ kref_init(&info->fwfb_rfcnt);
+}
+
+static void fb_kref_get(struct fb_info *info)
+{
+ if (info->flags == FBINFO_MISC_FIRMWARE)
+ kref_get(&info->fwfb_rfcnt);
+}
+
+static void fb_kref_release(struct kref *ref)
+{
+ struct fb_info *info = container_of(ref, struct fb_info, fwfb_rfcnt);
+
+ if (info->fbops->fb_destroy)
+ info->fbops->fb_destroy(info);
+}
+
+static int fb_kref_put(struct fb_info *info, bool in_unregister)
+{
+ if (info->flags == FBINFO_MISC_FIRMWARE)
+ return kref_put(&info->fwfb_rfcnt, fb_kref_release);
+
+ if (in_unregister && info->fbops->fb_destroy) {
+ info->fbops->fb_destroy(info);
+ return 1;
+ }
+
+ return 0;
+}
+
/*
* Helpers
*/
@@ -694,30 +760,30 @@ static ssize_t
fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct inode *inode = file->f_path.dentry->d_inode;
- int fbidx = iminor(inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info *info = file->private_data;
u8 *buffer, *dst;
u8 __iomem *src;
- int c, cnt = 0, err = 0;
+ int c, cnt = 0, err;
unsigned long total_size;

- if (!info || ! info->screen_base)
- return -ENODEV;
+ err = fbops_lock_fb_info(info);
+ if (err)
+ return err;

- if (info->state != FBINFO_STATE_RUNNING)
- return -EPERM;
+ if (info->fbops->fb_read) {
+ err = info->fbops->fb_read(info, buf, count, ppos);
+ goto fb_read_exit;
+ }

- if (info->fbops->fb_read)
- return info->fbops->fb_read(info, buf, count, ppos);
-
total_size = info->screen_size;

if (total_size == 0)
total_size = info->fix.smem_len;

- if (p >= total_size)
- return 0;
+ if (p >= total_size) {
+ err = 0;
+ goto fb_read_exit;
+ }

if (count >= total_size)
count = total_size;
@@ -727,8 +793,10 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)

buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
GFP_KERNEL);
- if (!buffer)
- return -ENOMEM;
+ if (!buffer) {
+ err = -ENOMEM;
+ goto fb_read_exit;
+ }

src = (u8 __iomem *) (info->screen_base + p);

@@ -754,37 +822,42 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)

kfree(buffer);

- return (err) ? err : cnt;
+ if (!err)
+ err = cnt;
+
+fb_read_exit:
+ fbops_unlock_fb_info(info);
+ return err;
}

static ssize_t
fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct inode *inode = file->f_path.dentry->d_inode;
- int fbidx = iminor(inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info *info = file->private_data;
u8 *buffer, *src;
u8 __iomem *dst;
- int c, cnt = 0, err = 0;
+ int c, cnt = 0, err;
unsigned long total_size;

- if (!info || !info->screen_base)
- return -ENODEV;
+ err = fbops_lock_fb_info(info);
+ if (err)
+ return err;

- if (info->state != FBINFO_STATE_RUNNING)
- return -EPERM;
+ if (info->fbops->fb_write) {
+ err = info->fbops->fb_write(info, buf, count, ppos);
+ goto fb_write_exit;
+ }

- if (info->fbops->fb_write)
- return info->fbops->fb_write(info, buf, count, ppos);
-
total_size = info->screen_size;

if (total_size == 0)
total_size = info->fix.smem_len;

- if (p > total_size)
- return -EFBIG;
+ if (p > total_size) {
+ err = -EFBIG;
+ goto fb_write_exit;
+ }

if (count > total_size) {
err = -EFBIG;
@@ -800,8 +873,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)

buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
GFP_KERNEL);
- if (!buffer)
- return -ENOMEM;
+ if (!buffer) {
+ err = -ENOMEM;
+ goto fb_write_exit;
+ }

dst = (u8 __iomem *) (info->screen_base + p);

@@ -828,7 +903,12 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)

kfree(buffer);

- return (cnt) ? cnt : err;
+ if (cnt)
+ err = cnt;
+
+fb_write_exit:
+ fbops_unlock_fb_info(info);
+ return err;
}

int
@@ -1141,9 +1221,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,

static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
- struct inode *inode = file->f_path.dentry->d_inode;
- int fbidx = iminor(inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info *info = file->private_data;

return do_fb_ioctl(info, cmd, arg);
}
@@ -1265,9 +1343,7 @@ static int fb_get_fscreeninfo(struct fb_info *info, unsigned int cmd,
static long fb_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
- struct inode *inode = file->f_path.dentry->d_inode;
- int fbidx = iminor(inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info *info = file->private_data;
struct fb_ops *fb = info->fbops;
long ret = -ENOIOCTLCMD;

@@ -1303,8 +1379,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
static int
fb_mmap(struct file *file, struct vm_area_struct * vma)
{
- int fbidx = iminor(file->f_path.dentry->d_inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info *info = file->private_data;
struct fb_ops *fb = info->fbops;
unsigned long off;
unsigned long start;
@@ -1380,6 +1455,8 @@ __releases(&info->lock)
if (res)
module_put(info->fbops->owner);
}
+ if (!res)
+ fb_kref_get(info);
#ifdef CONFIG_FB_DEFERRED_IO
if (info->fbdefio)
fb_deferred_io_open(info, inode, file);
@@ -1401,6 +1478,7 @@ __releases(&info->lock)
info->fbops->fb_release(info,1);
module_put(info->fbops->owner);
mutex_unlock(&info->lock);
+ fb_kref_put(info, false);
return 0;
}

@@ -1549,6 +1627,7 @@ register_framebuffer(struct fb_info *fb_info)
fb_info->node = i;
mutex_init(&fb_info->lock);
mutex_init(&fb_info->mm_lock);
+ fb_kref_init(fb_info);

fb_info->dev = device_create(fb_class, fb_info->device,
MKDEV(FB_MAJOR, i), NULL, "fb%d", i);
@@ -1622,9 +1701,9 @@ unregister_framebuffer(struct fb_info *fb_info)
goto done;
}

-
if (!lock_fb_info(fb_info))
return -ENODEV;
+ fb_info->state = FBINFO_STATE_EXITING;
event.info = fb_info;
ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
unlock_fb_info(fb_info);
@@ -1644,10 +1723,7 @@ unregister_framebuffer(struct fb_info *fb_info)
device_destroy(fb_class, MKDEV(FB_MAJOR, i));
event.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);
+ fb_kref_put(fb_info, true);
done:
return ret;
}
@@ -1655,7 +1731,7 @@ done:
/**
* fb_set_suspend - low level driver signals suspend
* @info: framebuffer affected
- * @state: 0 = resuming, !=0 = suspending
+ * @state: 0 = resuming, 1 = suspending
*
* This is meant to be used by low level drivers to
* signal suspend/resume to the core & clients.
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index 0a08f13..be361b5 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -398,6 +398,8 @@ static ssize_t store_fbstate(struct device *device,
char *last = NULL;

state = simple_strtoul(buf, &last, 0);
+ if (state && state > 1)
+ return -EINVAL;

acquire_console_sem();
fb_set_suspend(fb_info, (int)state);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index d1631d3..836f605 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -871,6 +871,7 @@ struct fb_info {
void *pseudo_palette; /* Fake palette of 16 colors */
#define FBINFO_STATE_RUNNING 0
#define FBINFO_STATE_SUSPENDED 1
+#define FBINFO_STATE_EXITING 2
u32 state; /* Hardware state i.e suspend */
void *fbcon_par; /* fbcon use-only private area */
/* From here on everything is device dependent */
@@ -885,6 +886,7 @@ struct fb_info {
resource_size_t size;
} ranges[0];
} *apertures;
+ struct kref fwfb_rfcnt;
};

static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
--
1.7.3.4


\
 
 \ /
  Last update: 2011-01-19 00:11    [W:0.172 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site