| Message ID | 20230404193934.472457-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | fbmem: Reject FB_ACTIVATE_KD_TEXT from userspace | expand |
On Tue, Apr 04, 2023 at 09:39:34PM +0200, Daniel Vetter wrote: > This is an oversight from dc5bdb68b5b3 ("drm/fb-helper: Fix vt > restore") - I failed to realize that nasty userspace could set this. > > It's not pretty to mix up kernel-internal and userspace uapi flags > like this, but since the entire fb_var_screeninfo structure is uapi > we'd need to either add a new parameter to the ->fb_set_par callback > and fb_set_par() function, which has a _lot_ of users. Or some other > fairly ugly side-channel int fb_info. Neither is a pretty prospect. > > Instead just correct the issue at hand by filtering out this > kernel-internal flag in the ioctl handling code. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Fixes: dc5bdb68b5b3 ("drm/fb-helper: Fix vt restore") > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: shlomo@fastmail.com > Cc: Michel Dänzer <michel@daenzer.net> > Cc: Noralf Trønnes <noralf@tronnes.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: dri-devel@lists.freedesktop.org > Cc: <stable@vger.kernel.org> # v5.7+ > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Nathan Chancellor <natechancellor@gmail.com> > Cc: Qiujun Huang <hqjagain@gmail.com> > Cc: Peter Rosin <peda@axentia.se> > Cc: linux-fbdev@vger.kernel.org > Cc: Helge Deller <deller@gmx.de> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Samuel Thibault <samuel.thibault@ens-lyon.org> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Cc: Shigeru Yoshida <syoshida@redhat.com> An Ack on this (or a better idea) would be great, so I can stuff it into -fixes. Thanks, Daniel > --- > drivers/video/fbdev/core/fbmem.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 875541ff185b..3fd95a79e4c3 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1116,6 +1116,8 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > case FBIOPUT_VSCREENINFO: > if (copy_from_user(&var, argp, sizeof(var))) > return -EFAULT; > + /* only for kernel-internal use */ > + var.activate &= ~FB_ACTIVATE_KD_TEXT; > console_lock(); > lock_fb_info(info); > ret = fbcon_modechange_possible(info, &var); > -- > 2.40.0 >
On 2023-04-11 15:44, Daniel Vetter wrote: > On Tue, Apr 04, 2023 at 09:39:34PM +0200, Daniel Vetter wrote: >> This is an oversight from dc5bdb68b5b3 ("drm/fb-helper: Fix vt >> restore") - I failed to realize that nasty userspace could set this. >> >> It's not pretty to mix up kernel-internal and userspace uapi flags >> like this, but since the entire fb_var_screeninfo structure is uapi >> we'd need to either add a new parameter to the ->fb_set_par callback >> and fb_set_par() function, which has a _lot_ of users. Or some other >> fairly ugly side-channel int fb_info. Neither is a pretty prospect. >> >> Instead just correct the issue at hand by filtering out this >> kernel-internal flag in the ioctl handling code. >> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> Fixes: dc5bdb68b5b3 ("drm/fb-helper: Fix vt restore") >> Cc: Alex Deucher <alexander.deucher@amd.com> >> Cc: shlomo@fastmail.com >> Cc: Michel Dänzer <michel@daenzer.net> >> Cc: Noralf Trønnes <noralf@tronnes.org> >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Maxime Ripard <mripard@kernel.org> >> Cc: David Airlie <airlied@linux.ie> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Cc: dri-devel@lists.freedesktop.org >> Cc: <stable@vger.kernel.org> # v5.7+ >> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> Cc: Geert Uytterhoeven <geert@linux-m68k.org> >> Cc: Nathan Chancellor <natechancellor@gmail.com> >> Cc: Qiujun Huang <hqjagain@gmail.com> >> Cc: Peter Rosin <peda@axentia.se> >> Cc: linux-fbdev@vger.kernel.org >> Cc: Helge Deller <deller@gmx.de> >> Cc: Sam Ravnborg <sam@ravnborg.org> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org> >> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> Cc: Shigeru Yoshida <syoshida@redhat.com> > An Ack on this (or a better idea) would be great, so I can stuff it into > -fixes. Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Daniel Vetter <daniel.vetter@ffwll.ch> writes: > This is an oversight from dc5bdb68b5b3 ("drm/fb-helper: Fix vt > restore") - I failed to realize that nasty userspace could set this. > > It's not pretty to mix up kernel-internal and userspace uapi flags > like this, but since the entire fb_var_screeninfo structure is uapi > we'd need to either add a new parameter to the ->fb_set_par callback > and fb_set_par() function, which has a _lot_ of users. Or some other > fairly ugly side-channel int fb_info. Neither is a pretty prospect. > > Instead just correct the issue at hand by filtering out this > kernel-internal flag in the ioctl handling code. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Fixes: dc5bdb68b5b3 ("drm/fb-helper: Fix vt restore") [..] > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 875541ff185b..3fd95a79e4c3 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1116,6 +1116,8 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > case FBIOPUT_VSCREENINFO: > if (copy_from_user(&var, argp, sizeof(var))) > return -EFAULT; > + /* only for kernel-internal use */ > + var.activate &= ~FB_ACTIVATE_KD_TEXT; > console_lock(); I don't have a better idea on how to fix this and as you said the whole struct fb_var_screeninfo is an uAPI anyways... Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
On Tue, Apr 11, 2023 at 04:03:24PM +0200, Javier Martinez Canillas wrote: > Daniel Vetter <daniel.vetter@ffwll.ch> writes: > > > This is an oversight from dc5bdb68b5b3 ("drm/fb-helper: Fix vt > > restore") - I failed to realize that nasty userspace could set this. > > > > It's not pretty to mix up kernel-internal and userspace uapi flags > > like this, but since the entire fb_var_screeninfo structure is uapi > > we'd need to either add a new parameter to the ->fb_set_par callback > > and fb_set_par() function, which has a _lot_ of users. Or some other > > fairly ugly side-channel int fb_info. Neither is a pretty prospect. > > > > Instead just correct the issue at hand by filtering out this > > kernel-internal flag in the ioctl handling code. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Fixes: dc5bdb68b5b3 ("drm/fb-helper: Fix vt restore") > > [..] > > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > > index 875541ff185b..3fd95a79e4c3 100644 > > --- a/drivers/video/fbdev/core/fbmem.c > > +++ b/drivers/video/fbdev/core/fbmem.c > > @@ -1116,6 +1116,8 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > case FBIOPUT_VSCREENINFO: > > if (copy_from_user(&var, argp, sizeof(var))) > > return -EFAULT; > > + /* only for kernel-internal use */ > > + var.activate &= ~FB_ACTIVATE_KD_TEXT; > > console_lock(); > > I don't have a better idea on how to fix this and as you said the whole > struct fb_var_screeninfo is an uAPI anyways... > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Thanks for taking a look, merged to drm-misc-fixes. > > -- > Best regards, > > Javier Martinez Canillas > Core Platforms > Red Hat >
Hi Daniel, On Tue, Apr 11, 2023 at 3:44 PM Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Apr 04, 2023 at 09:39:34PM +0200, Daniel Vetter wrote: > > This is an oversight from dc5bdb68b5b3 ("drm/fb-helper: Fix vt > > restore") - I failed to realize that nasty userspace could set this. > > > > It's not pretty to mix up kernel-internal and userspace uapi flags > > like this, but since the entire fb_var_screeninfo structure is uapi > > we'd need to either add a new parameter to the ->fb_set_par callback > > and fb_set_par() function, which has a _lot_ of users. Or some other > > fairly ugly side-channel int fb_info. Neither is a pretty prospect. > > > > Instead just correct the issue at hand by filtering out this > > kernel-internal flag in the ioctl handling code. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Fixes: dc5bdb68b5b3 ("drm/fb-helper: Fix vt restore") > An Ack on this (or a better idea) would be great, so I can stuff it into > -fixes. I don't understand what the original commit this fixes is doing anyway... > > --- a/drivers/video/fbdev/core/fbmem.c > > +++ b/drivers/video/fbdev/core/fbmem.c > > @@ -1116,6 +1116,8 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > case FBIOPUT_VSCREENINFO: > > if (copy_from_user(&var, argp, sizeof(var))) > > return -EFAULT; > > + /* only for kernel-internal use */ > > + var.activate &= ~FB_ACTIVATE_KD_TEXT; > > console_lock(); > > lock_fb_info(info); > > ret = fbcon_modechange_possible(info, &var); Perhaps FB_ACTIVATE_KD_TEXT should be removed (marked as reserved) from include/uapi/linux/fb.h, too? Gr{oetje,eeting}s, Geert
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 875541ff185b..3fd95a79e4c3 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1116,6 +1116,8 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, case FBIOPUT_VSCREENINFO: if (copy_from_user(&var, argp, sizeof(var))) return -EFAULT; + /* only for kernel-internal use */ + var.activate &= ~FB_ACTIVATE_KD_TEXT; console_lock(); lock_fb_info(info); ret = fbcon_modechange_possible(info, &var);
This is an oversight from dc5bdb68b5b3 ("drm/fb-helper: Fix vt restore") - I failed to realize that nasty userspace could set this. It's not pretty to mix up kernel-internal and userspace uapi flags like this, but since the entire fb_var_screeninfo structure is uapi we'd need to either add a new parameter to the ->fb_set_par callback and fb_set_par() function, which has a _lot_ of users. Or some other fairly ugly side-channel int fb_info. Neither is a pretty prospect. Instead just correct the issue at hand by filtering out this kernel-internal flag in the ioctl handling code. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Fixes: dc5bdb68b5b3 ("drm/fb-helper: Fix vt restore") Cc: Alex Deucher <alexander.deucher@amd.com> Cc: shlomo@fastmail.com Cc: Michel Dänzer <michel@daenzer.net> Cc: Noralf Trønnes <noralf@tronnes.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: dri-devel@lists.freedesktop.org Cc: <stable@vger.kernel.org> # v5.7+ Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Nathan Chancellor <natechancellor@gmail.com> Cc: Qiujun Huang <hqjagain@gmail.com> Cc: Peter Rosin <peda@axentia.se> Cc: linux-fbdev@vger.kernel.org Cc: Helge Deller <deller@gmx.de> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Shigeru Yoshida <syoshida@redhat.com> --- drivers/video/fbdev/core/fbmem.c | 2 ++ 1 file changed, 2 insertions(+)