diff mbox series

vt_ioctl: make VT_RESIZEX behave like VT_RESIZE

Message ID 4933b81b-9b1a-355b-df0e-9b31e8280ab9@i-love.sakura.ne.jp (mailing list archive)
State Superseded, archived
Headers show
Series vt_ioctl: make VT_RESIZEX behave like VT_RESIZE | expand

Commit Message

Tetsuo Handa Sept. 27, 2020, 11:46 a.m. UTC
syzbot is reporting UAF/OOB read at bit_putcs()/soft_cursor() [1][2], for
vt_resizex() from ioctl(VT_RESIZEX) allows setting font height larger than
actual font height calculated by con_font_set() from ioctl(PIO_FONT).
Since fbcon_set_font() from con_font_set() allocates minimal amount of
memory based on actual font height calculated by con_font_set(),
use of vt_resizex() can cause UAF/OOB read for font data.

VT_RESIZEX was introduced in Linux 1.3.3, but it is unclear that what
comes to the "+ more" part, and I couldn't find a user of VT_RESIZEX.

  #define VT_RESIZE   0x5609 /* set kernel's idea of screensize */
  #define VT_RESIZEX  0x560A /* set kernel's idea of screensize + more */

So far we are not aware of syzbot reports caused by setting non-zero value
to v_vlin parameter. But given that it is possible that nobody is using
VT_RESIZEX, we can try removing support for v_clin and v_vlin parameters.

Therefore, this patch effectively makes VT_RESIZEX behave like VT_RESIZE,
with emitting a message if somebody is still using v_clin and/or v_vlin
parameters.

[1] https://syzkaller.appspot.com/bug?id=32577e96d88447ded2d3b76d71254fb855245837
[2] https://syzkaller.appspot.com/bug?id=6b8355d27b2b94fb5cedf4655e3a59162d9e48e3

Reported-by: syzbot <syzbot+b308f5fd049fbbc6e74f@syzkaller.appspotmail.com>
Reported-by: syzbot <syzbot+16469b5e8e5a72e9131e@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/tty/vt/vt_ioctl.c | 57 +++++++--------------------------------
 1 file changed, 10 insertions(+), 47 deletions(-)

Comments

Greg KH Sept. 27, 2020, 12:06 p.m. UTC | #1
On Sun, Sep 27, 2020 at 08:46:30PM +0900, Tetsuo Handa wrote:
> syzbot is reporting UAF/OOB read at bit_putcs()/soft_cursor() [1][2], for
> vt_resizex() from ioctl(VT_RESIZEX) allows setting font height larger than
> actual font height calculated by con_font_set() from ioctl(PIO_FONT).
> Since fbcon_set_font() from con_font_set() allocates minimal amount of
> memory based on actual font height calculated by con_font_set(),
> use of vt_resizex() can cause UAF/OOB read for font data.
> 
> VT_RESIZEX was introduced in Linux 1.3.3, but it is unclear that what
> comes to the "+ more" part, and I couldn't find a user of VT_RESIZEX.
> 
>   #define VT_RESIZE   0x5609 /* set kernel's idea of screensize */
>   #define VT_RESIZEX  0x560A /* set kernel's idea of screensize + more */
> 
> So far we are not aware of syzbot reports caused by setting non-zero value
> to v_vlin parameter. But given that it is possible that nobody is using
> VT_RESIZEX, we can try removing support for v_clin and v_vlin parameters.

Debian code search doesn't show any users, and that's usually a good
indication of what userspace ioctls for old things like this, are being
used for.

So this makes sense to me, I'll queue it up, thanks!

greg k-h
Martin Hostettler Sept. 28, 2020, 5:59 p.m. UTC | #2
On Sun, Sep 27, 2020 at 08:46:30PM +0900, Tetsuo Handa wrote:
> VT_RESIZEX was introduced in Linux 1.3.3, but it is unclear that what
> comes to the "+ more" part, and I couldn't find a user of VT_RESIZEX.
> 

It seems this is/was used by "svgatextmode" which seems to be at
http://www.ibiblio.org/pub/Linux/utils/console/

Not sure if that kind of software still has a chance to work nowadays.

 - Martin Hostettler
Tetsuo Handa Sept. 29, 2020, 1:12 a.m. UTC | #3
On 2020/09/29 2:59, Martin Hostettler wrote:
> On Sun, Sep 27, 2020 at 08:46:30PM +0900, Tetsuo Handa wrote:
>> VT_RESIZEX was introduced in Linux 1.3.3, but it is unclear that what
>> comes to the "+ more" part, and I couldn't find a user of VT_RESIZEX.
>>
> 
> It seems this is/was used by "svgatextmode" which seems to be at
> http://www.ibiblio.org/pub/Linux/utils/console/
> 
> Not sure if that kind of software still has a chance to work nowadays.
> 

Thanks for the information.

It seems that v.v_vlin = curr_textmode->VDisplay / (MOFLG_ISSET(curr_textmode, ATTR_DOUBLESCAN) ? 2 : 1)
and v.v_clin = curr_textmode->FontHeight . Thus, v.v_clin is font's height and seems to be non-zero.
But according to https://bugs.gentoo.org/19485 , people are using kernel framebuffer instead.

---------- SVGATextMode-1.10/SVGATextMode.c ----------

      /*
       * Resize the screen. Still needs LOTS more error checking to avoid dropping out in the middle, leaving
       * the user with a garbled screen.
       *
       * sresize will be TRUE when resizing tty's should be forced (due to the 2nd attempt do_VT_RESIZE will do
       * when not enough memory is free).
       *
       */

        /*
         * ALWAYS do a VT_RESIZE, even if we already did a VT_RESIZEX on a 1.3.3 or higher kernel,
         * until those kernel programmers make this unambiguous
         */

       if (do_VT_RESIZE(curr_textmode->cols, curr_textmode->rows, resize1x1)) sresize=TRUE;

       if (check_kernel_version(1,3,3, "VT_RESIZEX"))
         {
           /*
            * VDisplay must de divided by 2 for DoubleScan modes,
            * or VT_RESIZEX will fail -- until someone fixes the kernel
            * so it understands about doublescan modes.
            */
           if (do_VT_RESIZEX(curr_textmode->cols,
                             curr_textmode->rows,
                             curr_textmode->VDisplay / (MOFLG_ISSET(curr_textmode, ATTR_DOUBLESCAN) ? 2 : 1),
                             curr_textmode->FontHeight,
                             curr_textmode->HDisplay/8*curr_textmode->FontWidth,
                             curr_textmode->FontWidth, resize1x1)) sresize=TRUE;
         }

---------- SVGATextMode-1.10/ttyresize.c ----------

/*
 * if VT_RESIZEX not supported (i.e. when compiling on < 1.3.3 kernels), define it.
 * this is just te keep the compiler happy
 */

#ifndef VT_RESIZEX
#  define VT_RESIZEX  0x560A
   typedef struct vt_consize {
      ushort v_rows; ushort v_cols; ushort v_vlin; ushort v_clin; ushort v_vcol; ushort v_ccol;
    } vt_consize;
#endif


int do_VT_RESIZEX(int cols, int rows, int vlin, int clin, int vcol, int ccol, int allow1x1)
{
  struct vt_consize my_vt_size;      /* passes the new screen size on to the kernel */
  struct vt_consize dummy_vt_size = { 1 , 1 , 1 , 1 , 1 , 1 };
  int ram_needed = cols * rows * 2 * MAX_NR_CONSOLES;

  my_vt_size.v_rows = rows;
  my_vt_size.v_cols = cols;
  my_vt_size.v_vlin = vlin;
  my_vt_size.v_clin = clin;
  my_vt_size.v_vcol = vcol;
  my_vt_size.v_ccol = ccol;

  PDEBUG(("VT_RESIZEX(cols=%d,rows=%d,vlin=%d,clin=%d,vcol=%d,ccol=%d)\n",cols, rows, vlin, clin, vcol, ccol));

  return(generic_VT_RESIZE(&my_vt_size, &dummy_vt_size, allow1x1, ram_needed, VT_RESIZEX, "VT_RESIZEX"));
}
Martin Hostettler Sept. 29, 2020, 10:52 a.m. UTC | #4
On Tue, Sep 29, 2020 at 10:12:46AM +0900, Tetsuo Handa wrote:
> On 2020/09/29 2:59, Martin Hostettler wrote:
> > On Sun, Sep 27, 2020 at 08:46:30PM +0900, Tetsuo Handa wrote:
> >> VT_RESIZEX was introduced in Linux 1.3.3, but it is unclear that what
> >> comes to the "+ more" part, and I couldn't find a user of VT_RESIZEX.
> >>
> > 
> > It seems this is/was used by "svgatextmode" which seems to be at
> > http://www.ibiblio.org/pub/Linux/utils/console/
> > 
> > Not sure if that kind of software still has a chance to work nowadays.
> > 
> 
> Thanks for the information.
> 
> It seems that v.v_vlin = curr_textmode->VDisplay / (MOFLG_ISSET(curr_textmode, ATTR_DOUBLESCAN) ? 2 : 1)
> and v.v_clin = curr_textmode->FontHeight . Thus, v.v_clin is font's height and seems to be non-zero.
> But according to https://bugs.gentoo.org/19485 , people are using kernel framebuffer instead.
> 

Yes, this seems to be from pre framebuffer times.

Back in the days "svga" was the wording you got for "pokes svga card
hardware registers from userspace drivers". And textmode means font
rendering is done via (fixed function in those times) hardware scanout
engine. Of course having only to update 2 bytes per character was a huge
saving early on. Likely this is also before vesa VBE was reliable.

So i guess the point where this all starts going wrong allowing the X parts
of the api to be combined with FB based rendering at all? Sounds the only
user didn't use that combination and so it was never tested?

Then again, this all relates to hardware from 20 years ago...

 - Martin Hostettler
Daniel Vetter Sept. 29, 2020, 4:56 p.m. UTC | #5
On Tue, Sep 29, 2020 at 12:52:03PM +0200, Martin Hostettler wrote:
> On Tue, Sep 29, 2020 at 10:12:46AM +0900, Tetsuo Handa wrote:
> > On 2020/09/29 2:59, Martin Hostettler wrote:
> > > On Sun, Sep 27, 2020 at 08:46:30PM +0900, Tetsuo Handa wrote:
> > >> VT_RESIZEX was introduced in Linux 1.3.3, but it is unclear that what
> > >> comes to the "+ more" part, and I couldn't find a user of VT_RESIZEX.
> > >>
> > > 
> > > It seems this is/was used by "svgatextmode" which seems to be at
> > > http://www.ibiblio.org/pub/Linux/utils/console/
> > > 
> > > Not sure if that kind of software still has a chance to work nowadays.
> > > 
> > 
> > Thanks for the information.
> > 
> > It seems that v.v_vlin = curr_textmode->VDisplay / (MOFLG_ISSET(curr_textmode, ATTR_DOUBLESCAN) ? 2 : 1)
> > and v.v_clin = curr_textmode->FontHeight . Thus, v.v_clin is font's height and seems to be non-zero.
> > But according to https://bugs.gentoo.org/19485 , people are using kernel framebuffer instead.
> > 
> 
> Yes, this seems to be from pre framebuffer times.
> 
> Back in the days "svga" was the wording you got for "pokes svga card
> hardware registers from userspace drivers". And textmode means font
> rendering is done via (fixed function in those times) hardware scanout
> engine. Of course having only to update 2 bytes per character was a huge
> saving early on. Likely this is also before vesa VBE was reliable.
> 
> So i guess the point where this all starts going wrong allowing the X parts
> of the api to be combined with FB based rendering at all? Sounds the only
> user didn't use that combination and so it was never tested?
> 
> Then again, this all relates to hardware from 20 years ago...

Imo userspace modesetting should be burned down anywhere we can. We've
gotten away with this in drivers/gpu by just seamlessly transitioning to
kernel drivers.

Since th only userspace we've found seems to be able to cope if this ioctl
doesn't do anything, my vote goes towards ripping it out completely and
doing nothing in there. Only question is whether we should error or fail
with a silent success: Former is safer, latter can avoid a few regression
reports since the userspace tools keep "working", and usually people don't
notice for stuff this old. It worked in drivers/gpu :-)

Cheers, Daniel
Greg KH Sept. 29, 2020, 5:10 p.m. UTC | #6
On Tue, Sep 29, 2020 at 06:56:57PM +0200, Daniel Vetter wrote:
> On Tue, Sep 29, 2020 at 12:52:03PM +0200, Martin Hostettler wrote:
> > On Tue, Sep 29, 2020 at 10:12:46AM +0900, Tetsuo Handa wrote:
> > > On 2020/09/29 2:59, Martin Hostettler wrote:
> > > > On Sun, Sep 27, 2020 at 08:46:30PM +0900, Tetsuo Handa wrote:
> > > >> VT_RESIZEX was introduced in Linux 1.3.3, but it is unclear that what
> > > >> comes to the "+ more" part, and I couldn't find a user of VT_RESIZEX.
> > > >>
> > > > 
> > > > It seems this is/was used by "svgatextmode" which seems to be at
> > > > http://www.ibiblio.org/pub/Linux/utils/console/
> > > > 
> > > > Not sure if that kind of software still has a chance to work nowadays.
> > > > 
> > > 
> > > Thanks for the information.
> > > 
> > > It seems that v.v_vlin = curr_textmode->VDisplay / (MOFLG_ISSET(curr_textmode, ATTR_DOUBLESCAN) ? 2 : 1)
> > > and v.v_clin = curr_textmode->FontHeight . Thus, v.v_clin is font's height and seems to be non-zero.
> > > But according to https://bugs.gentoo.org/19485 , people are using kernel framebuffer instead.
> > > 
> > 
> > Yes, this seems to be from pre framebuffer times.
> > 
> > Back in the days "svga" was the wording you got for "pokes svga card
> > hardware registers from userspace drivers". And textmode means font
> > rendering is done via (fixed function in those times) hardware scanout
> > engine. Of course having only to update 2 bytes per character was a huge
> > saving early on. Likely this is also before vesa VBE was reliable.
> > 
> > So i guess the point where this all starts going wrong allowing the X parts
> > of the api to be combined with FB based rendering at all? Sounds the only
> > user didn't use that combination and so it was never tested?
> > 
> > Then again, this all relates to hardware from 20 years ago...
> 
> Imo userspace modesetting should be burned down anywhere we can. We've
> gotten away with this in drivers/gpu by just seamlessly transitioning to
> kernel drivers.
> 
> Since th only userspace we've found seems to be able to cope if this ioctl
> doesn't do anything, my vote goes towards ripping it out completely and
> doing nothing in there. Only question is whether we should error or fail
> with a silent success: Former is safer, latter can avoid a few regression
> reports since the userspace tools keep "working", and usually people don't
> notice for stuff this old. It worked in drivers/gpu :-)

This patch just ignores the ioctl and keeps on going, so userspace
"shouldn't" notice it :)

And it's in linux-next now, so all should be good.

thanks,

greg k-h
Maciej W. Rozycki April 11, 2021, 9:43 p.m. UTC | #7
On Tue, 29 Sep 2020, Greg KH wrote:

> > > > >> VT_RESIZEX was introduced in Linux 1.3.3, but it is unclear that what
> > > > >> comes to the "+ more" part, and I couldn't find a user of VT_RESIZEX.
> > > > >>
> > > > > 
> > > > > It seems this is/was used by "svgatextmode" which seems to be at
> > > > > http://www.ibiblio.org/pub/Linux/utils/console/
> > > > > 
> > > > > Not sure if that kind of software still has a chance to work nowadays.
> > > > > 
> > > > 
> > > > Thanks for the information.
> > > > 
> > > > It seems that v.v_vlin = curr_textmode->VDisplay / (MOFLG_ISSET(curr_textmode, ATTR_DOUBLESCAN) ? 2 : 1)
> > > > and v.v_clin = curr_textmode->FontHeight . Thus, v.v_clin is font's height and seems to be non-zero.
> > > > But according to https://bugs.gentoo.org/19485 , people are using kernel framebuffer instead.
> > > > 
> > > 
> > > Yes, this seems to be from pre framebuffer times.
> > > 
> > > Back in the days "svga" was the wording you got for "pokes svga card
> > > hardware registers from userspace drivers". And textmode means font
> > > rendering is done via (fixed function in those times) hardware scanout
> > > engine. Of course having only to update 2 bytes per character was a huge
> > > saving early on. Likely this is also before vesa VBE was reliable.
> > > 
> > > So i guess the point where this all starts going wrong allowing the X parts
> > > of the api to be combined with FB based rendering at all? Sounds the only
> > > user didn't use that combination and so it was never tested?
> > > 
> > > Then again, this all relates to hardware from 20 years ago...
> > 
> > Imo userspace modesetting should be burned down anywhere we can. We've
> > gotten away with this in drivers/gpu by just seamlessly transitioning to
> > kernel drivers.
> > 
> > Since th only userspace we've found seems to be able to cope if this ioctl
> > doesn't do anything, my vote goes towards ripping it out completely and
> > doing nothing in there. Only question is whether we should error or fail
> > with a silent success: Former is safer, latter can avoid a few regression
> > reports since the userspace tools keep "working", and usually people don't
> > notice for stuff this old. It worked in drivers/gpu :-)
> 
> This patch just ignores the ioctl and keeps on going, so userspace
> "shouldn't" notice it :)
> 
> And it's in linux-next now, so all should be good.

 So it does trigger with vgacon and my old server, which I have started 
experimenting with and for a start I have switched to a new kernel for an 
unrelated purpose (now that I have relieved it from all its usual tasks 
except for the last remaining one for which I haven't got the required 
user software ported to the new system yet):

"struct vt_consize"->v_vlin is ignored. Please report if you need this.
"struct vt_consize"->v_clin is ignored. Please report if you need this.

It continues using svgatextmode with its glass (CRT) VT to set my usual 
80x37 text mode (720x576 pixel resolution) by manipulating the VGA clock 
chip and the CRT controller appropriately for a nice refresh rate of 85Hz:

Chipset = `TVGA8900', Textmode clock = 44.90 MHz, 80x37 chars, CharCell = 9x16. Refresh = 52.51kHz/84.7Hz.

 Indeed the piece of software became less usable around Y2K as clock chip 
support stopped being added to svgatextmode for new video adapters, but 
with the advent of LCD technology and its disregard for the refresh rate 
previously driven by the pixel clock the program got its second life and I 
have used it ever since with its plain VGA driver by just manipulating the 
CRTC for the resolution required:

Chipset = `VGA', Textmode clock = 28.32 MHz, 80x37 chars, CharCell = 9x16. Refresh = 31.47kHz/49.0Hz.

(that would still work with a standard 800x600 SVGA CRT, but the refresh 
rate would make anyone's eyes cry soon; with LCD it's just awesome, and 
the VGA emulation of the actual graphics adapter turns it at the video 
output into a 1600x1200 picture at the horizontal and vertical rates of 
75KHz and 60Hz respectively, making the text produced on LCD outstanding 
while showing about the right amount of it).

 But I'm currently ~160km/100mi away from the server I have triggered this 
message with, so I cannot easily check what's going on with its VT.  And I 
can't fiddle with my production laptop (ThinkPad P51) I have with me that 
I also use svgatextmode with so much as to reboot it with a new kernel 
(plain Debian 4.19.16-1~bpo9+1 still here).

 So what's the supposed impact of this change that prompted the inclusion 
of the messages?  I can port svgatextmode (it's my own compilation anyway) 
if that is required for it to continue working correctly, but I need to 
understand the circumstances here.  I have failed to find a satisfactory 
alternative solution to vgacon and svgatextmode; the main showstopper is 
the IBM's hardware trick for a 9x16 character cell that I rely on.

  Maciej
Linus Torvalds April 11, 2021, 10:15 p.m. UTC | #8
On Sun, Apr 11, 2021 at 2:43 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
>  So it does trigger with vgacon and my old server, which I have started
> experimenting with and for a start I have switched to a new kernel for an
> unrelated purpose (now that I have relieved it from all its usual tasks
> except for the last remaining one for which I haven't got the required
> user software ported to the new system yet):
>
> "struct vt_consize"->v_vlin is ignored. Please report if you need this.
> "struct vt_consize"->v_clin is ignored. Please report if you need this.

Note that it's entirely possible that things continue to work well
despite this warning. It's unclear to me from your email if you
actually see any difference (and apparently you're not able to see it
right now due to not being close to the machine).

The fact that v_vlin/v_clin are ignored doesn't necessarily mean that
they are different from what they were before, so the warning may be a
non-issue.

> It continues using svgatextmode with its glass (CRT) VT to set my usual
> 80x37 text mode (720x576 pixel resolution) by manipulating the VGA clock
> chip and the CRT controller appropriately for a nice refresh rate of 85Hz:
>
> Chipset = `TVGA8900', Textmode clock = 44.90 MHz, 80x37 chars, CharCell = 9x16. Refresh = 52.51kHz/84.7Hz.

That doesn't seem necessarily wrong to me.

>  So what's the supposed impact of this change that prompted the inclusion
> of the messages?

There _may_ be no impact at all apart from the messages.

The code _used_ to set the scan lines (v_vlin) and font height
(v_clin) from those numbers if they were non-zero, and now it just
ignores them and warns instead.

The code now just sets the font height from the actual font size when
the font is set. Which is honestly the only thing that ever made
sense. Trying to set it with v_clin is ignored, but it's entirely
possible - maybe even likely - that your user of VT_RESIZEX sets it to
the same values it already has.

Exactly because setting a font line number to anything else than the
font size isn't exactly sensible.

But if your screen looks different than it used to, that is obviously
interesting and says something actually changed (outside of the
message itself).

           Linus
Daniel Vetter April 12, 2021, 7:01 a.m. UTC | #9
On Mon, Apr 12, 2021 at 12:15 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, Apr 11, 2021 at 2:43 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> >
> >  So it does trigger with vgacon and my old server, which I have started
> > experimenting with and for a start I have switched to a new kernel for an
> > unrelated purpose (now that I have relieved it from all its usual tasks
> > except for the last remaining one for which I haven't got the required
> > user software ported to the new system yet):
> >
> > "struct vt_consize"->v_vlin is ignored. Please report if you need this.
> > "struct vt_consize"->v_clin is ignored. Please report if you need this.
>
> Note that it's entirely possible that things continue to work well
> despite this warning. It's unclear to me from your email if you
> actually see any difference (and apparently you're not able to see it
> right now due to not being close to the machine).

Original search didn't turn up any users of VT_RESIZEX, this is the
first. And looking at the source code I think we could outright remove
support for VT_RESIZEX (but make it silent) and everything should keep
working:

        /*
         * ALWAYS do a VT_RESIZE, even if we already did a VT_RESIZEX
on a 1.3.3 or higher kernel,
         * until those kernel programmers make this unambiguous
         */

       if (do_VT_RESIZE(curr_textmode->cols, curr_textmode->rows,
resize1x1)) sresize=TRUE;

       if (check_kernel_version(1,3,3, "VT_RESIZEX"))
         {
           /*
            * VDisplay must de divided by 2 for DoubleScan modes,
            * or VT_RESIZEX will fail -- until someone fixes the kernel
            * so it understands about doublescan modes.
            */
           if (do_VT_RESIZEX(curr_textmode->cols,
                             curr_textmode->rows,
                             curr_textmode->VDisplay /
(MOFLG_ISSET(curr_textmode, ATTR_DOUBLESCAN) ? 2 : 1),
                             curr_textmode->FontHeight,
                             curr_textmode->HDisplay/8*curr_textmode->FontWidth,
                             curr_textmode->FontWidth, resize1x1)) sresize=TRUE;
         }

The functions are just straightforward wrappers. There's also no cvs
repo, changelog or old releases before 2000 that would shed some light
on why this code even exists.

I think we can just tune down the pr_info_once to pr_debug and done.
Maybe a comment about where the single user we're aware of is.
-Daniel

>
> The fact that v_vlin/v_clin are ignored doesn't necessarily mean that
> they are different from what they were before, so the warning may be a
> non-issue.
>
> > It continues using svgatextmode with its glass (CRT) VT to set my usual
> > 80x37 text mode (720x576 pixel resolution) by manipulating the VGA clock
> > chip and the CRT controller appropriately for a nice refresh rate of 85Hz:
> >
> > Chipset = `TVGA8900', Textmode clock = 44.90 MHz, 80x37 chars, CharCell = 9x16. Refresh = 52.51kHz/84.7Hz.
>
> That doesn't seem necessarily wrong to me.
>
> >  So what's the supposed impact of this change that prompted the inclusion
> > of the messages?
>
> There _may_ be no impact at all apart from the messages.
>
> The code _used_ to set the scan lines (v_vlin) and font height
> (v_clin) from those numbers if they were non-zero, and now it just
> ignores them and warns instead.
>
> The code now just sets the font height from the actual font size when
> the font is set. Which is honestly the only thing that ever made
> sense. Trying to set it with v_clin is ignored, but it's entirely
> possible - maybe even likely - that your user of VT_RESIZEX sets it to
> the same values it already has.
>
> Exactly because setting a font line number to anything else than the
> font size isn't exactly sensible.
>
> But if your screen looks different than it used to, that is obviously
> interesting and says something actually changed (outside of the
> message itself).
>
>            Linus
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Maciej W. Rozycki April 12, 2021, 1:30 p.m. UTC | #10
On Mon, 12 Apr 2021, Daniel Vetter wrote:

> > Note that it's entirely possible that things continue to work well
> > despite this warning. It's unclear to me from your email if you
> > actually see any difference (and apparently you're not able to see it
> > right now due to not being close to the machine).
> 
> Original search didn't turn up any users of VT_RESIZEX, this is the
> first. And looking at the source code I think we could outright remove
> support for VT_RESIZEX (but make it silent) and everything should keep
> working:
> 
>         /*
>          * ALWAYS do a VT_RESIZE, even if we already did a VT_RESIZEX
> on a 1.3.3 or higher kernel,
>          * until those kernel programmers make this unambiguous
>          */
> 
>        if (do_VT_RESIZE(curr_textmode->cols, curr_textmode->rows,
> resize1x1)) sresize=TRUE;
> 
>        if (check_kernel_version(1,3,3, "VT_RESIZEX"))
>          {
>            /*
>             * VDisplay must de divided by 2 for DoubleScan modes,
>             * or VT_RESIZEX will fail -- until someone fixes the kernel
>             * so it understands about doublescan modes.
>             */
>            if (do_VT_RESIZEX(curr_textmode->cols,
>                              curr_textmode->rows,
>                              curr_textmode->VDisplay /
> (MOFLG_ISSET(curr_textmode, ATTR_DOUBLESCAN) ? 2 : 1),
>                              curr_textmode->FontHeight,
>                              curr_textmode->HDisplay/8*curr_textmode->FontWidth,
>                              curr_textmode->FontWidth, resize1x1)) sresize=TRUE;
>          }
> 
> The functions are just straightforward wrappers. There's also no cvs
> repo, changelog or old releases before 2000 that would shed some light
> on why this code even exists.

 I did some archaeology then, using a local copy of the linux-mips.org 
Linux tree that has historic information imported from the old oss.sgi.com 
MIPS/Linux CVS repo.  According to that the ioctl was added with or 
shortly before 2.1.14:

commit beb116954b9b7f3bb56412b2494b562f02b864b1
Author: Ralf Baechle <ralf@linux-mips.org>
Date:   Tue Jan 7 02:33:00 1997 +0000

    Import of Linux/MIPS 2.1.14

and, importantly, it was used to set some parameters: 

		if ( vlin )
		  video_scan_lines = vlin;
		if ( clin )
		  video_font_height = clin;

used by `con_adjust_height' in drivers/char/vga.c: `video_scan_lines' to 
set the vertical display limit (so that it is a whole multiple of the new 
font height) and `video_font_height' to set the cursor scan lines in the 
CRTC.  The function was used by the PIO_FONTX and PIO_FONTRESET VT ioctls 
at that point.

 That code was moved to `vgacon_adjust_height' in drivers/video/vgacon.c 
and then drivers/video/console/vgacon.c.  The code is still there, serving 
the KDFONTOP ioctl.

 With:

commit 9736a3546de7b6a2b16ad93539e4b3ac72b385bb
Author: Ralf Baechle <ralf@linux-mips.org>
Date:   Thu Jun 5 10:06:35 2003 +0000

    Merge with Linux 2.5.66.

the parameters were moved into `struct vc_data':

 		if (vlin)
-			video_scan_lines = vlin;
+			vc->vc_scan_lines = vlin;
 		if (clin)
-			video_font_height = clin;
+			vc->vc_font.height = clin;

and this piece of code to set them only removed with the change discussed 
here.

 So without even looking at the VT, which I'll surely get to eventually, I 
conclude this change regresses font resizing (KD_FONT_OP_SET) once a new 
resolution has been set with svgatextmode.  I think this change needs to 
be reverted, especially as the problematic PIO_FONT ioctl referred has 
been since removed with commit ff2047fb755d ("vt: drop old FONT ioctls").

  Maciej
diff mbox series

Patch

diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index a4e520bdd521..bc33938e2f20 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -773,58 +773,21 @@  static int vt_resizex(struct vc_data *vc, struct vt_consize __user *cs)
 	if (copy_from_user(&v, cs, sizeof(struct vt_consize)))
 		return -EFAULT;
 
-	/* FIXME: Should check the copies properly */
-	if (!v.v_vlin)
-		v.v_vlin = vc->vc_scan_lines;
-
-	if (v.v_clin) {
-		int rows = v.v_vlin / v.v_clin;
-		if (v.v_rows != rows) {
-			if (v.v_rows) /* Parameters don't add up */
-				return -EINVAL;
-			v.v_rows = rows;
-		}
-	}
-
-	if (v.v_vcol && v.v_ccol) {
-		int cols = v.v_vcol / v.v_ccol;
-		if (v.v_cols != cols) {
-			if (v.v_cols)
-				return -EINVAL;
-			v.v_cols = cols;
-		}
-	}
-
-	if (v.v_clin > 32)
-		return -EINVAL;
+	if (v.v_vlin)
+		pr_info_once("\"struct vt_consize\"->v_vlin is ignored. Please report if you need this.\n");
+	if (v.v_clin)
+		pr_info_once("\"struct vt_consize\"->v_clin is ignored. Please report if you need this.\n");
 
+	console_lock();
 	for (i = 0; i < MAX_NR_CONSOLES; i++) {
-		struct vc_data *vcp;
+		vc = vc_cons[i].d;
 
-		if (!vc_cons[i].d)
-			continue;
-		console_lock();
-		vcp = vc_cons[i].d;
-		if (vcp) {
-			int ret;
-			int save_scan_lines = vcp->vc_scan_lines;
-			int save_font_height = vcp->vc_font.height;
-
-			if (v.v_vlin)
-				vcp->vc_scan_lines = v.v_vlin;
-			if (v.v_clin)
-				vcp->vc_font.height = v.v_clin;
-			vcp->vc_resize_user = 1;
-			ret = vc_resize(vcp, v.v_cols, v.v_rows);
-			if (ret) {
-				vcp->vc_scan_lines = save_scan_lines;
-				vcp->vc_font.height = save_font_height;
-				console_unlock();
-				return ret;
-			}
+		if (vc) {
+			vc->vc_resize_user = 1;
+			vc_resize(vc, v.v_cols, v.v_rows);
 		}
-		console_unlock();
 	}
+	console_unlock();
 
 	return 0;
 }