linux-kernel - Re: [PATCH] gpu: drm: i915: display: Avoid null values intel_plane_atomic_check_with

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]

Message-ID: <ZvaduhDERL-zvED3@intel.com>
Date: Fri, 27 Sep 2024 14:57:46 +0300
From: Ville Syrjälä <ville.syrjala@...ux.intel.com>
To: Jani Nikula <jani.nikula@...ux.intel.com>
Cc: Alessandro Zanni <alessandro.zanni87@...il.com>, rodrigo.vivi@...el.com,
	joonas.lahtinen@...ux.intel.com, tursulin@...ulin.net,
	airlied@...il.com, simona@...ll.ch, intel-gfx@...ts.freedesktop.org,
	intel-xe@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org,
	anupnewsmail@...il.com
Subject: Re: [PATCH] gpu: drm: i915: display: Avoid null values
 intel_plane_atomic_check_with_state

On Fri, Sep 27, 2024 at 11:20:32AM +0300, Jani Nikula wrote:
> On Fri, 27 Sep 2024, Alessandro Zanni <alessandro.zanni87@...il.com> wrote:
> > This fix solves multiple Smatch errors:
> >
> > drivers/gpu/drm/i915/display/intel_atomic_plane.c:660
> > intel_plane_atomic_check_with_state() error:
> > we previously assumed 'fb' could be null (see line 648)
> >
> > drivers/gpu/drm/i915/display/intel_atomic_plane.c:664
> > intel_plane_atomic_check_with_state()
> > error: we previously assumed 'fb' could be null (see line 659)
> >
> > drivers/gpu/drm/i915/display/intel_atomic_plane.c:671
> > intel_plane_atomic_check_with_state()
> > error: we previously assumed 'fb' could be null (see line 663)
> >
> > We should check first if fb is not null before to access its properties.
> 
> new_plane_state->uapi.visible && !fb should not be possible, but it's
> probably too hard for smatch to figure out. It's not exactly trivial for
> humans to figure out either.
> 
> I'm thinking something like below to help both.
> 
> Ville, thoughts?
> 
> 
> BR,
> Jani.
> 
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 3505a5b52eb9..d9da47aed55d 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -629,6 +629,9 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  	if (ret)
>  		return ret;
>  
> +	if (drm_WARN_ON(display->drm, new_plane_state->uapi.visible && !fb))
> +		return -EINVAL;
> +

We have probably 100 places that would need this. So it's going
to be extremely ugly.

One approach I could maybe tolerate is something like
intel_plane_is_visible(plane_state) 
{
	if (drm_WARN_ON(visible && !fb))
		return false;

	return plane_state->visible;
}

+ s/plane_state->visible/intel_plane_is_visible(plane_state)/

But is that going to help these obtuse tools?

>  	if (fb)
>  		new_crtc_state->enabled_planes |= BIT(plane->id);
>  
> 
> 
> >
> > Signed-off-by: Alessandro Zanni <alessandro.zanni87@...il.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_atomic_plane.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index e979786aa5cf..1606f79b39e6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -656,18 +656,18 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >  	    intel_plane_is_scaled(new_plane_state))
> >  		new_crtc_state->scaled_planes |= BIT(plane->id);
> >  
> > -	if (new_plane_state->uapi.visible &&
> > +	if (new_plane_state->uapi.visible && fb &&
> >  	    intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier))
> >  		new_crtc_state->nv12_planes |= BIT(plane->id);
> >  
> > -	if (new_plane_state->uapi.visible &&
> > +	if (new_plane_state->uapi.visible && fb &&
> >  	    fb->format->format == DRM_FORMAT_C8)
> >  		new_crtc_state->c8_planes |= BIT(plane->id);
> >  
> >  	if (new_plane_state->uapi.visible || old_plane_state->uapi.visible)
> >  		new_crtc_state->update_planes |= BIT(plane->id);
> >  
> > -	if (new_plane_state->uapi.visible &&
> > +	if (new_plane_state->uapi.visible && fb &&
> >  	    intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier)) {
> >  		new_crtc_state->data_rate_y[plane->id] =
> >  			intel_plane_data_rate(new_crtc_state, new_plane_state, 0);
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel