linux-kernel - Re: [PATCH] drm/vc4: hdmi: Fix pointer dereference before check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 2 Nov 2022 12:10:03 +0100
From:   José Expósito <jose.exposito89@...il.com>
To:     Maxime Ripard <maxime@...no.tech>
Cc:     emma@...olt.net, airlied@...il.com, daniel@...ll.ch,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/vc4: hdmi: Fix pointer dereference before check

Hi Maxime,

Thanks a lot for looking into the patch.

On Wed, Nov 02, 2022 at 10:01:53AM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Sat, Oct 29, 2022 at 11:34:13AM +0200, José Expósito wrote:
> > Commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") introduced
> > the vc4_hdmi_reset_link() function. This function dereferences the
> > "connector" pointer before checking whether it is NULL or not.
> > 
> > Rework variable assignment to avoid this issue.
> > 
> > Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
> > Signed-off-by: José Expósito <jose.exposito89@...il.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index 4a73fafca51b..07d058b6afb7 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -319,9 +319,9 @@ static int reset_pipe(struct drm_crtc *crtc,
> >  static int vc4_hdmi_reset_link(struct drm_connector *connector,
> >  			       struct drm_modeset_acquire_ctx *ctx)
> >  {
> > -	struct drm_device *drm = connector->dev;
> > -	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> > -	struct drm_encoder *encoder = &vc4_hdmi->encoder.base;
> > +	struct drm_device *drm;
> > +	struct vc4_hdmi *vc4_hdmi;
> > +	struct drm_encoder *encoder;
> >  	struct drm_connector_state *conn_state;
> >  	struct drm_crtc_state *crtc_state;
> >  	struct drm_crtc *crtc;
> > @@ -332,6 +332,10 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector,
> >  	if (!connector)
> >  		return 0;
> >  
> > +	drm = connector->dev;
> > +	vc4_hdmi = connector_to_vc4_hdmi(connector);
> > +	encoder = &vc4_hdmi->encoder.base;
> > +
> 
> I don't think that's right. Connector shouldn't be NULL to begin with,
> how did you notice this?
> 
> Maxime

This issue was reported by Coverity. At the moment this function is not
invoked with a NULL connector by any code path. However, since the NULL
check is present, in my opinion, it makes sense to either remove it or
make it usefull just in case the preconditions change in the future.

But at the moment, this is not a big deal.

Thanks,
Jose