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: [email protected] Signed-off-by: Melbin K Mathew <[email protected]> --- drivers/video/fbdev/core/fbsysfs.c | 46 ++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c index af21dc5052..d3d60c555b 100644 --- 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; } -- 2.39.5
