Re: stack buffer overflow in fbdev

[Daniel Vetter <daniel () ffwll ch>]

On Sun, Jul 21, 2019 at 11:03:01AM -0700, Linus Torvalds wrote:
> Completely untested patch attached. There are probably better ways to do this.
>
> Adding the proper people to the cc, and quoting Tavis' email in its entirety.
>
> Daniel - you got added despite not being explicitly listed as
> maintainer because you've touched fbdev/core/ more than most lately,
> plus you know edid anyway. As such: "tag, you're it, sucker".

Yeah I also realized with regrets that get_maintainers thinks I'm
responsible for fbdev core :-/

Wrt the bug: I had a multi-paragraph explanation here about how fbmon.c
edid parser is only used by old crap drivers, and not when you have a
drm-kms driver providing the fbdev emulation (like pretty much every
modern system). Also that the version in fbmon.c seriously lacks compared
to the drm_edid.c one.

And then I ran grep and noticed it's dead code. The last user disappeared
in 34280340b1dc ("fbdev: Remove unused SH-Mobile HDMI driver") from 2015.
I'll type a patch for 5.4 to remove this outright.

Cheers, Daniel

PS: git log -G disappoints by not using all the cores I have here ..

>                 Linus
>
> On Sat, Jul 20, 2019 at 5:35 PM Tavis Ormandy <taviso () gmail com> wrote:
> > Hello, during a conversation on twitter we noticed a stack buffer
> > overflow in fbdev with malicious edid data:
> >
> > https://github.com/torvalds/linux/blob/22051d9c4a57d3b4a8b5a7407efc80c71c7bfb16/drivers/video/fbdev/core/fbmon.c#L1033
> >
> > There is enough space to have 52 1-byte length values, which makes svd_n
> > 52, then make the final value length 0x1f (the maximum), which makes
> > svd_n 83 and overflows the 64 byte stack buffer svd[] with controlled
> > data.
> >
> > This requires a malicious monitor / projector / etc, so pretty low impact.
> >
> > I pulled out the code to make a demo (I removed the checksum, but it
> > doesnt prevent the bug):
> >
> > https://gist.github.com/taviso/923776e633cb8fb1ab847cce761a0f10
> >
> > This was discovered by Nico Waisman of Semmle.
> >
> > Tavis.
> >
> > --
> > -------------------------------------
> > taviso () sdf lonestar org | finger me for my pgp key.
> > -------------------------------------------------------

>  drivers/video/fbdev/core/fbmon.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
> index 3558a70a6664..2ab1fd6e33b7 100644
> --- a/drivers/video/fbdev/core/fbmon.c
> +++ b/drivers/video/fbdev/core/fbmon.c
> @@ -1030,7 +1030,9 @@ void fb_edid_add_monspecs(unsigned char *edid, struct fb_monspecs *specs)
>               if (type == 2) {
>                       for (i = pos; i < pos + len; i++) {
>                               u8 idx = edid[pos + i] & 0x7f;
> -                             svd[svd_n++] = idx;
> +                             if (svd_n < sizeof(svd))
> +                                     svd[svd_n] = idx;
> +                             svd_n++;
>                               pr_debug("N%sative mode #%d\n",
>                                        edid[pos + i] & 0x80 ? "" : "on-n", idx);
>                       }
> @@ -1044,6 +1046,10 @@ void fb_edid_add_monspecs(unsigned char *edid, struct fb_monspecs *specs)
>               pos += len + 1;
>       }
>  
> +     /* Evil monitor? */
> +     if (WARN_ON_ONCE(svd_n > sizeof(svd)))
> +             return;
> +
>       block = edid + edid[2];
>  
>       DPRINTK("  Extended Detailed Timings\n");


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