From: "Maciej W. Rozycki" <macro@orcam.me.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linux Fbdev development list <linux-fbdev@vger.kernel.org>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
Jani Nikula <jani.nikula@intel.com>,
Colin King <colin.king@canonical.com>,
Jiri Slaby <jirislaby@kernel.org>,
syzbot <syzbot+1f29e126cf461c4de3b3@syzkaller.appspotmail.com>
Subject: Re: [PATCH] video: fbdev: vga16fb: fix OOB write in vga16fb_imageblit()
Date: Sat, 15 May 2021 18:11:46 +0200 (CEST) [thread overview]
Message-ID: <alpine.DEB.2.21.2105151733090.3032@angie.orcam.me.uk> (raw)
In-Reply-To: <CAHk-=wioOHwKNj8AmvXWV-oL60ae0jKswAHy9e6wCYYeA5EQXg@mail.gmail.com>
On Fri, 14 May 2021, Linus Torvalds wrote:
> > Overall I think it does make sense to resize the text console at any
> > time, even if the visible console (VT) chosen is in the graphics mode,
>
> It might make sense, but only if we call the function to update the
> low-level data.
>
> Not calling it, and then starting to randomly use the (wrong)
> geometry, and just limiting it so that it's all within the buffer -
> THAT does not make sense.
>
> So I think your patch is fundamentally wrong. It basically says "let's
> use random stale incorrect data, but just make sure that the end
> result is still within the allocated buffer".
I guess you mean Tetsuo-san's patch, right? I haven't sent any in this
discussion.
> My patch is at least conceptually sane.
>
> An alternative would be to just remove the "vcmode != KD_GRAPHICS"
> check entirely, and always call con_resize() to update the low-level
> data, but honestly, that seems very likelty to break something very
> fundamentally, since it's not how any of fbcon has ever been tested,
Umm, there isn't much to change as far as console data structures are
concerned with a resize: obviously the width and the height, which affect
the size of the character/attribute buffer, and maybe some cursor data
such as the size and screen coordinates.
For vgacon we have:
if (con_is_visible(c) && !vga_is_gfx) /* who knows */
vgacon_doresize(c, width, height);
in `vgacon_resize' already, following all the sanity checks, so the CRTC
isn't poked at if `vga_is_gfx', exactly as we want.
I can see fbcon does not have equivalent code and instead has relied on
the KD_GRAPHICS check made by the caller. Which I think has been a bug
since fbcon's inception. Instead I think `fbcon_resize' ought to make all
the sanity checks I can see it does and only then check for KD_GRAPHICS
and if so, then exit without poking at hardware. Then upon exit from the
gfx mode the `fb_set_var' call made from `fbcon_blank' will DTRT.
I can try verifying the latter hypothesis, though my framebuffer setups
(with DECstation hardware) have always been somewhat incomplete. I do
believe I have a MIPS fbdev X server binary somewhere to fiddle with,
which should work with that TGA/SFB+ video adapter I mentioned before.
> Another alternative would be to just delay the resize to when vcmode
> is put back to text mode again. That sounds somewhat reasonable to me,
> but it's a pretty big thing.
Methinks it works exactly like that already. On exit from the graphics
mode (a VT switch or gfx program termination) hardware is reprogrammed
according to the console geometry previously set. We just must not break
it.
Maciej
next prev parent reply other threads:[~2021-05-15 16:11 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-08 7:07 BUG: unable to handle kernel paging request in vga16fb_imageblit (2) syzbot
2021-05-01 20:31 ` [syzbot] " syzbot
2021-05-02 1:53 ` syzbot
2021-05-03 13:41 ` Tetsuo Handa
2021-05-07 11:09 ` Tetsuo Handa
2021-05-14 16:19 ` [PATCH] video: fbdev: vga16fb: fix OOB write in vga16fb_imageblit() Tetsuo Handa
2021-05-14 17:29 ` Linus Torvalds
2021-05-14 17:37 ` Linus Torvalds
2021-05-14 18:23 ` Linus Torvalds
2021-05-14 20:25 ` Maciej W. Rozycki
2021-05-14 20:32 ` Linus Torvalds
2021-05-14 21:10 ` Linus Torvalds
2021-05-15 7:43 ` [PATCH v2] tty: vt: always invoke vc->vc_sw->con_resize callback Tetsuo Handa
2021-05-15 16:21 ` Maciej W. Rozycki
2021-05-15 16:32 ` Maciej W. Rozycki
2021-05-15 16:41 ` Linus Torvalds
2021-05-17 13:13 ` Daniel Vetter
2021-05-15 16:11 ` Maciej W. Rozycki [this message]
2021-05-17 13:07 ` [PATCH] video: fbdev: vga16fb: fix OOB write in vga16fb_imageblit() Daniel Vetter
2021-05-17 13:10 ` Daniel Vetter
2021-05-15 0:45 ` Tetsuo Handa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.21.2105151733090.3032@angie.orcam.me.uk \
--to=macro@orcam.me.uk \
--cc=b.zolnierkie@samsung.com \
--cc=colin.king@canonical.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=jani.nikula@intel.com \
--cc=jirislaby@kernel.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=syzbot+1f29e126cf461c4de3b3@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).