fbdev: serialize mode sysfs access with lock_fb_info()

show_mode(), show_modes(), and store_mode() access fb_info->modelist
and fb_info->mode without holding lock_fb_info(). store_modes() takes
lock_fb_info() while replacing the modelist and freeing the old one.

A concurrent reader or writer can load a pointer to an old modelist
entry before store_modes() frees it, then dereference freed memory or
store a stale freed pointer in fb_info->mode.

Take lock_fb_info() in show_mode(), show_modes(), and store_mode() to
serialize with store_modes(). In show_mode(), copy the mode to the
stack and format after dropping the lock. In store_mode(), split
activate() into a _locked variant to avoid double-locking, and hold
the locks for the modelist walk, mode conversion, activation, and
fb_info->mode assignment together.

Cc: stable@vger.kernel.org # v7.1+
Signed-off-by: Melbin K Mathew <mlbnkm1@gmail.com>
Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index af21dc5052df37825d5ff858c02cfd47a7b13dce..d3d60c555bb86fb644bf83a0dbf1dc17ccaf69b2 100644 (file)
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -12,19 +12,24 @@
 #include "fb_internal.h"
 #include "fbcon.h"
 
+static int activate_locked(struct fb_info *fb_info,
+                           struct fb_var_screeninfo *var)
+{
+       var->activate |= FB_ACTIVATE_FORCE;
+       return fb_set_var_from_user(fb_info, var);
+}
+
 static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
 {
        int err;
 
-       var->activate |= FB_ACTIVATE_FORCE;
        console_lock();
        lock_fb_info(fb_info);
-       err = fb_set_var_from_user(fb_info, var);
+       err = activate_locked(fb_info, var);
        unlock_fb_info(fb_info);
        console_unlock();
-       if (err)
-               return err;
-       return 0;
+
+       return err;
 }
 
 static int mode_string(char *buf, size_t size, unsigned int offset,
@@ -65,6 +70,9 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
 
        memset(&var, 0, sizeof(var));
 
+       console_lock();
+       lock_fb_info(fb_info);
+
        list_for_each_entry(modelist, &fb_info->modelist, list) {
                mode = &modelist->mode;
                i = mode_string(mstr, sizeof(mstr), 0, mode);
@@ -72,12 +80,22 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
 
                        var = fb_info->var;
                        fb_videomode_to_var(&var, mode);
-                       if ((err = activate(fb_info, &var)))
+                       err = activate_locked(fb_info, &var);
+                       if (err) {
+                               unlock_fb_info(fb_info);
+                               console_unlock();
                                return err;
+                       }
                        fb_info->mode = mode;
+                       unlock_fb_info(fb_info);
+                       console_unlock();
                        return count;
                }
        }
+
+       unlock_fb_info(fb_info);
+       console_unlock();
+
        return -EINVAL;
 }
 
@@ -85,11 +103,20 @@ static ssize_t show_mode(struct device *device, struct device_attribute *attr,
                         char *buf)
 {
        struct fb_info *fb_info = dev_get_drvdata(device);
+       struct fb_videomode mode;
+       bool have_mode = false;
 
-       if (!fb_info->mode)
+       lock_fb_info(fb_info);
+       if (fb_info->mode) {
+               mode = *fb_info->mode;
+               have_mode = true;
+       }
+       unlock_fb_info(fb_info);
+
+       if (!have_mode)
                return 0;
 
-       return mode_string(buf, PAGE_SIZE, 0, fb_info->mode);
+       return mode_string(buf, PAGE_SIZE, 0, &mode);
 }
 
 static ssize_t store_modes(struct device *device,
@@ -137,12 +164,15 @@ static ssize_t show_modes(struct device *device, struct device_attribute *attr,
        const struct fb_videomode *mode;
 
        i = 0;
+       lock_fb_info(fb_info);
        list_for_each_entry(modelist, &fb_info->modelist, list) {
                mode = &modelist->mode;
                i += mode_string(buf, PAGE_SIZE, i, mode);
                if (i >= PAGE_SIZE - 1)
                        break;
        }
+       unlock_fb_info(fb_info);
+
        return i;
 }