RE: [PATCH] drm/edid: Fix crash with zero/invalid EDID

[Zuo, Jerry]

[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@xxxxxxxxxxxxxxx>
> Sent: October 4, 2021 3:45 PM
> To: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; geert@xxxxxxxxxxxxxx;
> oliver.sang@xxxxxxxxx; Daniel Vetter <daniel@xxxxxxxx>; David Airlie
> <airlied@xxxxxxxx>; Jani Nikula <jani.nikula@xxxxxxxxx>; Linus Walleij
> <linus.walleij@xxxxxxxxxx>; Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard
> <mripard@xxxxxxxxxx>; Sam Ravnborg <sam@xxxxxxxxxxxx>; Thomas
> Zimmermann <tzimmermann@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; Zuo,
> Jerry <Jerry.Zuo@xxxxxxx>; Wentland, Harry <Harry.Wentland@xxxxxxx>;
> Siqueira, Rodrigo <Rodrigo.Siqueira@xxxxxxx>
> 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@xxxxxxxxx>
> > Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > ---
> >
> >  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