| 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 |
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
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
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")); }
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
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
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
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
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
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
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 --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; }
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(-)