Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Nov 2022 12:10:03 +0100
From:   =?iso-8859-1?Q?Jos=E9_Exp=F3sito?= <jose.exposito89@gmail.com>
To:     Maxime Ripard <maxime@cerno.tech>
Cc:     emma@anholt.net, airlied@gmail.com, daniel@ffwll.ch,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/vc4: hdmi: Fix pointer dereference before check
Message-ID: <20221102111003.GA3233@elementary>
References: <20221029093413.546103-1-jose.exposito89@gmail.com>
 <20221102090153.tujblkvd3jlhdtxr@houat>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20221102090153.tujblkvd3jlhdtxr@houat>
Precedence: bulk

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@gmail.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