The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- Note: Weigang asked for this to stay under embargo until it's all review and tested. -Daniel --- drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) { + var->xres != info->var.xres || var->yres != info->var.yres || + 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,
Update Michel's address.
On Tue, 21 Jun 2022 at 11:23, Daniel Vetter daniel.vetter@ffwll.ch wrote:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) {
var->xres != info->var.xres || var->yres != info->var.yres ||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,-- 2.36.0
Hi
Am 21.06.22 um 11:23 schrieb Daniel Vetter:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) {
var->xres != info->var.xres || var->yres != info->var.yres ||
This looks reasonable. We effectively only support a single resolution here.
var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
AFAIU this change would require that all userspace always uses maximum values for {xres,yres}_virtual. Seems unrealistic to me.
Best regards Thomas
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,
update Michel's address
Am 21.06.22 um 12:15 schrieb Thomas Zimmermann:
Hi
Am 21.06.22 um 11:23 schrieb Daniel Vetter:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) { + var->xres != info->var.xres || var->yres != info->var.yres ||
This looks reasonable. We effectively only support a single resolution here.
+ var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
AFAIU this change would require that all userspace always uses maximum values for {xres,yres}_virtual. Seems unrealistic to me.
Best regards Thomas
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,
On Tue, 21 Jun 2022 at 12:15, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 21.06.22 um 11:23 schrieb Daniel Vetter:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) {
var->xres != info->var.xres || var->yres != info->var.yres ||This looks reasonable. We effectively only support a single resolution here.
var->xres_virtual != fb->width || var->yres_virtual != fb->height) {AFAIU this change would require that all userspace always uses maximum values for {xres,yres}_virtual. Seems unrealistic to me.
The thing is, they kinda have to, because that's always going to be the actually visible part :-) Otoh I guess we could also allow virtual size to match the fbdev real size, but maybe that kind of sanity check should be done in fbmem.c?
Tbh for these kind of things I'm leaning towards "let's wait until we get a regression report", since there's simply too many random bugs all over in the fbcon/vc/vt code when sou start resizing stuff. So I'm very heavily leaning towards rejecting everything (and e.g. we should have fixed this all up already in 2020 when the bugfix for x/yres related underflows landed in 2020 imo). -Daniel
Best regards Thomas
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,-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Hi
Am 21.06.22 um 12:20 schrieb Daniel Vetter:
On Tue, 21 Jun 2022 at 12:15, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 21.06.22 um 11:23 schrieb Daniel Vetter:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) {
var->xres != info->var.xres || var->yres != info->var.yres ||This looks reasonable. We effectively only support a single resolution here.
var->xres_virtual != fb->width || var->yres_virtual != fb->height) {AFAIU this change would require that all userspace always uses maximum values for {xres,yres}_virtual. Seems unrealistic to me.
The thing is, they kinda have to, because that's always going to be the actually visible part :-) Otoh I guess we could also allow virtual size to match the fbdev real size, but maybe that kind of sanity check should be done in fbmem.c?
I'm not sure I understand. I'd expect that fbdev userspace allocates xres_virtual to be twice the size of xres. So it can do double buffering. In many cases fb->height would be larger than that.
Tbh for these kind of things I'm leaning towards "let's wait until we get a regression report", since there's simply too many random bugs
Not that I disagree, but in this case a regression report seems inevitable.
all over in the fbcon/vc/vt code when sou start resizing stuff. So I'm very heavily leaning towards rejecting everything (and e.g. we should have fixed this all up already in 2020 when the bugfix for x/yres related underflows landed in 2020 imo).
Do the fbdev igt tests work with this change if overalloc has been set to more than 200?
Best regards Thomas
-Daniel
Best regards Thomas
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,-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
On Tue, 21 Jun 2022 at 12:33, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 21.06.22 um 12:20 schrieb Daniel Vetter:
On Tue, 21 Jun 2022 at 12:15, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 21.06.22 um 11:23 schrieb Daniel Vetter:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) {
var->xres != info->var.xres || var->yres != info->var.yres ||This looks reasonable. We effectively only support a single resolution here.
var->xres_virtual != fb->width || var->yres_virtual != fb->height) {AFAIU this change would require that all userspace always uses maximum values for {xres,yres}_virtual. Seems unrealistic to me.
The thing is, they kinda have to, because that's always going to be the actually visible part :-) Otoh I guess we could also allow virtual size to match the fbdev real size, but maybe that kind of sanity check should be done in fbmem.c?
I'm not sure I understand. I'd expect that fbdev userspace allocates xres_virtual to be twice the size of xres. So it can do double buffering. In many cases fb->height would be larger than that.
Tbh for these kind of things I'm leaning towards "let's wait until we get a regression report", since there's simply too many random bugs
Not that I disagree, but in this case a regression report seems inevitable.
Hm yeah maybe we need to allow the flexibility.
all over in the fbcon/vc/vt code when sou start resizing stuff. So I'm very heavily leaning towards rejecting everything (and e.g. we should have fixed this all up already in 2020 when the bugfix for x/yres related underflows landed in 2020 imo).
Do the fbdev igt tests work with this change if overalloc has been set to more than 200?
Hm haven't tried, but I guess I'll just cc the thing to our trybot CI. The joys of patch testing when you can't really use any of the public CI systems by default. -Daniel
Best regards Thomas
-Daniel
Best regards Thomas
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,-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Am 21.06.22 um 12:33 schrieb Thomas Zimmermann:
Hi
Am 21.06.22 um 12:20 schrieb Daniel Vetter:
On Tue, 21 Jun 2022 at 12:15, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 21.06.22 um 11:23 schrieb Daniel Vetter:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) { + var->xres != info->var.xres || var->yres != info->var.yres ||
This looks reasonable. We effectively only support a single resolution here.
+ var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
AFAIU this change would require that all userspace always uses maximum values for {xres,yres}_virtual. Seems unrealistic to me.
The thing is, they kinda have to, because that's always going to be the actually visible part :-) Otoh I guess we could also allow virtual size to match the fbdev real size, but maybe that kind of sanity check should be done in fbmem.c?
I'm not sure I understand. I'd expect that fbdev userspace allocates xres_virtual to be twice the size of xres. So it can do double buffering. In many cases fb->height would be larger than that.
I meant yres and yres_virtual here.
Tbh for these kind of things I'm leaning towards "let's wait until we get a regression report", since there's simply too many random bugs
Not that I disagree, but in this case a regression report seems inevitable.
all over in the fbcon/vc/vt code when sou start resizing stuff. So I'm very heavily leaning towards rejecting everything (and e.g. we should have fixed this all up already in 2020 when the bugfix for x/yres related underflows landed in 2020 imo).
Do the fbdev igt tests work with this change if overalloc has been set to more than 200?
Best regards Thomas
-Daniel
Best regards Thomas
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,
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
On 6/21/22 11:23, Daniel Vetter wrote:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
I don't understand why you are blaming fbcon here (again)...
The real problem is that user-provided input (virt/physical screen sizes) isn't correctly validated. And that's even what your patch below does.
Helge
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) {
var->xres != info->var.xres || var->yres != info->var.yres ||var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
On Tue, 21 Jun 2022 at 16:57, Helge Deller deller@gmx.de wrote:
On 6/21/22 11:23, Daniel Vetter wrote:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
I don't understand why you are blaming fbcon here (again)...
The real problem is that user-provided input (virt/physical screen sizes) isn't correctly validated. And that's even what your patch below does.
I don't want to play whack-a-mole in here. And what I tried to do here (but oh well, too many things would break) is outright disallow any changes, not just try to validate (and probably in vain) that the changes look decent. Because making stuff invariant also solves all the locking fun. And sure even then we could have bugs that break stuff, but since everything would be invariant people would notice when booting, instead of trying to hit corner cases using syzkaller for stuff that mostly only syzkaller exercises.
And I'm pretty sure syzkaller isn't good enough to really hit concurrency issues, it has a pretty hard time just finding basic validation bugs like this. -Daniel
Helge
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) {
var->xres != info->var.xres || var->yres != info->var.yres ||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,
On Tue, 21 Jun 2022 at 17:37, Daniel Vetter daniel.vetter@ffwll.ch wrote:
On Tue, 21 Jun 2022 at 16:57, Helge Deller deller@gmx.de wrote:
On 6/21/22 11:23, Daniel Vetter wrote:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
I don't understand why you are blaming fbcon here (again)...
The real problem is that user-provided input (virt/physical screen sizes) isn't correctly validated. And that's even what your patch below does.
I don't want to play whack-a-mole in here. And what I tried to do here (but oh well, too many things would break) is outright disallow any changes, not just try to validate (and probably in vain) that the changes look decent. Because making stuff invariant also solves all the locking fun. And sure even then we could have bugs that break stuff, but since everything would be invariant people would notice when booting, instead of trying to hit corner cases using syzkaller for stuff that mostly only syzkaller exercises.
And I'm pretty sure syzkaller isn't good enough to really hit concurrency issues, it has a pretty hard time just finding basic validation bugs like this.
Like I'm pretty sure if you make the font big enough and yres small enough this can all blow up right away again, but I also really don't want to find out.
And once you fix that you get to fix the races of changing fonts through vt against changing resolution through fbdev ioctls, and at that point you have a neat locking problem to solve. -Daniel
Helge
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) {
var->xres != info->var.xres || var->yres != info->var.yres ||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,-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On 6/21/22 17:40, Daniel Vetter wrote:
On Tue, 21 Jun 2022 at 17:37, Daniel Vetter daniel.vetter@ffwll.ch wrote:
On Tue, 21 Jun 2022 at 16:57, Helge Deller deller@gmx.de wrote:
On 6/21/22 11:23, Daniel Vetter wrote:
The drm fbdev emulation does not forward mode changes to the driver, and hence all changes where rejected in 865afb11949e ("drm/fb-helper: reject any changes to the fbdev").
Unfortunately this resulted in bugs on multiple monitor systems with different resolutions. In that case the fbdev emulation code sizes the underlying framebuffer for the largest screen (which dictates x/yres_virtual), but adjust the fbdev x/yres to match the smallest resolution. The above mentioned patch failed to realize that, and errornously validated x/yres against the fb dimensions.
This was fixed by just dropping the validation for too small sizes, which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again").
But this also restored all kinds of validation issues and their fallout in the notoriously buggy fbcon code for too small sizes. Since no one is volunteering to really make fbcon and vc/vt fully robust against these math issues make sure this barn door is closed for good again.
I don't understand why you are blaming fbcon here (again)...
The real problem is that user-provided input (virt/physical screen sizes) isn't correctly validated. And that's even what your patch below does.
I don't want to play whack-a-mole in here. And what I tried to do here (but oh well, too many things would break) is outright disallow any changes, not just try to validate (and probably in vain) that the changes look decent. Because making stuff invariant also solves all the locking fun. And sure even then we could have bugs that break stuff, but since everything would be invariant people would notice when booting, instead of trying to hit corner cases using syzkaller for stuff that mostly only syzkaller exercises.
And I'm pretty sure syzkaller isn't good enough to really hit concurrency issues, it has a pretty hard time just finding basic validation bugs like this.
Like I'm pretty sure if you make the font big enough and yres small enough this can all blow up right away again, but I also really don't want to find out.
Yes, the font thing is something which came to my mind as well. But that's probably another patch...
And once you fix that you get to fix the races of changing fonts through vt against changing resolution through fbdev ioctls, and at that point you have a neat locking problem to solve.
Yes, maybe. But people need/want to be able to change screen resolutions and fonts, so it has to be made working somehow, e.g. by returning -EAGAIN temporarily.
Helge
-Daniel
Helge
Since it's a bit tricky to remember the x/yres we picked across both the newer generic fbdev emulation and the older code with more driver involvement, we simply check that it doesn't change. This relies on drm_fb_helper_fill_var() having done things correctly, and nothing having trampled it yet.
Note that this leaves all the other fbdev drivers out in the rain. Given that distros have finally started to move away from those completely for real I think that's good enough. The code it spaghetti enough that I do not feel confident to even review fixes for it.
What might help fbdev is doing something similar to what was done in a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure x/yres_virtual aren't too small, for some value of "too small". Maybe checking that they're at least x/yres makes sense?
Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again") Cc: Michel Dänzer michel.daenzer@amd.com Cc: Daniel Stone daniels@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: stable@vger.kernel.org # v4.11+ Cc: Helge Deller deller@gmx.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: openeuler-security@openeuler.org Cc: guodaxing@huawei.com Cc: Weigang (Jimmy) weigang12@huawei.com Reported-by: Weigang (Jimmy) weigang12@huawei.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Note: Weigang asked for this to stay under embargo until it's all review and tested.
-Daniel
drivers/gpu/drm/drm_fb_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 695997ae2a7c..5664a177a404 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, * 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) {
var->xres != info->var.xres || var->yres != info->var.yres ||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,-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
linux-stable-mirror@lists.linaro.org
-
Daniel Vetter -
Helge Deller -
Thomas Zimmermann