Date:   Tue, 5 Oct 2021 13:33:23 +0000
From:   "Zuo, Jerry" <Jerry.Zuo@....com>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        Douglas Anderson <dianders@...omium.org>
CC:     "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "geert@...ux-m68k.org" <geert@...ux-m68k.org>,
        "oliver.sang@...el.com" <oliver.sang@...el.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Jani Nikula <jani.nikula@...el.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Wentland, Harry" <Harry.Wentland@....com>,
        "Siqueira, Rodrigo" <Rodrigo.Siqueira@....com>
Subject: RE: [PATCH] drm/edid: Fix crash with zero/invalid EDID

[AMD Official Use Only]

Hi Ville:

     Yhea, it is pretty old change from two years ago, and it is no long valid anymore. Please simply drop it.

Regards,
Jerry

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@...ux.intel.com>
> Sent: October 4, 2021 3:45 PM
> To: Douglas Anderson <dianders@...omium.org>
> Cc: dri-devel@...ts.freedesktop.org; geert@...ux-m68k.org;
> oliver.sang@...el.com; Daniel Vetter <daniel@...ll.ch>; David Airlie
> <airlied@...ux.ie>; Jani Nikula <jani.nikula@...el.com>; Linus Walleij
> <linus.walleij@...aro.org>; Maarten Lankhorst
> <maarten.lankhorst@...ux.intel.com>; Maxime Ripard
> <mripard@...nel.org>; Sam Ravnborg <sam@...nborg.org>; Thomas
> Zimmermann <tzimmermann@...e.de>; linux-kernel@...r.kernel.org; Zuo,
> Jerry <Jerry.Zuo@....com>; Wentland, Harry <Harry.Wentland@....com>;
> Siqueira, Rodrigo <Rodrigo.Siqueira@....com>
> Subject: Re: [PATCH] drm/edid: Fix crash with zero/invalid EDID
>
> On Mon, Oct 04, 2021 at 09:21:27AM -0700, Douglas Anderson wrote:
> > In the commit bac9c2948224 ("drm/edid: Break out reading block 0 of
> > the EDID") I broke out reading the base block of the EDID to its own
> > function. Unfortunately, when I did that I messed up the handling when
> > drm_edid_is_zero() indicated that we had an EDID that was all 0x00 or
> > when we went through 4 loops and didn't get a valid EDID. Specifically
> > I needed to pass the broken EDID to connector_bad_edid() but now I was
> > passing an error-pointer.
> >
> > Let's re-jigger things so we can pass the bad EDID in properly.
> >
> > Fixes: bac9c2948224 ("drm/edid: Break out reading block 0 of the
> > EDID")
> > Reported-by: kernel test robot <oliver.sang@...el.com>
> > Reported-by: Geert Uytterhoeven <geert@...ux-m68k.org>
> > Signed-off-by: Douglas Anderson <dianders@...omium.org>
> > ---
> >
> >  drivers/gpu/drm/drm_edid.c | 27 +++++++++++----------------
> >  1 file changed, 11 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 9b19eee0e1b4..9c9463ec5465 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -1911,13 +1911,15 @@ int drm_add_override_edid_modes(struct
> > drm_connector *connector)  }
> > EXPORT_SYMBOL(drm_add_override_edid_modes);
> >
> > -static struct edid *drm_do_get_edid_base_block(
> > +static struct edid *drm_do_get_edid_base_block(struct drm_connector
> > +*connector,
> >     int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
> >                           size_t len),
> > -   void *data, bool *edid_corrupt, int *null_edid_counter)
> > +   void *data)
> >  {
> > -   int i;
> > +   int *null_edid_counter = connector ? &connector-
> >null_edid_counter : NULL;
> > +   bool *edid_corrupt = connector ? &connector->edid_corrupt : NULL;
> >     void *edid;
> > +   int i;
> >
> >     edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
> >     if (edid == NULL)
> > @@ -1941,9 +1943,8 @@ static struct edid
> *drm_do_get_edid_base_block(
> >     return edid;
> >
> >  carp:
> > -   kfree(edid);
> > -   return ERR_PTR(-EINVAL);
> > -
> > +   if (connector)
> > +           connector_bad_edid(connector, edid, 1);
>
> BTW I believe connector_bad_edid() itself is broken since commit
> e11f5bd8228f ("drm: Add support for DP 1.4 Compliance edid corruption
> test"). Before we've even allocated the memory for the extension blocks
> that code now assumes edid[0x7e] is to be 100% trusted and goes and
> calculates the checksum on a block based on that. So that's likely going to be
> pointing somewhere beyond the base block into memory we've not even
> allocated. So anyone who wanted could craft a bogus EDID and maybe get
> something interesting to happen.
>
> Would be good if someone could fix that while at it. Or just revert the
> offending commit if there is no simple solution immediately in sight.
>
> The fact that we're parsing entirely untrustworthy crap in the kernel always
> worries me. Either we need super careful review of all relevant code, and/or
> we need to think about moving the parser out of the kernel.
> I was considering playing around with the usermode helper stuff. IIRC there
> is a way to embed the userspace binary into the kernel and just fire it up
> when needed. But so far it's been the usual -ENOTIME for me...
>
> --
> Ville Syrjälä
> Intel

Powered by blists - more mailing lists