LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Zuo, Jerry" <Jerry.Zuo@amd.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Douglas Anderson" <dianders@chromium.org>
Cc: "dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"geert@linux-m68k.org" <geert@linux-m68k.org>,
	"oliver.sang@intel.com" <oliver.sang@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	Jani Nikula <jani.nikula@intel.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Sam Ravnborg <sam@ravnborg.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Wentland, Harry" <Harry.Wentland@amd.com>,
	"Siqueira, Rodrigo" <Rodrigo.Siqueira@amd.com>
Subject: RE: [PATCH] drm/edid: Fix crash with zero/invalid EDID
Date: Tue, 5 Oct 2021 13:33:23 +0000	[thread overview]
Message-ID: <DM6PR12MB49127B8B63079E6533197EA6E5AF9@DM6PR12MB4912.namprd12.prod.outlook.com> (raw)
In-Reply-To: <YVtZstInQxXfPmsZ@intel.com>

[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@linux.intel.com>
> Sent: October 4, 2021 3:45 PM
> To: Douglas Anderson <dianders@chromium.org>
> Cc: dri-devel@lists.freedesktop.org; geert@linux-m68k.org;
> oliver.sang@intel.com; Daniel Vetter <daniel@ffwll.ch>; David Airlie
> <airlied@linux.ie>; Jani Nikula <jani.nikula@intel.com>; Linus Walleij
> <linus.walleij@linaro.org>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> <mripard@kernel.org>; Sam Ravnborg <sam@ravnborg.org>; Thomas
> Zimmermann <tzimmermann@suse.de>; linux-kernel@vger.kernel.org; Zuo,
> Jerry <Jerry.Zuo@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>;
> Siqueira, Rodrigo <Rodrigo.Siqueira@amd.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@intel.com>
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.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

  reply	other threads:[~2021-10-05 13:33 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-04 16:21 Douglas Anderson
2021-10-04 17:14 ` Geert Uytterhoeven
2021-10-05  0:40   ` Doug Anderson
2021-10-05 13:45     ` Doug Anderson
2021-10-04 19:44 ` Ville Syrjälä
2021-10-05 13:33   ` Zuo, Jerry [this message]
2021-10-05 15:13     ` connector_bad_edid() is broken (was: Re: [PATCH] drm/edid: Fix crash with zero/invalid EDID) Doug Anderson
2021-10-05 15:25       ` Zuo, Jerry
2021-10-05 18:03         ` Harry Wentland
2021-10-06 12:05           ` Zuo, Jerry
2021-10-05 16:43 ` [PATCH] drm/edid: Fix crash with zero/invalid EDID Ville Syrjälä
2021-10-06  2:20   ` Doug Anderson

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=DM6PR12MB49127B8B63079E6533197EA6E5AF9@DM6PR12MB4912.namprd12.prod.outlook.com \
    --to=jerry.zuo@amd.com \
    --cc=Harry.Wentland@amd.com \
    --cc=Rodrigo.Siqueira@amd.com \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=geert@linux-m68k.org \
    --cc=jani.nikula@intel.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mripard@kernel.org \
    --cc=oliver.sang@intel.com \
    --cc=sam@ravnborg.org \
    --cc=tzimmermann@suse.de \
    --cc=ville.syrjala@linux.intel.com \
    --subject='RE: [PATCH] drm/edid: Fix crash with zero/invalid EDID' \
    /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

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).