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