drm/vc4: hdmi: Fix pointer dereference before check

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

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
  

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
  

On Wed, Nov 02, 2022 at 12:10:03PM +0100, José Expósito wrote:
> 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.

Yeah, it makes sense

I'd ask for a small cosmetic change then, could you add the assignments
where they are actually needed instead of at the top of the function?

Something like

 if (!connector)
 	return 0;

+drm = connector->dev;
 ret = drm_modeset_lock(&drm->mode_config.connection_mutex, ctx);

...

+vc4_hdmi = connector_to_vc4_hdmi(connector);
 if (!vc4_hdmi_supports_scrambling(vc4_hdmi))

...

Changing the prototype of vc4_hdmi_supports_scrambling to take a struct
vc4_hdmi pointer would also help, it's much more convenient.

Maxime
  

On Mon, Nov 07, 2022 at 09:26:30AM +0100, Maxime Ripard wrote:
> On Wed, Nov 02, 2022 at 12:10:03PM +0100, José Expósito wrote:
> > 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.
> 
> Yeah, it makes sense
> 
> I'd ask for a small cosmetic change then, could you add the assignments
> where they are actually needed instead of at the top of the function?
> 
> Something like
> 
>  if (!connector)
>  	return 0;

Dunno why you want to keep around dead code like that.
I'd just nuke the bogus null check.

> 
> +drm = connector->dev;
>  ret = drm_modeset_lock(&drm->mode_config.connection_mutex, ctx);
> 
> ...
> 
> +vc4_hdmi = connector_to_vc4_hdmi(connector);
>  if (!vc4_hdmi_supports_scrambling(vc4_hdmi))
> 
> ...
> 
> Changing the prototype of vc4_hdmi_supports_scrambling to take a struct
> vc4_hdmi pointer would also help, it's much more convenient.
> 
> Maxime