[PATCH] drm/i915/display: Check source height is > 0

[PATCH] drm/i915/display: Check source height is > 0

[Drew Davenport]

The error message suggests that the height of the src rect must be at
least 1. Reject source with height of 0.

Signed-off-by: Drew Davenport <ddavenport@chromium.org>

---
I was investigating some divide-by-zero crash reports on ChromeOS which
pointed to the intel_adjusted_rate function. Further prodding showed
that I could reproduce this in a simple test program if I made src_h
some value less than 1 but greater than 0.

This seemed to be a sensible place to check that the source height is at
least 1. I tried to repro this issue on an amd device I had on hand, and
the configuration was rejected.

Would it make sense to add a check that source dimensions are at least 1
somewhere in core, like in drm_atomic_plane_check? Or is that a valid
use case on some devices, and thus any such check should be done on a
per-driver basis?

Thanks.

 drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 4b79c2d2d6177..9b172a1e90deb 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
 	u32 offset;
 	int ret;
 
-	if (w > max_width || w < min_width || h > max_height) {
+	if (w > max_width || w < min_width || h > max_height || h < 1) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n",
 			    w, h, min_width, max_width, max_height);
-- 
2.39.0.314.g84b9a713c41-goog

Re: [Intel-gfx] [PATCH] drm/i915/display: Check source height is > 0

[Teres Alexis, Alan Previn]

Is there a better place for this check higher up the intel specific atomic-check? (so the check won't be skl specific - i notice that intel_adjusted_rate is also called by
ilk_foo as well and non-backend-specific functions). Else, perhaps intel_adjusted_rate should add a check + WARN? (if we are trying to propagate this slowly across HW).


...alan 

On Mon, 2022-12-26 at 22:53 -0700, Drew Davenport wrote:
> The error message suggests that the height of the src rect must be at
> least 1. Reject source with height of 0.
> 
> Signed-off-by: Drew Davenport <ddavenport@chromium.org>
> 
> ---
> I was investigating some divide-by-zero crash reports on ChromeOS which
> pointed to the intel_adjusted_rate function. Further prodding showed
> that I could reproduce this in a simple test program if I made src_h
> some value less than 1 but greater than 0.
> 
> This seemed to be a sensible place to check that the source height is at
> least 1. I tried to repro this issue on an amd device I had on hand, and
> the configuration was rejected.
> 
> Would it make sense to add a check that source dimensions are at least 1
> somewhere in core, like in drm_atomic_plane_check? Or is that a valid
> use case on some devices, and thus any such check should be done on a
> per-driver basis?
> 
> Thanks.
> 
>  drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4b79c2d2d6177..9b172a1e90deb 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
>  	u32 offset;
>  	int ret;
>  
> -	if (w > max_width || w < min_width || h > max_height) {
> +	if (w > max_width || w < min_width || h > max_height || h < 1) {
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n",
>  			    w, h, min_width, max_width, max_height);
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 

Re: [Intel-gfx] [PATCH] drm/i915/display: Check source height is > 0

[Drew Davenport]

On Tue, Dec 27, 2022 at 05:55:17PM +0000, Teres Alexis, Alan Previn wrote:
> Is there a better place for this check higher up the intel specific atomic-check? (so the check won't be skl specific - i notice that intel_adjusted_rate is also called by
> ilk_foo as well and non-backend-specific functions). Else, perhaps intel_adjusted_rate should add a check + WARN? (if we are trying to propagate this slowly across HW).

Would intel_plane_atomic_check_with_state be a more appropriate
place to check that the src width and height are at least 1? This is
where skl_plane_check and other HW's foo_plane_check functions are called
from.

I don't think that the potential divide-by-zero will be hit in the case
where intel_adjusted_rate is called from ilk_pipe_pixel_rate because the
src rect will not have a fractional part to it in this case. I'm assuming
that something earlier on would catch and reject a src with zero
width/height.

Drew

> 
> 
> ...alan 
> 
> On Mon, 2022-12-26 at 22:53 -0700, Drew Davenport wrote:
> > The error message suggests that the height of the src rect must be at
> > least 1. Reject source with height of 0.
> > 
> > Signed-off-by: Drew Davenport <ddavenport@chromium.org>
> > 
> > ---
> > I was investigating some divide-by-zero crash reports on ChromeOS which
> > pointed to the intel_adjusted_rate function. Further prodding showed
> > that I could reproduce this in a simple test program if I made src_h
> > some value less than 1 but greater than 0.
> > 
> > This seemed to be a sensible place to check that the source height is at
> > least 1. I tried to repro this issue on an amd device I had on hand, and
> > the configuration was rejected.
> > 
> > Would it make sense to add a check that source dimensions are at least 1
> > somewhere in core, like in drm_atomic_plane_check? Or is that a valid
> > use case on some devices, and thus any such check should be done on a
> > per-driver basis?
> > 
> > Thanks.
> > 
> >  drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 4b79c2d2d6177..9b172a1e90deb 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> >  	u32 offset;
> >  	int ret;
> >  
> > -	if (w > max_width || w < min_width || h > max_height) {
> > +	if (w > max_width || w < min_width || h > max_height || h < 1) {
> >  		drm_dbg_kms(&dev_priv->drm,
> >  			    "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n",
> >  			    w, h, min_width, max_width, max_height);
> > -- 
> > 2.39.0.314.g84b9a713c41-goog
> > 
> 

Re: [PATCH] drm/i915/display: Check source height is > 0

[Juha-Pekka Heikkila]

Hi Drew,

this is good find. I went looking where the problem is in and saw what 
you probably also saw earlier.

I was wondering if diff below would be better fix? I assume this would 
end up with einval or erange in your case but code flow otherwise would 
stay as is while fixing all future callers for same issue:

diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c 
b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index 10e1fc9d0698..a9948e8d3543 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct 
drm_rect *src,
                                  const struct drm_rect *dst,
                                  unsigned int rate)
  {
-       unsigned int src_w, src_h, dst_w, dst_h;
+       unsigned int src_w, src_h, dst_w, dst_h, dst_wh;

         src_w = drm_rect_width(src) >> 16;
         src_h = drm_rect_height(src) >> 16;
@@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct 
drm_rect *src,
         dst_w = min(src_w, dst_w);
         dst_h = min(src_h, dst_h);

-       return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h),
-                               dst_w * dst_h);
+       /* in case src contained only fractional part */
+       dst_wh = max(dst_w * dst_h, (unsigned) 1);
+
+       return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh);
  }

  unsigned int intel_plane_pixel_rate(const struct intel_crtc_state 
*crtc_state,


What do you think? I'll in any case come up with some test for this in igt.

/Juha-Pekka

On 27.12.2022 7.53, Drew Davenport wrote:
> The error message suggests that the height of the src rect must be at
> least 1. Reject source with height of 0.
> 
> Signed-off-by: Drew Davenport <ddavenport@chromium.org>
> 
> ---
> I was investigating some divide-by-zero crash reports on ChromeOS which
> pointed to the intel_adjusted_rate function. Further prodding showed
> that I could reproduce this in a simple test program if I made src_h
> some value less than 1 but greater than 0.
> 
> This seemed to be a sensible place to check that the source height is at
> least 1. I tried to repro this issue on an amd device I had on hand, and
> the configuration was rejected.
> 
> Would it make sense to add a check that source dimensions are at least 1
> somewhere in core, like in drm_atomic_plane_check? Or is that a valid
> use case on some devices, and thus any such check should be done on a
> per-driver basis?
> 
> Thanks.
> 
>   drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4b79c2d2d6177..9b172a1e90deb 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
>   	u32 offset;
>   	int ret;
>   
> -	if (w > max_width || w < min_width || h > max_height) {
> +	if (w > max_width || w < min_width || h > max_height || h < 1) {
>   		drm_dbg_kms(&dev_priv->drm,
>   			    "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n",
>   			    w, h, min_width, max_width, max_height);

Re: [PATCH] drm/i915/display: Check source height is > 0

[Drew Davenport]

On Tue, Jan 03, 2023 at 12:42:43PM +0200, Juha-Pekka Heikkila wrote:
> Hi Drew,

Hi Juha-Pekka, sorry for the late response since I was on vacation.

> 
> this is good find. I went looking where the problem is in and saw what you
> probably also saw earlier.
> 
> I was wondering if diff below would be better fix? I assume this would end
> up with einval or erange in your case but code flow otherwise would stay as
> is while fixing all future callers for same issue:

Yes, the function you identify below is where I encountered
divide-by-zero errors. If width/height less than 1 is a legitimate use
case, then your proposed solution looks like it would be better. It
should have no risk of regression in userspace either, which is nice.

I tested your patch, and as expected I did not hit the divide-by-zero
error, though the test commit was rejected due to a check further along
inside skl_update_scaler. Perhaps there is some other configuration
which would pass the test commit with a width/height less than 1, but I
didn't dig much further.

> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 10e1fc9d0698..a9948e8d3543 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect
> *src,
>                                  const struct drm_rect *dst,
>                                  unsigned int rate)
>  {
> -       unsigned int src_w, src_h, dst_w, dst_h;
> +       unsigned int src_w, src_h, dst_w, dst_h, dst_wh;
> 
>         src_w = drm_rect_width(src) >> 16;
>         src_h = drm_rect_height(src) >> 16;
> @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect
> *src,
>         dst_w = min(src_w, dst_w);
>         dst_h = min(src_h, dst_h);
> 
> -       return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h),
> -                               dst_w * dst_h);
> +       /* in case src contained only fractional part */
> +       dst_wh = max(dst_w * dst_h, (unsigned) 1);
> +
> +       return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh);
>  }
> 
>  unsigned int intel_plane_pixel_rate(const struct intel_crtc_state
> *crtc_state,
> 
> 
> What do you think? I'll in any case come up with some test for this in igt.

I see that you've posted your fix to the list already. Adding a
test to cover this in IGT also sounds great. Thanks!

Breadcrumbs to Juha-Pekka's patch for anyone following this
thread: https://patchwork.freedesktop.org/series/112396/

> 
> /Juha-Pekka
> 
> On 27.12.2022 7.53, Drew Davenport wrote:
> > The error message suggests that the height of the src rect must be at
> > least 1. Reject source with height of 0.
> > 
> > Signed-off-by: Drew Davenport <ddavenport@chromium.org>
> > 
> > ---
> > I was investigating some divide-by-zero crash reports on ChromeOS which
> > pointed to the intel_adjusted_rate function. Further prodding showed
> > that I could reproduce this in a simple test program if I made src_h
> > some value less than 1 but greater than 0.
> > 
> > This seemed to be a sensible place to check that the source height is at
> > least 1. I tried to repro this issue on an amd device I had on hand, and
> > the configuration was rejected.
> > 
> > Would it make sense to add a check that source dimensions are at least 1
> > somewhere in core, like in drm_atomic_plane_check? Or is that a valid
> > use case on some devices, and thus any such check should be done on a
> > per-driver basis?
> > 
> > Thanks.
> > 
> >   drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 4b79c2d2d6177..9b172a1e90deb 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> >   	u32 offset;
> >   	int ret;
> > -	if (w > max_width || w < min_width || h > max_height) {
> > +	if (w > max_width || w < min_width || h > max_height || h < 1) {
> >   		drm_dbg_kms(&dev_priv->drm,
> >   			    "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n",
> >   			    w, h, min_width, max_width, max_height);
> 

Re: [PATCH] drm/i915/display: Check source height is > 0

[Ville Syrjälä]

On Mon, Dec 26, 2022 at 10:53:24PM -0700, Drew Davenport wrote:
> The error message suggests that the height of the src rect must be at
> least 1. Reject source with height of 0.
> 
> Signed-off-by: Drew Davenport <ddavenport@chromium.org>
> 
> ---
> I was investigating some divide-by-zero crash reports on ChromeOS which
> pointed to the intel_adjusted_rate function. Further prodding showed
> that I could reproduce this in a simple test program if I made src_h
> some value less than 1 but greater than 0.
> 
> This seemed to be a sensible place to check that the source height is at
> least 1. I tried to repro this issue on an amd device I had on hand, and
> the configuration was rejected.
> 
> Would it make sense to add a check that source dimensions are at least 1
> somewhere in core, like in drm_atomic_plane_check? Or is that a valid
> use case on some devices, and thus any such check should be done on a
> per-driver basis?
> 
> Thanks.
> 
>  drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4b79c2d2d6177..9b172a1e90deb 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
>  	u32 offset;
>  	int ret;
>  
> -	if (w > max_width || w < min_width || h > max_height) {
> +	if (w > max_width || w < min_width || h > max_height || h < 1) {

I liked this one best so pushed to drm-intel-next with cc:stable. Thanks.

In the future we might want to move some of these checks to an earlier
spot to make sure we don't hit any other weird issues in some other
code, but for the moment I think this will do.

>  		drm_dbg_kms(&dev_priv->drm,
>  			    "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n",
>  			    w, h, min_width, max_width, max_height);
> -- 
> 2.39.0.314.g84b9a713c41-goog

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH] drm/i915/display: Check source height is > 0

[Juha-Pekka Heikkila]

On 12.1.2023 20.28, Ville Syrjälä wrote:
> On Mon, Dec 26, 2022 at 10:53:24PM -0700, Drew Davenport wrote:
>> The error message suggests that the height of the src rect must be at
>> least 1. Reject source with height of 0.
>>
>> Signed-off-by: Drew Davenport <ddavenport@chromium.org>
>>
>> ---
>> I was investigating some divide-by-zero crash reports on ChromeOS which
>> pointed to the intel_adjusted_rate function. Further prodding showed
>> that I could reproduce this in a simple test program if I made src_h
>> some value less than 1 but greater than 0.
>>
>> This seemed to be a sensible place to check that the source height is at
>> least 1. I tried to repro this issue on an amd device I had on hand, and
>> the configuration was rejected.
>>
>> Would it make sense to add a check that source dimensions are at least 1
>> somewhere in core, like in drm_atomic_plane_check? Or is that a valid
>> use case on some devices, and thus any such check should be done on a
>> per-driver basis?
>>
>> Thanks.
>>
>>   drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> index 4b79c2d2d6177..9b172a1e90deb 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
>>   	u32 offset;
>>   	int ret;
>>   
>> -	if (w > max_width || w < min_width || h > max_height) {
>> +	if (w > max_width || w < min_width || h > max_height || h < 1) {
> 
> I liked this one best so pushed to drm-intel-next with cc:stable. Thanks.
> 
> In the future we might want to move some of these checks to an earlier
> spot to make sure we don't hit any other weird issues in some other
> code, but for the moment I think this will do.
> 

Look ok to me. Tests which I had written to try different ways to cause 
this issue are now returning einval as expected. I'll polish my igt test 
for this issue and send it out bit later.

/Juha-pekka

>>   		drm_dbg_kms(&dev_priv->drm,
>>   			    "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n",
>>   			    w, h, min_width, max_width, max_height);
>> -- 
>> 2.39.0.314.g84b9a713c41-goog
>