Re: [PATCH] video: fbdev: vga16fb: fix OOB write in vga16fb_imageblit()

From: Daniel Vetter <daniel@ffwll.ch>
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>,
	"Maciej W. Rozycki" <macro@orcam.me.uk>
Subject: Re: [PATCH] video: fbdev: vga16fb: fix OOB write in vga16fb_imageblit()
Date: Mon, 17 May 2021 15:07:35 +0200	[thread overview]
Message-ID: <CAKMK7uGLP2zn7LX4ATExA4DLo16shVivSd_W58X-rBZNPSb3_w@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wioOHwKNj8AmvXWV-oL60ae0jKswAHy9e6wCYYeA5EQXg@mail.gmail.com>

On Fri, May 14, 2021 at 10:33 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, May 14, 2021 at 1:25 PM Maciej W. Rozycki <macro@orcam.me.uk> 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".
>
> 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,

Just an aside: I think with fbdev drivers this would go boom, because
you'd have fbcon interferring with a direct /dev/fb/* user.

But if your fbdev driver is actually a drm modeset driver, then we
have additional limitations: If the userspace accesses the display
through /dev/dri/card0, then the kernel blocks all access through
/dev/fb/* (including fbcon) to the actual display (it only goes into
the buffer used for fbdev emulation). And everything would be fine.

Also generally you'd get away with this even in problematic cases,
since usually you resize your console when looking at it, not when X
or something else is using your fbdev direct access.

The one thing that's left out here a bit in the cold is userspace
modeset drivers in X. Those would get hosed. But also, we stopped
supporting those in at least i915/amd/radeon/nouveau drivers,
automatically falling back to the fbdev stuff in most cases (with or
without the drm drivers underneath that), and no one screamed. So
probably not many users left.

So I /think/ we could wager this, if it's the least intrusive fix from
the kernel pov. But it has some risks that we need to revert again if
we break some of the really old use-cases here.

Cheers, Daniel

> 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.
>
> But no, your patch to just "knowingly use entirely wrong values, then
> add a limit check because we know the values are possibly garbage and
> not consistent with reality" is simply not acceptable.
>
>               Linus

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch