Re: connector_bad_edid() is broken (was: Re: [PATCH] drm/edid: Fix crash with zero/invalid EDID)

[Doug Anderson]

Hi,

On Tue, Oct 5, 2021 at 6:33 AM Zuo, Jerry <Jerry.Zuo@xxxxxxx> wrote:
>
> > 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...
> >
> [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

I've cut out other bits from this email and changed the subject line
since I think this is an issue unrelated to the one my original patch
was fixing.

I don't actually know a ton about DP compliance testing, but I
attempted to try to be helpful and revert commit e11f5bd8228f ("drm:
Add support for DP 1.4 Compliance edid corruption test"). It wasn't
too hard to deal with the conflicts in the revert itself, but then
things didn't compile because there are two places that use
`real_edid_checksum` and that goes away if I revert the patch.

I've made an attempt to fix the problem by just adding a bounds check.
Perhaps you can see if that looks good to you:

https://lore.kernel.org/r/20211005081022.1.Ib059f9c23c2611cb5a9d760e7d0a700c1295928d@changeid

-Doug