On Sat, Jul 02, 2022 at 08:05:54PM +0200, Helge Deller wrote: > On 6/29/22 12:56, Geert Uytterhoeven wrote: > > The DRM fbdev emulation layer does not support pushing back > > changes to fb_var_screeninfo to KMS. > > > > However, drm_fb_helper still implements the fb_ops.fb_check_var() and > > fb_ops.fb_set_par() callbacks, but the former fails to validate various > > parameters passed from userspace. Hence unsanitized and possible > > invaled values are passed up through the fbdev, fbcon, and console > > stack, which has become an endless source of security issues reported > > against fbdev. > > > > Fix this by not populating the fb_ops.fb_check_var() and > > fb_ops.fb_set_par() callbacks, as there is no point in providing them if > > the frame buffer config cannot be changed anyway. This makes the fbdev > > core aware that making changes to the frame buffer config is not > > supported, so it will always return the current config. > > > > Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> > > It's unfortunate that DRM currently isn't able to switch resolutions > at runtime. > > With that in mind I agree with Geert that it's probably better to > disable (or drop) that code until DRM can cope with fbdev's > interface to switch resolutions. > > Furthermore, with the upcoming patches to fbcon (which prevents crashes on > invalid userspace input), you will face a kernel WARNING if you call fbset > to switch screen resolutions with DRM drivers. > > So, from my side (although I'd prefer a better fix for DRM): > > Acked-by: Helge Deller <deller@xxxxxx> So maybe I'm missing something, but I think this breaks a lot of stuff. The issue is that fbdev is only a subordinate owner of the kms device, if there's a real drm kms owner around that wins. Which means when you switch back then set_par needs to restore the fbdev framebuffer. So that might break some recovery/use-cases. The other thing is that I think this also breaks the scanout offset, and people do use that for double-buffering on top of fbdev on top of drm drivers. So we can't just nuke it completely. For better or worse I think we need to keep playing the whack-a-mole game. Or do I miss something here? -Daniel > > Helge > > > --- > > The only remaining DRM driver that implements fb_ops.fb_check_var() is > > also broken, as it fails to validate various parameters passed from > > userspace. So vmw_fb_check_var() should either be fixed, or removed. > > --- > > drivers/gpu/drm/drm_fb_helper.c | 180 ++------------------- > > drivers/gpu/drm/i915/display/intel_fbdev.c | 15 -- > > drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 - > > include/drm/drm_fb_helper.h | 16 -- > > 4 files changed, 13 insertions(+), 200 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > index 2d4cee6a10ffffe7..1041a11c410d7967 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -228,9 +228,18 @@ int drm_fb_helper_debug_leave(struct fb_info *info) > > } > > EXPORT_SYMBOL(drm_fb_helper_debug_leave); > > > > -static int > > -__drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper, > > - bool force) > > +/** > > + * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration > > + * @fb_helper: driver-allocated fbdev helper, can be NULL > > + * > > + * This should be called from driver's drm &drm_driver.lastclose callback > > + * when implementing an fbcon on top of kms using this helper. This ensures that > > + * the user isn't greeted with a black screen when e.g. X dies. > > + * > > + * RETURNS: > > + * Zero if everything went ok, negative error code otherwise. > > + */ > > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > > { > > bool do_delayed; > > int ret; > > @@ -242,16 +251,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper, > > return 0; > > > > mutex_lock(&fb_helper->lock); > > - if (force) { > > - /* > > - * Yes this is the _locked version which expects the master lock > > - * to be held. But for forced restores we're intentionally > > - * racing here, see drm_fb_helper_set_par(). > > - */ > > - ret = drm_client_modeset_commit_locked(&fb_helper->client); > > - } else { > > - ret = drm_client_modeset_commit(&fb_helper->client); > > - } > > + ret = drm_client_modeset_commit(&fb_helper->client); > > > > do_delayed = fb_helper->delayed_hotplug; > > if (do_delayed) > > @@ -263,22 +263,6 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper, > > > > return ret; > > } > > - > > -/** > > - * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration > > - * @fb_helper: driver-allocated fbdev helper, can be NULL > > - * > > - * This should be called from driver's drm &drm_driver.lastclose callback > > - * when implementing an fbcon on top of kms using this helper. This ensures that > > - * the user isn't greeted with a black screen when e.g. X dies. > > - * > > - * RETURNS: > > - * Zero if everything went ok, negative error code otherwise. > > - */ > > -int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > > -{ > > - return __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, false); > > -} > > EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked); > > > > #ifdef CONFIG_MAGIC_SYSRQ > > @@ -1254,25 +1238,6 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, > > } > > EXPORT_SYMBOL(drm_fb_helper_ioctl); > > > > -static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1, > > - const struct fb_var_screeninfo *var_2) > > -{ > > - return var_1->bits_per_pixel == var_2->bits_per_pixel && > > - var_1->grayscale == var_2->grayscale && > > - var_1->red.offset == var_2->red.offset && > > - var_1->red.length == var_2->red.length && > > - var_1->red.msb_right == var_2->red.msb_right && > > - var_1->green.offset == var_2->green.offset && > > - var_1->green.length == var_2->green.length && > > - var_1->green.msb_right == var_2->green.msb_right && > > - var_1->blue.offset == var_2->blue.offset && > > - var_1->blue.length == var_2->blue.length && > > - var_1->blue.msb_right == var_2->blue.msb_right && > > - var_1->transp.offset == var_2->transp.offset && > > - var_1->transp.length == var_2->transp.length && > > - var_1->transp.msb_right == var_2->transp.msb_right; > > -} > > - > > static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var, > > u8 depth) > > { > > @@ -1331,123 +1296,6 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var, > > } > > } > > > > -/** > > - * drm_fb_helper_check_var - implementation for &fb_ops.fb_check_var > > - * @var: screeninfo to check > > - * @info: fbdev registered by the helper > > - */ > > -int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > > - struct fb_info *info) > > -{ > > - struct drm_fb_helper *fb_helper = info->par; > > - struct drm_framebuffer *fb = fb_helper->fb; > > - struct drm_device *dev = fb_helper->dev; > > - > > - if (in_dbg_master()) > > - return -EINVAL; > > - > > - if (var->pixclock != 0) { > > - drm_dbg_kms(dev, "fbdev emulation doesn't support changing the pixel clock, value of pixclock is ignored\n"); > > - var->pixclock = 0; > > - } > > - > > - if ((drm_format_info_block_width(fb->format, 0) > 1) || > > - (drm_format_info_block_height(fb->format, 0) > 1)) > > - return -EINVAL; > > - > > - /* > > - * Changes struct fb_var_screeninfo are currently not pushed back > > - * to KMS, hence fail if different settings are requested. > > - */ > > - if (var->bits_per_pixel > fb->format->cpp[0] * 8 || > > - var->xres > fb->width || var->yres > fb->height || > > - var->xres_virtual > fb->width || var->yres_virtual > fb->height) { > > - drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb " > > - "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n", > > - var->xres, var->yres, var->bits_per_pixel, > > - var->xres_virtual, var->yres_virtual, > > - fb->width, fb->height, fb->format->cpp[0] * 8); > > - return -EINVAL; > > - } > > - > > - /* > > - * Workaround for SDL 1.2, which is known to be setting all pixel format > > - * fields values to zero in some cases. We treat this situation as a > > - * kind of "use some reasonable autodetected values". > > - */ > > - if (!var->red.offset && !var->green.offset && > > - !var->blue.offset && !var->transp.offset && > > - !var->red.length && !var->green.length && > > - !var->blue.length && !var->transp.length && > > - !var->red.msb_right && !var->green.msb_right && > > - !var->blue.msb_right && !var->transp.msb_right) { > > - drm_fb_helper_fill_pixel_fmt(var, fb->format->depth); > > - } > > - > > - /* > > - * Likewise, bits_per_pixel should be rounded up to a supported value. > > - */ > > - var->bits_per_pixel = fb->format->cpp[0] * 8; > > - > > - /* > > - * drm fbdev emulation doesn't support changing the pixel format at all, > > - * so reject all pixel format changing requests. > > - */ > > - if (!drm_fb_pixel_format_equal(var, &info->var)) { > > - drm_dbg_kms(dev, "fbdev emulation doesn't support changing the pixel format\n"); > > - return -EINVAL; > > - } > > - > > - return 0; > > -} > > -EXPORT_SYMBOL(drm_fb_helper_check_var); > > - > > -/** > > - * drm_fb_helper_set_par - implementation for &fb_ops.fb_set_par > > - * @info: fbdev registered by the helper > > - * > > - * This will let fbcon do the mode init and is called at initialization time by > > - * the fbdev core when registering the driver, and later on through the hotplug > > - * callback. > > - */ > > -int drm_fb_helper_set_par(struct fb_info *info) > > -{ > > - struct drm_fb_helper *fb_helper = info->par; > > - struct fb_var_screeninfo *var = &info->var; > > - bool force; > > - > > - if (oops_in_progress) > > - return -EBUSY; > > - > > - if (var->pixclock != 0) { > > - drm_err(fb_helper->dev, "PIXEL CLOCK SET\n"); > > - return -EINVAL; > > - } > > - > > - /* > > - * Normally we want to make sure that a kms master takes precedence over > > - * fbdev, to avoid fbdev flickering and occasionally stealing the > > - * display status. But Xorg first sets the vt back to text mode using > > - * the KDSET IOCTL with KD_TEXT, and only after that drops the master > > - * status when exiting. > > - * > > - * In the past this was caught by drm_fb_helper_lastclose(), but on > > - * modern systems where logind always keeps a drm fd open to orchestrate > > - * the vt switching, this doesn't work. > > - * > > - * To not break the userspace ABI we have this special case here, which > > - * is only used for the above case. Everything else uses the normal > > - * commit function, which ensures that we never steal the display from > > - * an active drm master. > > - */ > > - force = var->activate & FB_ACTIVATE_KD_TEXT; > > - > > - __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, force); > > - > > - return 0; > > -} > > -EXPORT_SYMBOL(drm_fb_helper_set_par); > > - > > static void pan_set(struct drm_fb_helper *fb_helper, int x, int y) > > { > > struct drm_mode_set *mode_set; > > @@ -2028,8 +1876,6 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > > drm_setup_crtcs_fb(fb_helper); > > mutex_unlock(&fb_helper->lock); > > > > - drm_fb_helper_set_par(fb_helper->fbdev); > > - > > return 0; > > } > > EXPORT_SYMBOL(drm_fb_helper_hotplug_event); > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > > index 221336178991f04f..26dbe9487c79ae1b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > @@ -77,20 +77,6 @@ static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev) > > intel_frontbuffer_invalidate(to_frontbuffer(ifbdev), ORIGIN_CPU); > > } > > > > -static int intel_fbdev_set_par(struct fb_info *info) > > -{ > > - struct drm_fb_helper *fb_helper = info->par; > > - struct intel_fbdev *ifbdev = > > - container_of(fb_helper, struct intel_fbdev, helper); > > - int ret; > > - > > - ret = drm_fb_helper_set_par(info); > > - if (ret == 0) > > - intel_fbdev_invalidate(ifbdev); > > - > > - return ret; > > -} > > - > > static int intel_fbdev_blank(int blank, struct fb_info *info) > > { > > struct drm_fb_helper *fb_helper = info->par; > > @@ -123,7 +109,6 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var, > > static const struct fb_ops intelfb_ops = { > > .owner = THIS_MODULE, > > DRM_FB_HELPER_DEFAULT_OPS, > > - .fb_set_par = intel_fbdev_set_par, > > .fb_fillrect = drm_fb_helper_cfb_fillrect, > > .fb_copyarea = drm_fb_helper_cfb_copyarea, > > .fb_imageblit = drm_fb_helper_cfb_imageblit, > > diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c > > index 40706c5aad7b5c9b..2df8875baaca10b6 100644 > > --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c > > +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c > > @@ -74,8 +74,6 @@ static int omap_fbdev_pan_display(struct fb_var_screeninfo *var, > > static const struct fb_ops omap_fb_ops = { > > .owner = THIS_MODULE, > > > > - .fb_check_var = drm_fb_helper_check_var, > > - .fb_set_par = drm_fb_helper_set_par, > > .fb_setcmap = drm_fb_helper_setcmap, > > .fb_blank = drm_fb_helper_blank, > > .fb_pan_display = omap_fbdev_pan_display, > > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > > index 329607ca65c06684..19b40adc156295a1 100644 > > --- a/include/drm/drm_fb_helper.h > > +++ b/include/drm/drm_fb_helper.h > > @@ -200,8 +200,6 @@ drm_fb_helper_from_client(struct drm_client_dev *client) > > * functions. To be used in struct fb_ops of drm drivers. > > */ > > #define DRM_FB_HELPER_DEFAULT_OPS \ > > - .fb_check_var = drm_fb_helper_check_var, \ > > - .fb_set_par = drm_fb_helper_set_par, \ > > .fb_setcmap = drm_fb_helper_setcmap, \ > > .fb_blank = drm_fb_helper_blank, \ > > .fb_pan_display = drm_fb_helper_pan_display, \ > > @@ -217,9 +215,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *helper); > > int drm_fb_helper_blank(int blank, struct fb_info *info); > > int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, > > struct fb_info *info); > > -int drm_fb_helper_set_par(struct fb_info *info); > > -int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > > - struct fb_info *info); > > > > int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper); > > > > @@ -303,17 +298,6 @@ static inline int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, > > return 0; > > } > > > > -static inline int drm_fb_helper_set_par(struct fb_info *info) > > -{ > > - return 0; > > -} > > - > > -static inline int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > > - struct fb_info *info) > > -{ > > - return 0; > > -} > > - > > static inline int > > drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > > { > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch