drm/radeon/evergreen_cs: fix int overflow errors in cs track offsets

Commit Message

Nikita Zhandarovich July 25, 2024, 6:09 p.m. UTC

Several cs track offsets (such as 'track->db_s_read_offset')
either are initialized with or plainly take big enough values that,
once shifted 8 bits left, may be hit with integer overflow if the
resulting values end up going over u32 limit.

Some debug prints take this into account (see according dev_warn() in
evergreen_cs_track_validate_stencil()), even if the actual
calculated value assigned to local 'offset' variable is missing
similar proper expansion.

Mitigate the problem by casting the type of right operands to the
wider type of corresponding left ones in all such cases.

Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.

Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni tiling informations v11")
Cc: [e-mail]
Signed-off-by: Nikita Zhandarovich <[e-mail]>
---
P.S. While I am not certain that track->cb_color_bo_offset[id]
actually ends up taking values high enough to cause an overflow,
nonetheless I thought it prudent to cast it to ulong as well.

 drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Alex Deucher July 25, 2024, 8:59 p.m. UTC | #1

Applied.  Thanks!

Alex

On Thu, Jul 25, 2024 at 2:20 PM Nikita Zhandarovich
<[e-mail]> wrote:
>
> Several cs track offsets (such as 'track->db_s_read_offset')
> either are initialized with or plainly take big enough values that,
> once shifted 8 bits left, may be hit with integer overflow if the
> resulting values end up going over u32 limit.
>
> Some debug prints take this into account (see according dev_warn() in
> evergreen_cs_track_validate_stencil()), even if the actual
> calculated value assigned to local 'offset' variable is missing
> similar proper expansion.
>
> Mitigate the problem by casting the type of right operands to the
> wider type of corresponding left ones in all such cases.
>
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
>
> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni tiling informations v11")
> Cc: [e-mail]
> Signed-off-by: Nikita Zhandarovich <[e-mail]>
> ---
> P.S. While I am not certain that track->cb_color_bo_offset[id]
> actually ends up taking values high enough to cause an overflow,
> nonetheless I thought it prudent to cast it to ulong as well.
>
>  drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c b/drivers/gpu/drm/radeon/evergreen_cs.c
> index 1fe6e0d883c7..d734d221e2da 100644
> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
> @@ -433,7 +433,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>                 return r;
>         }
>
> -       offset = track->cb_color_bo_offset[id] << 8;
> +       offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>         if (offset & (surf.base_align - 1)) {
>                 dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not aligned with %ld\n",
>                          __func__, __LINE__, id, offset, surf.base_align);
> @@ -455,7 +455,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>                                 min = surf.nby - 8;
>                         }
>                         bsize = radeon_bo_size(track->cb_color_bo[id]);
> -                       tmp = track->cb_color_bo_offset[id] << 8;
> +                       tmp = (unsigned long)track->cb_color_bo_offset[id] << 8;
>                         for (nby = surf.nby; nby > min; nby--) {
>                                 size = nby * surf.nbx * surf.bpe * surf.nsamples;
>                                 if ((tmp + size * mslice) <= bsize) {
> @@ -476,10 +476,10 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>                         }
>                 }
>                 dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer size %d, "
> -                        "offset %d, max layer %d, bo size %ld, slice %d)\n",
> +                        "offset %ld, max layer %d, bo size %ld, slice %d)\n",
>                          __func__, __LINE__, id, surf.layer_size,
> -                       track->cb_color_bo_offset[id] << 8, mslice,
> -                       radeon_bo_size(track->cb_color_bo[id]), slice);
> +                       (unsigned long)track->cb_color_bo_offset[id] << 8,
> +                       mslice, radeon_bo_size(track->cb_color_bo[id]), slice);
>                 dev_warn(p->dev, "%s:%d problematic surf: (%d %d) (%d %d %d %d %d %d %d)\n",
>                          __func__, __LINE__, surf.nbx, surf.nby,
>                         surf.mode, surf.bpe, surf.nsamples,
> @@ -608,7 +608,7 @@ static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>                 return r;
>         }
>
> -       offset = track->db_s_read_offset << 8;
> +       offset = (unsigned long)track->db_s_read_offset << 8;
>         if (offset & (surf.base_align - 1)) {
>                 dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
>                          __func__, __LINE__, offset, surf.base_align);
> @@ -627,7 +627,7 @@ static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>                 return -EINVAL;
>         }
>
> -       offset = track->db_s_write_offset << 8;
> +       offset = (unsigned long)track->db_s_write_offset << 8;
>         if (offset & (surf.base_align - 1)) {
>                 dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
>                          __func__, __LINE__, offset, surf.base_align);
> @@ -706,7 +706,7 @@ static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>                 return r;
>         }
>
> -       offset = track->db_z_read_offset << 8;
> +       offset = (unsigned long)track->db_z_read_offset << 8;
>         if (offset & (surf.base_align - 1)) {
>                 dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
>                          __func__, __LINE__, offset, surf.base_align);
> @@ -722,7 +722,7 @@ static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>                 return -EINVAL;
>         }
>
> -       offset = track->db_z_write_offset << 8;
> +       offset = (unsigned long)track->db_z_write_offset << 8;
>         if (offset & (surf.base_align - 1)) {
>                 dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
>                          __func__, __LINE__, offset, surf.base_align);

Christian König July 26, 2024, 7:05 a.m. UTC | #2

Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
> Several cs track offsets (such as 'track->db_s_read_offset')
> either are initialized with or plainly take big enough values that,
> once shifted 8 bits left, may be hit with integer overflow if the
> resulting values end up going over u32 limit.
>
> Some debug prints take this into account (see according dev_warn() in
> evergreen_cs_track_validate_stencil()), even if the actual
> calculated value assigned to local 'offset' variable is missing
> similar proper expansion.
>
> Mitigate the problem by casting the type of right operands to the
> wider type of corresponding left ones in all such cases.
>
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
>
> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni tiling informations v11")
> Cc: [e-mail]

Well first of all the long cast doesn't makes the value 64bit, it 
depends on the architecture.

Then IIRC the underlying hw can only handle a 32bit address space so 
having the offset as long is incorrect to begin with.

And finally that is absolutely not material for stable.

Regards,
Christian.

> Signed-off-by: Nikita Zhandarovich <[e-mail]>
> ---
> P.S. While I am not certain that track->cb_color_bo_offset[id]
> actually ends up taking values high enough to cause an overflow,
> nonetheless I thought it prudent to cast it to ulong as well.
>
>   drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c b/drivers/gpu/drm/radeon/evergreen_cs.c
> index 1fe6e0d883c7..d734d221e2da 100644
> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
> @@ -433,7 +433,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>   		return r;
>   	}
>   
> -	offset = track->cb_color_bo_offset[id] << 8;
> +	offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>   	if (offset & (surf.base_align - 1)) {
>   		dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not aligned with %ld\n",
>   			 __func__, __LINE__, id, offset, surf.base_align);
> @@ -455,7 +455,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>   				min = surf.nby - 8;
>   			}
>   			bsize = radeon_bo_size(track->cb_color_bo[id]);
> -			tmp = track->cb_color_bo_offset[id] << 8;
> +			tmp = (unsigned long)track->cb_color_bo_offset[id] << 8;
>   			for (nby = surf.nby; nby > min; nby--) {
>   				size = nby * surf.nbx * surf.bpe * surf.nsamples;
>   				if ((tmp + size * mslice) <= bsize) {
> @@ -476,10 +476,10 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>   			}
>   		}
>   		dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer size %d, "
> -			 "offset %d, max layer %d, bo size %ld, slice %d)\n",
> +			 "offset %ld, max layer %d, bo size %ld, slice %d)\n",
>   			 __func__, __LINE__, id, surf.layer_size,
> -			track->cb_color_bo_offset[id] << 8, mslice,
> -			radeon_bo_size(track->cb_color_bo[id]), slice);
> +			(unsigned long)track->cb_color_bo_offset[id] << 8,
> +			mslice,	radeon_bo_size(track->cb_color_bo[id]), slice);
>   		dev_warn(p->dev, "%s:%d problematic surf: (%d %d) (%d %d %d %d %d %d %d)\n",
>   			 __func__, __LINE__, surf.nbx, surf.nby,
>   			surf.mode, surf.bpe, surf.nsamples,
> @@ -608,7 +608,7 @@ static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>   		return r;
>   	}
>   
> -	offset = track->db_s_read_offset << 8;
> +	offset = (unsigned long)track->db_s_read_offset << 8;
>   	if (offset & (surf.base_align - 1)) {
>   		dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
>   			 __func__, __LINE__, offset, surf.base_align);
> @@ -627,7 +627,7 @@ static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>   		return -EINVAL;
>   	}
>   
> -	offset = track->db_s_write_offset << 8;
> +	offset = (unsigned long)track->db_s_write_offset << 8;
>   	if (offset & (surf.base_align - 1)) {
>   		dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
>   			 __func__, __LINE__, offset, surf.base_align);
> @@ -706,7 +706,7 @@ static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>   		return r;
>   	}
>   
> -	offset = track->db_z_read_offset << 8;
> +	offset = (unsigned long)track->db_z_read_offset << 8;
>   	if (offset & (surf.base_align - 1)) {
>   		dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
>   			 __func__, __LINE__, offset, surf.base_align);
> @@ -722,7 +722,7 @@ static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>   		return -EINVAL;
>   	}
>   
> -	offset = track->db_z_write_offset << 8;
> +	offset = (unsigned long)track->db_z_write_offset << 8;
>   	if (offset & (surf.base_align - 1)) {
>   		dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
>   			 __func__, __LINE__, offset, surf.base_align);

Christian König July 26, 2024, 7:06 a.m. UTC | #3

I strongly suggest to revert that again. See my other mail.

Christian.

Am 25.07.24 um 22:59 schrieb Alex Deucher:
> Applied.  Thanks!
>
> Alex
>
> On Thu, Jul 25, 2024 at 2:20 PM Nikita Zhandarovich
> <[e-mail]> wrote:
>> Several cs track offsets (such as 'track->db_s_read_offset')
>> either are initialized with or plainly take big enough values that,
>> once shifted 8 bits left, may be hit with integer overflow if the
>> resulting values end up going over u32 limit.
>>
>> Some debug prints take this into account (see according dev_warn() in
>> evergreen_cs_track_validate_stencil()), even if the actual
>> calculated value assigned to local 'offset' variable is missing
>> similar proper expansion.
>>
>> Mitigate the problem by casting the type of right operands to the
>> wider type of corresponding left ones in all such cases.
>>
>> Found by Linux Verification Center (linuxtesting.org) with static
>> analysis tool SVACE.
>>
>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni tiling informations v11")
>> Cc: [e-mail]
>> Signed-off-by: Nikita Zhandarovich <[e-mail]>
>> ---
>> P.S. While I am not certain that track->cb_color_bo_offset[id]
>> actually ends up taking values high enough to cause an overflow,
>> nonetheless I thought it prudent to cast it to ulong as well.
>>
>>   drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c b/drivers/gpu/drm/radeon/evergreen_cs.c
>> index 1fe6e0d883c7..d734d221e2da 100644
>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
>> @@ -433,7 +433,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>>                  return r;
>>          }
>>
>> -       offset = track->cb_color_bo_offset[id] << 8;
>> +       offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>          if (offset & (surf.base_align - 1)) {
>>                  dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not aligned with %ld\n",
>>                           __func__, __LINE__, id, offset, surf.base_align);
>> @@ -455,7 +455,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>>                                  min = surf.nby - 8;
>>                          }
>>                          bsize = radeon_bo_size(track->cb_color_bo[id]);
>> -                       tmp = track->cb_color_bo_offset[id] << 8;
>> +                       tmp = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>                          for (nby = surf.nby; nby > min; nby--) {
>>                                  size = nby * surf.nbx * surf.bpe * surf.nsamples;
>>                                  if ((tmp + size * mslice) <= bsize) {
>> @@ -476,10 +476,10 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>>                          }
>>                  }
>>                  dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer size %d, "
>> -                        "offset %d, max layer %d, bo size %ld, slice %d)\n",
>> +                        "offset %ld, max layer %d, bo size %ld, slice %d)\n",
>>                           __func__, __LINE__, id, surf.layer_size,
>> -                       track->cb_color_bo_offset[id] << 8, mslice,
>> -                       radeon_bo_size(track->cb_color_bo[id]), slice);
>> +                       (unsigned long)track->cb_color_bo_offset[id] << 8,
>> +                       mslice, radeon_bo_size(track->cb_color_bo[id]), slice);
>>                  dev_warn(p->dev, "%s:%d problematic surf: (%d %d) (%d %d %d %d %d %d %d)\n",
>>                           __func__, __LINE__, surf.nbx, surf.nby,
>>                          surf.mode, surf.bpe, surf.nsamples,
>> @@ -608,7 +608,7 @@ static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>                  return r;
>>          }
>>
>> -       offset = track->db_s_read_offset << 8;
>> +       offset = (unsigned long)track->db_s_read_offset << 8;
>>          if (offset & (surf.base_align - 1)) {
>>                  dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
>>                           __func__, __LINE__, offset, surf.base_align);
>> @@ -627,7 +627,7 @@ static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>                  return -EINVAL;
>>          }
>>
>> -       offset = track->db_s_write_offset << 8;
>> +       offset = (unsigned long)track->db_s_write_offset << 8;
>>          if (offset & (surf.base_align - 1)) {
>>                  dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
>>                           __func__, __LINE__, offset, surf.base_align);
>> @@ -706,7 +706,7 @@ static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>                  return r;
>>          }
>>
>> -       offset = track->db_z_read_offset << 8;
>> +       offset = (unsigned long)track->db_z_read_offset << 8;
>>          if (offset & (surf.base_align - 1)) {
>>                  dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
>>                           __func__, __LINE__, offset, surf.base_align);
>> @@ -722,7 +722,7 @@ static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>                  return -EINVAL;
>>          }
>>
>> -       offset = track->db_z_write_offset << 8;
>> +       offset = (unsigned long)track->db_z_write_offset << 8;
>>          if (offset & (surf.base_align - 1)) {
>>                  dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
>>                           __func__, __LINE__, offset, surf.base_align);

Alex Deucher July 26, 2024, 12:52 p.m. UTC | #4

On Fri, Jul 26, 2024 at 3:05 AM Christian König
<[e-mail]> wrote:
>
> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
> > Several cs track offsets (such as 'track->db_s_read_offset')
> > either are initialized with or plainly take big enough values that,
> > once shifted 8 bits left, may be hit with integer overflow if the
> > resulting values end up going over u32 limit.
> >
> > Some debug prints take this into account (see according dev_warn() in
> > evergreen_cs_track_validate_stencil()), even if the actual
> > calculated value assigned to local 'offset' variable is missing
> > similar proper expansion.
> >
> > Mitigate the problem by casting the type of right operands to the
> > wider type of corresponding left ones in all such cases.
> >
> > Found by Linux Verification Center (linuxtesting.org) with static
> > analysis tool SVACE.
> >
> > Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni tiling informations v11")
> > Cc: [e-mail]
>
> Well first of all the long cast doesn't makes the value 64bit, it
> depends on the architecture.
>
> Then IIRC the underlying hw can only handle a 32bit address space so
> having the offset as long is incorrect to begin with.

Evergreen chips support a 36 bit internal address space and NI and
newer support a 40 bit one, so this is applicable.

Alex

>
> And finally that is absolutely not material for stable.
>
> Regards,
> Christian.
>
> > Signed-off-by: Nikita Zhandarovich <[e-mail]>
> > ---
> > P.S. While I am not certain that track->cb_color_bo_offset[id]
> > actually ends up taking values high enough to cause an overflow,
> > nonetheless I thought it prudent to cast it to ulong as well.
> >
> >   drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
> >   1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c b/drivers/gpu/drm/radeon/evergreen_cs.c
> > index 1fe6e0d883c7..d734d221e2da 100644
> > --- a/drivers/gpu/drm/radeon/evergreen_cs.c
> > +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
> > @@ -433,7 +433,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
> >               return r;
> >       }
> >
> > -     offset = track->cb_color_bo_offset[id] << 8;
> > +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
> >       if (offset & (surf.base_align - 1)) {
> >               dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not aligned with %ld\n",
> >                        __func__, __LINE__, id, offset, surf.base_align);
> > @@ -455,7 +455,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
> >                               min = surf.nby - 8;
> >                       }
> >                       bsize = radeon_bo_size(track->cb_color_bo[id]);
> > -                     tmp = track->cb_color_bo_offset[id] << 8;
> > +                     tmp = (unsigned long)track->cb_color_bo_offset[id] << 8;
> >                       for (nby = surf.nby; nby > min; nby--) {
> >                               size = nby * surf.nbx * surf.bpe * surf.nsamples;
> >                               if ((tmp + size * mslice) <= bsize) {
> > @@ -476,10 +476,10 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
> >                       }
> >               }
> >               dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer size %d, "
> > -                      "offset %d, max layer %d, bo size %ld, slice %d)\n",
> > +                      "offset %ld, max layer %d, bo size %ld, slice %d)\n",
> >                        __func__, __LINE__, id, surf.layer_size,
> > -                     track->cb_color_bo_offset[id] << 8, mslice,
> > -                     radeon_bo_size(track->cb_color_bo[id]), slice);
> > +                     (unsigned long)track->cb_color_bo_offset[id] << 8,
> > +                     mslice, radeon_bo_size(track->cb_color_bo[id]), slice);
> >               dev_warn(p->dev, "%s:%d problematic surf: (%d %d) (%d %d %d %d %d %d %d)\n",
> >                        __func__, __LINE__, surf.nbx, surf.nby,
> >                       surf.mode, surf.bpe, surf.nsamples,
> > @@ -608,7 +608,7 @@ static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
> >               return r;
> >       }
> >
> > -     offset = track->db_s_read_offset << 8;
> > +     offset = (unsigned long)track->db_s_read_offset << 8;
> >       if (offset & (surf.base_align - 1)) {
> >               dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
> >                        __func__, __LINE__, offset, surf.base_align);
> > @@ -627,7 +627,7 @@ static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
> >               return -EINVAL;
> >       }
> >
> > -     offset = track->db_s_write_offset << 8;
> > +     offset = (unsigned long)track->db_s_write_offset << 8;
> >       if (offset & (surf.base_align - 1)) {
> >               dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
> >                        __func__, __LINE__, offset, surf.base_align);
> > @@ -706,7 +706,7 @@ static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
> >               return r;
> >       }
> >
> > -     offset = track->db_z_read_offset << 8;
> > +     offset = (unsigned long)track->db_z_read_offset << 8;
> >       if (offset & (surf.base_align - 1)) {
> >               dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
> >                        __func__, __LINE__, offset, surf.base_align);
> > @@ -722,7 +722,7 @@ static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
> >               return -EINVAL;
> >       }
> >
> > -     offset = track->db_z_write_offset << 8;
> > +     offset = (unsigned long)track->db_z_write_offset << 8;
> >       if (offset & (surf.base_align - 1)) {
> >               dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
> >                        __func__, __LINE__, offset, surf.base_align);
>

Christian König July 29, 2024, 9:23 a.m. UTC | #5

Am 26.07.24 um 14:52 schrieb Alex Deucher:
> On Fri, Jul 26, 2024 at 3:05 AM Christian König
> <[e-mail]> wrote:
>> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
>>> Several cs track offsets (such as 'track->db_s_read_offset')
>>> either are initialized with or plainly take big enough values that,
>>> once shifted 8 bits left, may be hit with integer overflow if the
>>> resulting values end up going over u32 limit.
>>>
>>> Some debug prints take this into account (see according dev_warn() in
>>> evergreen_cs_track_validate_stencil()), even if the actual
>>> calculated value assigned to local 'offset' variable is missing
>>> similar proper expansion.
>>>
>>> Mitigate the problem by casting the type of right operands to the
>>> wider type of corresponding left ones in all such cases.
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with static
>>> analysis tool SVACE.
>>>
>>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni tiling informations v11")
>>> Cc: [e-mail]
>> Well first of all the long cast doesn't makes the value 64bit, it
>> depends on the architecture.
>>
>> Then IIRC the underlying hw can only handle a 32bit address space so
>> having the offset as long is incorrect to begin with.
> Evergreen chips support a 36 bit internal address space and NI and
> newer support a 40 bit one, so this is applicable.

In that case I strongly suggest that we replace the unsigned long with 
u64 or otherwise we get different behavior on 32 and 64bit machines.

Regards,
Christian.

>
> Alex
>
>> And finally that is absolutely not material for stable.
>>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Nikita Zhandarovich <[e-mail]>
>>> ---
>>> P.S. While I am not certain that track->cb_color_bo_offset[id]
>>> actually ends up taking values high enough to cause an overflow,
>>> nonetheless I thought it prudent to cast it to ulong as well.
>>>
>>>    drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>>>    1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c b/drivers/gpu/drm/radeon/evergreen_cs.c
>>> index 1fe6e0d883c7..d734d221e2da 100644
>>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
>>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
>>> @@ -433,7 +433,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>>>                return r;
>>>        }
>>>
>>> -     offset = track->cb_color_bo_offset[id] << 8;
>>> +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>>        if (offset & (surf.base_align - 1)) {
>>>                dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not aligned with %ld\n",
>>>                         __func__, __LINE__, id, offset, surf.base_align);
>>> @@ -455,7 +455,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>>>                                min = surf.nby - 8;
>>>                        }
>>>                        bsize = radeon_bo_size(track->cb_color_bo[id]);
>>> -                     tmp = track->cb_color_bo_offset[id] << 8;
>>> +                     tmp = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>>                        for (nby = surf.nby; nby > min; nby--) {
>>>                                size = nby * surf.nbx * surf.bpe * surf.nsamples;
>>>                                if ((tmp + size * mslice) <= bsize) {
>>> @@ -476,10 +476,10 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>>>                        }
>>>                }
>>>                dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer size %d, "
>>> -                      "offset %d, max layer %d, bo size %ld, slice %d)\n",
>>> +                      "offset %ld, max layer %d, bo size %ld, slice %d)\n",
>>>                         __func__, __LINE__, id, surf.layer_size,
>>> -                     track->cb_color_bo_offset[id] << 8, mslice,
>>> -                     radeon_bo_size(track->cb_color_bo[id]), slice);
>>> +                     (unsigned long)track->cb_color_bo_offset[id] << 8,
>>> +                     mslice, radeon_bo_size(track->cb_color_bo[id]), slice);
>>>                dev_warn(p->dev, "%s:%d problematic surf: (%d %d) (%d %d %d %d %d %d %d)\n",
>>>                         __func__, __LINE__, surf.nbx, surf.nby,
>>>                        surf.mode, surf.bpe, surf.nsamples,
>>> @@ -608,7 +608,7 @@ static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>                return r;
>>>        }
>>>
>>> -     offset = track->db_s_read_offset << 8;
>>> +     offset = (unsigned long)track->db_s_read_offset << 8;
>>>        if (offset & (surf.base_align - 1)) {
>>>                dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
>>>                         __func__, __LINE__, offset, surf.base_align);
>>> @@ -627,7 +627,7 @@ static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>                return -EINVAL;
>>>        }
>>>
>>> -     offset = track->db_s_write_offset << 8;
>>> +     offset = (unsigned long)track->db_s_write_offset << 8;
>>>        if (offset & (surf.base_align - 1)) {
>>>                dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
>>>                         __func__, __LINE__, offset, surf.base_align);
>>> @@ -706,7 +706,7 @@ static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>                return r;
>>>        }
>>>
>>> -     offset = track->db_z_read_offset << 8;
>>> +     offset = (unsigned long)track->db_z_read_offset << 8;
>>>        if (offset & (surf.base_align - 1)) {
>>>                dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
>>>                         __func__, __LINE__, offset, surf.base_align);
>>> @@ -722,7 +722,7 @@ static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>                return -EINVAL;
>>>        }
>>>
>>> -     offset = track->db_z_write_offset << 8;
>>> +     offset = (unsigned long)track->db_z_write_offset << 8;
>>>        if (offset & (surf.base_align - 1)) {
>>>                dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
>>>                         __func__, __LINE__, offset, surf.base_align);

Nikita Zhandarovich July 29, 2024, 5:26 p.m. UTC | #6

Hi,

On 7/29/24 02:23, Christian König wrote:
> Am 26.07.24 um 14:52 schrieb Alex Deucher:
>> On Fri, Jul 26, 2024 at 3:05 AM Christian König
>> <[e-mail]> wrote:
>>> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
>>>> Several cs track offsets (such as 'track->db_s_read_offset')
>>>> either are initialized with or plainly take big enough values that,
>>>> once shifted 8 bits left, may be hit with integer overflow if the
>>>> resulting values end up going over u32 limit.
>>>>
>>>> Some debug prints take this into account (see according dev_warn() in
>>>> evergreen_cs_track_validate_stencil()), even if the actual
>>>> calculated value assigned to local 'offset' variable is missing
>>>> similar proper expansion.
>>>>
>>>> Mitigate the problem by casting the type of right operands to the
>>>> wider type of corresponding left ones in all such cases.
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with static
>>>> analysis tool SVACE.
>>>>
>>>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni
>>>> tiling informations v11")
>>>> Cc: [e-mail]
>>> Well first of all the long cast doesn't makes the value 64bit, it
>>> depends on the architecture.
>>>
>>> Then IIRC the underlying hw can only handle a 32bit address space so
>>> having the offset as long is incorrect to begin with.
>> Evergreen chips support a 36 bit internal address space and NI and
>> newer support a 40 bit one, so this is applicable.
> 
> In that case I strongly suggest that we replace the unsigned long with
> u64 or otherwise we get different behavior on 32 and 64bit machines.
> 
> Regards,
> Christian.
> 

To be clear, I'll prepare v2 patch that changes 'offset' to u64 as well
as the cast of 'track->db_z_read_offset' (and the likes) to u64 too.

On the other note, should I also include casting to wider type of the
expression surf.layer_size * mslice (example down below) in
evergreen_cs_track_validate_cb() and other similar functions? I can't
properly gauge if the result will definitively fit into u32, maybe it
makes sense to expand it as well?

441         }
442
443         offset += surf.layer_size * mslice;
444         if (offset > radeon_bo_size(track->cb_color_bo[id])) {
445                 /* old ddx are broken they allocate bo with w*h*bpp

Regards,
Nikita
>>
>> Alex
>>
>>> And finally that is absolutely not material for stable.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> Signed-off-by: Nikita Zhandarovich <[e-mail]>
>>>> ---
>>>> P.S. While I am not certain that track->cb_color_bo_offset[id]
>>>> actually ends up taking values high enough to cause an overflow,
>>>> nonetheless I thought it prudent to cast it to ulong as well.
>>>>
>>>>    drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>>>>    1 file changed, 9 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>> b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>> index 1fe6e0d883c7..d734d221e2da 100644
>>>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>> @@ -433,7 +433,7 @@ static int evergreen_cs_track_validate_cb(struct
>>>> radeon_cs_parser *p, unsigned i
>>>>                return r;
>>>>        }
>>>>
>>>> -     offset = track->cb_color_bo_offset[id] << 8;
>>>> +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>>>        if (offset & (surf.base_align - 1)) {
>>>>                dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not
>>>> aligned with %ld\n",
>>>>                         __func__, __LINE__, id, offset,
>>>> surf.base_align);
>>>> @@ -455,7 +455,7 @@ static int evergreen_cs_track_validate_cb(struct
>>>> radeon_cs_parser *p, unsigned i
>>>>                                min = surf.nby - 8;
>>>>                        }
>>>>                        bsize = radeon_bo_size(track->cb_color_bo[id]);
>>>> -                     tmp = track->cb_color_bo_offset[id] << 8;
>>>> +                     tmp = (unsigned
>>>> long)track->cb_color_bo_offset[id] << 8;
>>>>                        for (nby = surf.nby; nby > min; nby--) {
>>>>                                size = nby * surf.nbx * surf.bpe *
>>>> surf.nsamples;
>>>>                                if ((tmp + size * mslice) <= bsize) {
>>>> @@ -476,10 +476,10 @@ static int
>>>> evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>>>>                        }
>>>>                }
>>>>                dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer
>>>> size %d, "
>>>> -                      "offset %d, max layer %d, bo size %ld, slice
>>>> %d)\n",
>>>> +                      "offset %ld, max layer %d, bo size %ld, slice
>>>> %d)\n",
>>>>                         __func__, __LINE__, id, surf.layer_size,
>>>> -                     track->cb_color_bo_offset[id] << 8, mslice,
>>>> -                     radeon_bo_size(track->cb_color_bo[id]), slice);
>>>> +                     (unsigned long)track->cb_color_bo_offset[id]
>>>> << 8,
>>>> +                     mslice,
>>>> radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>                dev_warn(p->dev, "%s:%d problematic surf: (%d %d) (%d
>>>> %d %d %d %d %d %d)\n",
>>>>                         __func__, __LINE__, surf.nbx, surf.nby,
>>>>                        surf.mode, surf.bpe, surf.nsamples,
>>>> @@ -608,7 +608,7 @@ static int
>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>                return r;
>>>>        }
>>>>
>>>> -     offset = track->db_s_read_offset << 8;
>>>> +     offset = (unsigned long)track->db_s_read_offset << 8;
>>>>        if (offset & (surf.base_align - 1)) {
>>>>                dev_warn(p->dev, "%s:%d stencil read bo base %ld not
>>>> aligned with %ld\n",
>>>>                         __func__, __LINE__, offset, surf.base_align);
>>>> @@ -627,7 +627,7 @@ static int
>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>                return -EINVAL;
>>>>        }
>>>>
>>>> -     offset = track->db_s_write_offset << 8;
>>>> +     offset = (unsigned long)track->db_s_write_offset << 8;
>>>>        if (offset & (surf.base_align - 1)) {
>>>>                dev_warn(p->dev, "%s:%d stencil write bo base %ld not
>>>> aligned with %ld\n",
>>>>                         __func__, __LINE__, offset, surf.base_align);
>>>> @@ -706,7 +706,7 @@ static int
>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>                return r;
>>>>        }
>>>>
>>>> -     offset = track->db_z_read_offset << 8;
>>>> +     offset = (unsigned long)track->db_z_read_offset << 8;
>>>>        if (offset & (surf.base_align - 1)) {
>>>>                dev_warn(p->dev, "%s:%d stencil read bo base %ld not
>>>> aligned with %ld\n",
>>>>                         __func__, __LINE__, offset, surf.base_align);
>>>> @@ -722,7 +722,7 @@ static int
>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>                return -EINVAL;
>>>>        }
>>>>
>>>> -     offset = track->db_z_write_offset << 8;
>>>> +     offset = (unsigned long)track->db_z_write_offset << 8;
>>>>        if (offset & (surf.base_align - 1)) {
>>>>                dev_warn(p->dev, "%s:%d stencil write bo base %ld not
>>>> aligned with %ld\n",
>>>>                         __func__, __LINE__, offset, surf.base_align);
>

Christian König July 29, 2024, 6:04 p.m. UTC | #7

Am 29.07.24 um 19:26 schrieb Nikita Zhandarovich:
> Hi,
>
> On 7/29/24 02:23, Christian König wrote:
>> Am 26.07.24 um 14:52 schrieb Alex Deucher:
>>> On Fri, Jul 26, 2024 at 3:05 AM Christian König
>>> <[e-mail]> wrote:
>>>> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
>>>>> Several cs track offsets (such as 'track->db_s_read_offset')
>>>>> either are initialized with or plainly take big enough values that,
>>>>> once shifted 8 bits left, may be hit with integer overflow if the
>>>>> resulting values end up going over u32 limit.
>>>>>
>>>>> Some debug prints take this into account (see according dev_warn() in
>>>>> evergreen_cs_track_validate_stencil()), even if the actual
>>>>> calculated value assigned to local 'offset' variable is missing
>>>>> similar proper expansion.
>>>>>
>>>>> Mitigate the problem by casting the type of right operands to the
>>>>> wider type of corresponding left ones in all such cases.
>>>>>
>>>>> Found by Linux Verification Center (linuxtesting.org) with static
>>>>> analysis tool SVACE.
>>>>>
>>>>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni
>>>>> tiling informations v11")
>>>>> Cc: [e-mail]
>>>> Well first of all the long cast doesn't makes the value 64bit, it
>>>> depends on the architecture.
>>>>
>>>> Then IIRC the underlying hw can only handle a 32bit address space so
>>>> having the offset as long is incorrect to begin with.
>>> Evergreen chips support a 36 bit internal address space and NI and
>>> newer support a 40 bit one, so this is applicable.
>> In that case I strongly suggest that we replace the unsigned long with
>> u64 or otherwise we get different behavior on 32 and 64bit machines.
>>
>> Regards,
>> Christian.
>>
> To be clear, I'll prepare v2 patch that changes 'offset' to u64 as well
> as the cast of 'track->db_z_read_offset' (and the likes) to u64 too.
>
> On the other note, should I also include casting to wider type of the
> expression surf.layer_size * mslice (example down below) in
> evergreen_cs_track_validate_cb() and other similar functions? I can't
> properly gauge if the result will definitively fit into u32, maybe it
> makes sense to expand it as well?

The integer overflows caused by shifts are irrelevant and doesn't need 
any fixing in the first place.

The point is rather that we need to avoid multiplication overflows and 
the security problems which come with those.

>
> 441         }
> 442
> 443         offset += surf.layer_size * mslice;

In other words that here needs to be validated correctly.

Regards,
Christian.

> 444         if (offset > radeon_bo_size(track->cb_color_bo[id])) {
> 445                 /* old ddx are broken they allocate bo with w*h*bpp
>
> Regards,
> Nikita
>>> Alex
>>>
>>>> And finally that is absolutely not material for stable.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Signed-off-by: Nikita Zhandarovich <[e-mail]>
>>>>> ---
>>>>> P.S. While I am not certain that track->cb_color_bo_offset[id]
>>>>> actually ends up taking values high enough to cause an overflow,
>>>>> nonetheless I thought it prudent to cast it to ulong as well.
>>>>>
>>>>>     drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>>>>>     1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>> b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>> index 1fe6e0d883c7..d734d221e2da 100644
>>>>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>> @@ -433,7 +433,7 @@ static int evergreen_cs_track_validate_cb(struct
>>>>> radeon_cs_parser *p, unsigned i
>>>>>                 return r;
>>>>>         }
>>>>>
>>>>> -     offset = track->cb_color_bo_offset[id] << 8;
>>>>> +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>                 dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not
>>>>> aligned with %ld\n",
>>>>>                          __func__, __LINE__, id, offset,
>>>>> surf.base_align);
>>>>> @@ -455,7 +455,7 @@ static int evergreen_cs_track_validate_cb(struct
>>>>> radeon_cs_parser *p, unsigned i
>>>>>                                 min = surf.nby - 8;
>>>>>                         }
>>>>>                         bsize = radeon_bo_size(track->cb_color_bo[id]);
>>>>> -                     tmp = track->cb_color_bo_offset[id] << 8;
>>>>> +                     tmp = (unsigned
>>>>> long)track->cb_color_bo_offset[id] << 8;
>>>>>                         for (nby = surf.nby; nby > min; nby--) {
>>>>>                                 size = nby * surf.nbx * surf.bpe *
>>>>> surf.nsamples;
>>>>>                                 if ((tmp + size * mslice) <= bsize) {
>>>>> @@ -476,10 +476,10 @@ static int
>>>>> evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
>>>>>                         }
>>>>>                 }
>>>>>                 dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer
>>>>> size %d, "
>>>>> -                      "offset %d, max layer %d, bo size %ld, slice
>>>>> %d)\n",
>>>>> +                      "offset %ld, max layer %d, bo size %ld, slice
>>>>> %d)\n",
>>>>>                          __func__, __LINE__, id, surf.layer_size,
>>>>> -                     track->cb_color_bo_offset[id] << 8, mslice,
>>>>> -                     radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>> +                     (unsigned long)track->cb_color_bo_offset[id]
>>>>> << 8,
>>>>> +                     mslice,
>>>>> radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>                 dev_warn(p->dev, "%s:%d problematic surf: (%d %d) (%d
>>>>> %d %d %d %d %d %d)\n",
>>>>>                          __func__, __LINE__, surf.nbx, surf.nby,
>>>>>                         surf.mode, surf.bpe, surf.nsamples,
>>>>> @@ -608,7 +608,7 @@ static int
>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>                 return r;
>>>>>         }
>>>>>
>>>>> -     offset = track->db_s_read_offset << 8;
>>>>> +     offset = (unsigned long)track->db_s_read_offset << 8;
>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>                 dev_warn(p->dev, "%s:%d stencil read bo base %ld not
>>>>> aligned with %ld\n",
>>>>>                          __func__, __LINE__, offset, surf.base_align);
>>>>> @@ -627,7 +627,7 @@ static int
>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>                 return -EINVAL;
>>>>>         }
>>>>>
>>>>> -     offset = track->db_s_write_offset << 8;
>>>>> +     offset = (unsigned long)track->db_s_write_offset << 8;
>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>                 dev_warn(p->dev, "%s:%d stencil write bo base %ld not
>>>>> aligned with %ld\n",
>>>>>                          __func__, __LINE__, offset, surf.base_align);
>>>>> @@ -706,7 +706,7 @@ static int
>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>                 return r;
>>>>>         }
>>>>>
>>>>> -     offset = track->db_z_read_offset << 8;
>>>>> +     offset = (unsigned long)track->db_z_read_offset << 8;
>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>                 dev_warn(p->dev, "%s:%d stencil read bo base %ld not
>>>>> aligned with %ld\n",
>>>>>                          __func__, __LINE__, offset, surf.base_align);
>>>>> @@ -722,7 +722,7 @@ static int
>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>                 return -EINVAL;
>>>>>         }
>>>>>
>>>>> -     offset = track->db_z_write_offset << 8;
>>>>> +     offset = (unsigned long)track->db_z_write_offset << 8;
>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>                 dev_warn(p->dev, "%s:%d stencil write bo base %ld not
>>>>> aligned with %ld\n",
>>>>>                          __func__, __LINE__, offset, surf.base_align);

Christian König July 29, 2024, 6:12 p.m. UTC | #8

Am 29.07.24 um 20:04 schrieb Christian König:
> Am 29.07.24 um 19:26 schrieb Nikita Zhandarovich:
>> Hi,
>>
>> On 7/29/24 02:23, Christian König wrote:
>>> Am 26.07.24 um 14:52 schrieb Alex Deucher:
>>>> On Fri, Jul 26, 2024 at 3:05 AM Christian König
>>>> <[e-mail]> wrote:
>>>>> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
>>>>>> Several cs track offsets (such as 'track->db_s_read_offset')
>>>>>> either are initialized with or plainly take big enough values that,
>>>>>> once shifted 8 bits left, may be hit with integer overflow if the
>>>>>> resulting values end up going over u32 limit.
>>>>>>
>>>>>> Some debug prints take this into account (see according 
>>>>>> dev_warn() in
>>>>>> evergreen_cs_track_validate_stencil()), even if the actual
>>>>>> calculated value assigned to local 'offset' variable is missing
>>>>>> similar proper expansion.
>>>>>>
>>>>>> Mitigate the problem by casting the type of right operands to the
>>>>>> wider type of corresponding left ones in all such cases.
>>>>>>
>>>>>> Found by Linux Verification Center (linuxtesting.org) with static
>>>>>> analysis tool SVACE.
>>>>>>
>>>>>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni
>>>>>> tiling informations v11")
>>>>>> Cc: [e-mail]
>>>>> Well first of all the long cast doesn't makes the value 64bit, it
>>>>> depends on the architecture.
>>>>>
>>>>> Then IIRC the underlying hw can only handle a 32bit address space so
>>>>> having the offset as long is incorrect to begin with.
>>>> Evergreen chips support a 36 bit internal address space and NI and
>>>> newer support a 40 bit one, so this is applicable.
>>> In that case I strongly suggest that we replace the unsigned long with
>>> u64 or otherwise we get different behavior on 32 and 64bit machines.
>>>
>>> Regards,
>>> Christian.
>>>
>> To be clear, I'll prepare v2 patch that changes 'offset' to u64 as well
>> as the cast of 'track->db_z_read_offset' (and the likes) to u64 too.
>>
>> On the other note, should I also include casting to wider type of the
>> expression surf.layer_size * mslice (example down below) in
>> evergreen_cs_track_validate_cb() and other similar functions? I can't
>> properly gauge if the result will definitively fit into u32, maybe it
>> makes sense to expand it as well?
>
> The integer overflows caused by shifts are irrelevant and doesn't need 
> any fixing in the first place.

Wait a second.

Thinking more about it the integer overflows are actually necessary 
because that is exactly what happens in the hardware as well.

If you don't overflow those shifts you actually create a security 
problem because the HW the might access at a different offset then you 
calculated here.

We need to use something like a mask or use lower_32_bits() here.

Regards,
Christian.

>
> The point is rather that we need to avoid multiplication overflows and 
> the security problems which come with those.
>
>>
>> 441         }
>> 442
>> 443         offset += surf.layer_size * mslice;
>
> In other words that here needs to be validated correctly.
>
> Regards,
> Christian.
>
>> 444         if (offset > radeon_bo_size(track->cb_color_bo[id])) {
>> 445                 /* old ddx are broken they allocate bo with w*h*bpp
>>
>> Regards,
>> Nikita
>>>> Alex
>>>>
>>>>> And finally that is absolutely not material for stable.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> Signed-off-by: Nikita Zhandarovich <[e-mail]>
>>>>>> ---
>>>>>> P.S. While I am not certain that track->cb_color_bo_offset[id]
>>>>>> actually ends up taking values high enough to cause an overflow,
>>>>>> nonetheless I thought it prudent to cast it to ulong as well.
>>>>>>
>>>>>>     drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>>>>>>     1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>> b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>> index 1fe6e0d883c7..d734d221e2da 100644
>>>>>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>> @@ -433,7 +433,7 @@ static int evergreen_cs_track_validate_cb(struct
>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>                 return r;
>>>>>>         }
>>>>>>
>>>>>> -     offset = track->cb_color_bo_offset[id] << 8;
>>>>>> +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>>                 dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not
>>>>>> aligned with %ld\n",
>>>>>>                          __func__, __LINE__, id, offset,
>>>>>> surf.base_align);
>>>>>> @@ -455,7 +455,7 @@ static int evergreen_cs_track_validate_cb(struct
>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>                                 min = surf.nby - 8;
>>>>>>                         }
>>>>>>                         bsize = 
>>>>>> radeon_bo_size(track->cb_color_bo[id]);
>>>>>> -                     tmp = track->cb_color_bo_offset[id] << 8;
>>>>>> +                     tmp = (unsigned
>>>>>> long)track->cb_color_bo_offset[id] << 8;
>>>>>>                         for (nby = surf.nby; nby > min; nby--) {
>>>>>>                                 size = nby * surf.nbx * surf.bpe *
>>>>>> surf.nsamples;
>>>>>>                                 if ((tmp + size * mslice) <= 
>>>>>> bsize) {
>>>>>> @@ -476,10 +476,10 @@ static int
>>>>>> evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, 
>>>>>> unsigned i
>>>>>>                         }
>>>>>>                 }
>>>>>>                 dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer
>>>>>> size %d, "
>>>>>> -                      "offset %d, max layer %d, bo size %ld, slice
>>>>>> %d)\n",
>>>>>> +                      "offset %ld, max layer %d, bo size %ld, slice
>>>>>> %d)\n",
>>>>>>                          __func__, __LINE__, id, surf.layer_size,
>>>>>> -                     track->cb_color_bo_offset[id] << 8, mslice,
>>>>>> - radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>> +                     (unsigned long)track->cb_color_bo_offset[id]
>>>>>> << 8,
>>>>>> +                     mslice,
>>>>>> radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>                 dev_warn(p->dev, "%s:%d problematic surf: (%d %d) 
>>>>>> (%d
>>>>>> %d %d %d %d %d %d)\n",
>>>>>>                          __func__, __LINE__, surf.nbx, surf.nby,
>>>>>>                         surf.mode, surf.bpe, surf.nsamples,
>>>>>> @@ -608,7 +608,7 @@ static int
>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>                 return r;
>>>>>>         }
>>>>>>
>>>>>> -     offset = track->db_s_read_offset << 8;
>>>>>> +     offset = (unsigned long)track->db_s_read_offset << 8;
>>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>>                 dev_warn(p->dev, "%s:%d stencil read bo base %ld not
>>>>>> aligned with %ld\n",
>>>>>>                          __func__, __LINE__, offset, 
>>>>>> surf.base_align);
>>>>>> @@ -627,7 +627,7 @@ static int
>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>                 return -EINVAL;
>>>>>>         }
>>>>>>
>>>>>> -     offset = track->db_s_write_offset << 8;
>>>>>> +     offset = (unsigned long)track->db_s_write_offset << 8;
>>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>>                 dev_warn(p->dev, "%s:%d stencil write bo base %ld 
>>>>>> not
>>>>>> aligned with %ld\n",
>>>>>>                          __func__, __LINE__, offset, 
>>>>>> surf.base_align);
>>>>>> @@ -706,7 +706,7 @@ static int
>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>                 return r;
>>>>>>         }
>>>>>>
>>>>>> -     offset = track->db_z_read_offset << 8;
>>>>>> +     offset = (unsigned long)track->db_z_read_offset << 8;
>>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>>                 dev_warn(p->dev, "%s:%d stencil read bo base %ld not
>>>>>> aligned with %ld\n",
>>>>>>                          __func__, __LINE__, offset, 
>>>>>> surf.base_align);
>>>>>> @@ -722,7 +722,7 @@ static int
>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>                 return -EINVAL;
>>>>>>         }
>>>>>>
>>>>>> -     offset = track->db_z_write_offset << 8;
>>>>>> +     offset = (unsigned long)track->db_z_write_offset << 8;
>>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>>                 dev_warn(p->dev, "%s:%d stencil write bo base %ld 
>>>>>> not
>>>>>> aligned with %ld\n",
>>>>>>                          __func__, __LINE__, offset, 
>>>>>> surf.base_align);
>

Nikita Zhandarovich July 30, 2024, 5:36 p.m. UTC | #9

On 7/29/24 11:12, Christian König wrote:
> Am 29.07.24 um 20:04 schrieb Christian König:
>> Am 29.07.24 um 19:26 schrieb Nikita Zhandarovich:
>>> Hi,
>>>
>>> On 7/29/24 02:23, Christian König wrote:
>>>> Am 26.07.24 um 14:52 schrieb Alex Deucher:
>>>>> On Fri, Jul 26, 2024 at 3:05 AM Christian König
>>>>> <[e-mail]> wrote:
>>>>>> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
>>>>>>> Several cs track offsets (such as 'track->db_s_read_offset')
>>>>>>> either are initialized with or plainly take big enough values that,
>>>>>>> once shifted 8 bits left, may be hit with integer overflow if the
>>>>>>> resulting values end up going over u32 limit.
>>>>>>>
>>>>>>> Some debug prints take this into account (see according
>>>>>>> dev_warn() in
>>>>>>> evergreen_cs_track_validate_stencil()), even if the actual
>>>>>>> calculated value assigned to local 'offset' variable is missing
>>>>>>> similar proper expansion.
>>>>>>>
>>>>>>> Mitigate the problem by casting the type of right operands to the
>>>>>>> wider type of corresponding left ones in all such cases.
>>>>>>>
>>>>>>> Found by Linux Verification Center (linuxtesting.org) with static
>>>>>>> analysis tool SVACE.
>>>>>>>
>>>>>>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni
>>>>>>> tiling informations v11")
>>>>>>> Cc: [e-mail]
>>>>>> Well first of all the long cast doesn't makes the value 64bit, it
>>>>>> depends on the architecture.
>>>>>>
>>>>>> Then IIRC the underlying hw can only handle a 32bit address space so
>>>>>> having the offset as long is incorrect to begin with.
>>>>> Evergreen chips support a 36 bit internal address space and NI and
>>>>> newer support a 40 bit one, so this is applicable.
>>>> In that case I strongly suggest that we replace the unsigned long with
>>>> u64 or otherwise we get different behavior on 32 and 64bit machines.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>> To be clear, I'll prepare v2 patch that changes 'offset' to u64 as well
>>> as the cast of 'track->db_z_read_offset' (and the likes) to u64 too.
>>>
>>> On the other note, should I also include casting to wider type of the
>>> expression surf.layer_size * mslice (example down below) in
>>> evergreen_cs_track_validate_cb() and other similar functions? I can't
>>> properly gauge if the result will definitively fit into u32, maybe it
>>> makes sense to expand it as well?
>>
>> The integer overflows caused by shifts are irrelevant and doesn't need
>> any fixing in the first place.
> 
> Wait a second.
> 
> Thinking more about it the integer overflows are actually necessary
> because that is exactly what happens in the hardware as well.
> 
> If you don't overflow those shifts you actually create a security
> problem because the HW the might access at a different offset then you
> calculated here.
> 
> We need to use something like a mask or use lower_32_bits() here.

Christian,

My apologies, I may be getting a bit confused here.

If integer overflows caused by shifts are predictable and constitute
normal behavior in this case, and there is no need to "fix" them, does
it still make sense to use any mitigations at all, i.e. masks or macros?
Leaving these shifts to u32 variables as they are now will achieve the
same result as, for example, doing something along the lines of:

offset = lower_32_bits((u64)track->cb_color_bo_offset[id] << 8);

which seems clunky and unnecessary, even if it suppresses some static
analyzer triggers (and that seems overboard).
Or am I missing something obvious here?
> 
> Regards,
> Christian.
> 
>>
>> The point is rather that we need to avoid multiplication overflows and
>> the security problems which come with those.
>>
>>>
>>> 441         }
>>> 442
>>> 443         offset += surf.layer_size * mslice;
>>
>> In other words that here needs to be validated correctly.
>>

Agreed, I think either casting right operand to u64 (once 'offset' is
also changed from unsigned long to u64) or using mul_u32_u32() here and
in other places should suffice.

>> Regards,
>> Christian.
>>
>>> 444         if (offset > radeon_bo_size(track->cb_color_bo[id])) {
>>> 445                 /* old ddx are broken they allocate bo with w*h*bpp
>>>
>>> Regards,
>>> Nikita
>>>>> Alex
>>>>>
>>>>>> And finally that is absolutely not material for stable.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> Signed-off-by: Nikita Zhandarovich <[e-mail]>
>>>>>>> ---
>>>>>>> P.S. While I am not certain that track->cb_color_bo_offset[id]
>>>>>>> actually ends up taking values high enough to cause an overflow,
>>>>>>> nonetheless I thought it prudent to cast it to ulong as well.
>>>>>>>
>>>>>>>     drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>>>>>>>     1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>> b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>> index 1fe6e0d883c7..d734d221e2da 100644
>>>>>>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>> @@ -433,7 +433,7 @@ static int evergreen_cs_track_validate_cb(struct
>>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>>                 return r;
>>>>>>>         }
>>>>>>>
>>>>>>> -     offset = track->cb_color_bo_offset[id] << 8;
>>>>>>> +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>>>                 dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not
>>>>>>> aligned with %ld\n",
>>>>>>>                          __func__, __LINE__, id, offset,
>>>>>>> surf.base_align);
>>>>>>> @@ -455,7 +455,7 @@ static int evergreen_cs_track_validate_cb(struct
>>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>>                                 min = surf.nby - 8;
>>>>>>>                         }
>>>>>>>                         bsize =
>>>>>>> radeon_bo_size(track->cb_color_bo[id]);
>>>>>>> -                     tmp = track->cb_color_bo_offset[id] << 8;
>>>>>>> +                     tmp = (unsigned
>>>>>>> long)track->cb_color_bo_offset[id] << 8;
>>>>>>>                         for (nby = surf.nby; nby > min; nby--) {
>>>>>>>                                 size = nby * surf.nbx * surf.bpe *
>>>>>>> surf.nsamples;
>>>>>>>                                 if ((tmp + size * mslice) <=
>>>>>>> bsize) {
>>>>>>> @@ -476,10 +476,10 @@ static int
>>>>>>> evergreen_cs_track_validate_cb(struct radeon_cs_parser *p,
>>>>>>> unsigned i
>>>>>>>                         }
>>>>>>>                 }
>>>>>>>                 dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer
>>>>>>> size %d, "
>>>>>>> -                      "offset %d, max layer %d, bo size %ld, slice
>>>>>>> %d)\n",
>>>>>>> +                      "offset %ld, max layer %d, bo size %ld, slice
>>>>>>> %d)\n",
>>>>>>>                          __func__, __LINE__, id, surf.layer_size,
>>>>>>> -                     track->cb_color_bo_offset[id] << 8, mslice,
>>>>>>> - radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>> +                     (unsigned long)track->cb_color_bo_offset[id]
>>>>>>> << 8,
>>>>>>> +                     mslice,
>>>>>>> radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>>                 dev_warn(p->dev, "%s:%d problematic surf: (%d %d)
>>>>>>> (%d
>>>>>>> %d %d %d %d %d %d)\n",
>>>>>>>                          __func__, __LINE__, surf.nbx, surf.nby,
>>>>>>>                         surf.mode, surf.bpe, surf.nsamples,
>>>>>>> @@ -608,7 +608,7 @@ static int
>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>>                 return r;
>>>>>>>         }
>>>>>>>
>>>>>>> -     offset = track->db_s_read_offset << 8;
>>>>>>> +     offset = (unsigned long)track->db_s_read_offset << 8;
>>>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>>>                 dev_warn(p->dev, "%s:%d stencil read bo base %ld not
>>>>>>> aligned with %ld\n",
>>>>>>>                          __func__, __LINE__, offset,
>>>>>>> surf.base_align);
>>>>>>> @@ -627,7 +627,7 @@ static int
>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>>                 return -EINVAL;
>>>>>>>         }
>>>>>>>
>>>>>>> -     offset = track->db_s_write_offset << 8;
>>>>>>> +     offset = (unsigned long)track->db_s_write_offset << 8;
>>>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>>>                 dev_warn(p->dev, "%s:%d stencil write bo base %ld
>>>>>>> not
>>>>>>> aligned with %ld\n",
>>>>>>>                          __func__, __LINE__, offset,
>>>>>>> surf.base_align);
>>>>>>> @@ -706,7 +706,7 @@ static int
>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>>                 return r;
>>>>>>>         }
>>>>>>>
>>>>>>> -     offset = track->db_z_read_offset << 8;
>>>>>>> +     offset = (unsigned long)track->db_z_read_offset << 8;
>>>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>>>                 dev_warn(p->dev, "%s:%d stencil read bo base %ld not
>>>>>>> aligned with %ld\n",
>>>>>>>                          __func__, __LINE__, offset,
>>>>>>> surf.base_align);
>>>>>>> @@ -722,7 +722,7 @@ static int
>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>>                 return -EINVAL;
>>>>>>>         }
>>>>>>>
>>>>>>> -     offset = track->db_z_write_offset << 8;
>>>>>>> +     offset = (unsigned long)track->db_z_write_offset << 8;
>>>>>>>         if (offset & (surf.base_align - 1)) {
>>>>>>>                 dev_warn(p->dev, "%s:%d stencil write bo base %ld
>>>>>>> not
>>>>>>> aligned with %ld\n",
>>>>>>>                          __func__, __LINE__, offset,
>>>>>>> surf.base_align);
>>
>

Christian König July 31, 2024, 6:56 a.m. UTC | #10

Am 30.07.24 um 19:36 schrieb Nikita Zhandarovich:
> On 7/29/24 11:12, Christian König wrote:
>> Am 29.07.24 um 20:04 schrieb Christian König:
>>> Am 29.07.24 um 19:26 schrieb Nikita Zhandarovich:
>>>> Hi,
>>>>
>>>> On 7/29/24 02:23, Christian König wrote:
>>>>> Am 26.07.24 um 14:52 schrieb Alex Deucher:
>>>>>> On Fri, Jul 26, 2024 at 3:05 AM Christian König
>>>>>> <[e-mail]> wrote:
>>>>>>> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
>>>>>>>> Several cs track offsets (such as 'track->db_s_read_offset')
>>>>>>>> either are initialized with or plainly take big enough values that,
>>>>>>>> once shifted 8 bits left, may be hit with integer overflow if the
>>>>>>>> resulting values end up going over u32 limit.
>>>>>>>>
>>>>>>>> Some debug prints take this into account (see according
>>>>>>>> dev_warn() in
>>>>>>>> evergreen_cs_track_validate_stencil()), even if the actual
>>>>>>>> calculated value assigned to local 'offset' variable is missing
>>>>>>>> similar proper expansion.
>>>>>>>>
>>>>>>>> Mitigate the problem by casting the type of right operands to the
>>>>>>>> wider type of corresponding left ones in all such cases.
>>>>>>>>
>>>>>>>> Found by Linux Verification Center (linuxtesting.org) with static
>>>>>>>> analysis tool SVACE.
>>>>>>>>
>>>>>>>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni
>>>>>>>> tiling informations v11")
>>>>>>>> Cc: [e-mail]
>>>>>>> Well first of all the long cast doesn't makes the value 64bit, it
>>>>>>> depends on the architecture.
>>>>>>>
>>>>>>> Then IIRC the underlying hw can only handle a 32bit address space so
>>>>>>> having the offset as long is incorrect to begin with.
>>>>>> Evergreen chips support a 36 bit internal address space and NI and
>>>>>> newer support a 40 bit one, so this is applicable.
>>>>> In that case I strongly suggest that we replace the unsigned long with
>>>>> u64 or otherwise we get different behavior on 32 and 64bit machines.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>> To be clear, I'll prepare v2 patch that changes 'offset' to u64 as well
>>>> as the cast of 'track->db_z_read_offset' (and the likes) to u64 too.
>>>>
>>>> On the other note, should I also include casting to wider type of the
>>>> expression surf.layer_size * mslice (example down below) in
>>>> evergreen_cs_track_validate_cb() and other similar functions? I can't
>>>> properly gauge if the result will definitively fit into u32, maybe it
>>>> makes sense to expand it as well?
>>> The integer overflows caused by shifts are irrelevant and doesn't need
>>> any fixing in the first place.
>> Wait a second.
>>
>> Thinking more about it the integer overflows are actually necessary
>> because that is exactly what happens in the hardware as well.
>>
>> If you don't overflow those shifts you actually create a security
>> problem because the HW the might access at a different offset then you
>> calculated here.
>>
>> We need to use something like a mask or use lower_32_bits() here.
> Christian,
>
> My apologies, I may be getting a bit confused here.
>
> If integer overflows caused by shifts are predictable and constitute
> normal behavior in this case, and there is no need to "fix" them, does
> it still make sense to use any mitigations at all, i.e. masks or macros?

Well you stumbled over that somehow, so some automated checker things 
that this is a bad idea.

> Leaving these shifts to u32 variables as they are now will achieve the
> same result as, for example, doing something along the lines of:
>
> offset = lower_32_bits((u64)track->cb_color_bo_offset[id] << 8);
>
> which seems clunky and unnecessary, even if it suppresses some static
> analyzer triggers (and that seems overboard).
> Or am I missing something obvious here?

No, it's just about suppressing the static checker warnings.

I'm also not 100% sure how that old hw works. Alex mentioned that it is 
using 36bits internally.

So it could be that we need to switch to using u64 here. I need to 
double check that with Alex.

But using unsigned long is certainly incorrect cause we then get 
different behavior based on the CPU architecture.

Thanks for pointing this out,
Christian.

>> Regards,
>> Christian.
>>
>>> The point is rather that we need to avoid multiplication overflows and
>>> the security problems which come with those.
>>>
>>>> 441         }
>>>> 442
>>>> 443         offset += surf.layer_size * mslice;
>>> In other words that here needs to be validated correctly.
>>>
> Agreed, I think either casting right operand to u64 (once 'offset' is
> also changed from unsigned long to u64) or using mul_u32_u32() here and
> in other places should suffice.
>
>>> Regards,
>>> Christian.
>>>
>>>> 444         if (offset > radeon_bo_size(track->cb_color_bo[id])) {
>>>> 445                 /* old ddx are broken they allocate bo with w*h*bpp
>>>>
>>>> Regards,
>>>> Nikita
>>>>>> Alex
>>>>>>
>>>>>>> And finally that is absolutely not material for stable.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>>> Signed-off-by: Nikita Zhandarovich <[e-mail]>
>>>>>>>> ---
>>>>>>>> P.S. While I am not certain that track->cb_color_bo_offset[id]
>>>>>>>> actually ends up taking values high enough to cause an overflow,
>>>>>>>> nonetheless I thought it prudent to cast it to ulong as well.
>>>>>>>>
>>>>>>>>      drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>>>>>>>>      1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>> b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>> index 1fe6e0d883c7..d734d221e2da 100644
>>>>>>>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>> @@ -433,7 +433,7 @@ static int evergreen_cs_track_validate_cb(struct
>>>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>>>                  return r;
>>>>>>>>          }
>>>>>>>>
>>>>>>>> -     offset = track->cb_color_bo_offset[id] << 8;
>>>>>>>> +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>                  dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not
>>>>>>>> aligned with %ld\n",
>>>>>>>>                           __func__, __LINE__, id, offset,
>>>>>>>> surf.base_align);
>>>>>>>> @@ -455,7 +455,7 @@ static int evergreen_cs_track_validate_cb(struct
>>>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>>>                                  min = surf.nby - 8;
>>>>>>>>                          }
>>>>>>>>                          bsize =
>>>>>>>> radeon_bo_size(track->cb_color_bo[id]);
>>>>>>>> -                     tmp = track->cb_color_bo_offset[id] << 8;
>>>>>>>> +                     tmp = (unsigned
>>>>>>>> long)track->cb_color_bo_offset[id] << 8;
>>>>>>>>                          for (nby = surf.nby; nby > min; nby--) {
>>>>>>>>                                  size = nby * surf.nbx * surf.bpe *
>>>>>>>> surf.nsamples;
>>>>>>>>                                  if ((tmp + size * mslice) <=
>>>>>>>> bsize) {
>>>>>>>> @@ -476,10 +476,10 @@ static int
>>>>>>>> evergreen_cs_track_validate_cb(struct radeon_cs_parser *p,
>>>>>>>> unsigned i
>>>>>>>>                          }
>>>>>>>>                  }
>>>>>>>>                  dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer
>>>>>>>> size %d, "
>>>>>>>> -                      "offset %d, max layer %d, bo size %ld, slice
>>>>>>>> %d)\n",
>>>>>>>> +                      "offset %ld, max layer %d, bo size %ld, slice
>>>>>>>> %d)\n",
>>>>>>>>                           __func__, __LINE__, id, surf.layer_size,
>>>>>>>> -                     track->cb_color_bo_offset[id] << 8, mslice,
>>>>>>>> - radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>>> +                     (unsigned long)track->cb_color_bo_offset[id]
>>>>>>>> << 8,
>>>>>>>> +                     mslice,
>>>>>>>> radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>>>                  dev_warn(p->dev, "%s:%d problematic surf: (%d %d)
>>>>>>>> (%d
>>>>>>>> %d %d %d %d %d %d)\n",
>>>>>>>>                           __func__, __LINE__, surf.nbx, surf.nby,
>>>>>>>>                          surf.mode, surf.bpe, surf.nsamples,
>>>>>>>> @@ -608,7 +608,7 @@ static int
>>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>>>                  return r;
>>>>>>>>          }
>>>>>>>>
>>>>>>>> -     offset = track->db_s_read_offset << 8;
>>>>>>>> +     offset = (unsigned long)track->db_s_read_offset << 8;
>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil read bo base %ld not
>>>>>>>> aligned with %ld\n",
>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>> surf.base_align);
>>>>>>>> @@ -627,7 +627,7 @@ static int
>>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>>>                  return -EINVAL;
>>>>>>>>          }
>>>>>>>>
>>>>>>>> -     offset = track->db_s_write_offset << 8;
>>>>>>>> +     offset = (unsigned long)track->db_s_write_offset << 8;
>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil write bo base %ld
>>>>>>>> not
>>>>>>>> aligned with %ld\n",
>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>> surf.base_align);
>>>>>>>> @@ -706,7 +706,7 @@ static int
>>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>>>                  return r;
>>>>>>>>          }
>>>>>>>>
>>>>>>>> -     offset = track->db_z_read_offset << 8;
>>>>>>>> +     offset = (unsigned long)track->db_z_read_offset << 8;
>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil read bo base %ld not
>>>>>>>> aligned with %ld\n",
>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>> surf.base_align);
>>>>>>>> @@ -722,7 +722,7 @@ static int
>>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>>>                  return -EINVAL;
>>>>>>>>          }
>>>>>>>>
>>>>>>>> -     offset = track->db_z_write_offset << 8;
>>>>>>>> +     offset = (unsigned long)track->db_z_write_offset << 8;
>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil write bo base %ld
>>>>>>>> not
>>>>>>>> aligned with %ld\n",
>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>> surf.base_align);

Nikita Zhandarovich Aug. 5, 2024, 7:34 a.m. UTC | #11

On 7/30/24 23:56, Christian König wrote:
> Am 30.07.24 um 19:36 schrieb Nikita Zhandarovich:
>> On 7/29/24 11:12, Christian König wrote:
>>> Am 29.07.24 um 20:04 schrieb Christian König:
>>>> Am 29.07.24 um 19:26 schrieb Nikita Zhandarovich:
>>>>> Hi,
>>>>>
>>>>> On 7/29/24 02:23, Christian König wrote:
>>>>>> Am 26.07.24 um 14:52 schrieb Alex Deucher:
>>>>>>> On Fri, Jul 26, 2024 at 3:05 AM Christian König
>>>>>>> <[e-mail]> wrote:
>>>>>>>> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
>>>>>>>>> Several cs track offsets (such as 'track->db_s_read_offset')
>>>>>>>>> either are initialized with or plainly take big enough values
>>>>>>>>> that,
>>>>>>>>> once shifted 8 bits left, may be hit with integer overflow if the
>>>>>>>>> resulting values end up going over u32 limit.
>>>>>>>>>
>>>>>>>>> Some debug prints take this into account (see according
>>>>>>>>> dev_warn() in
>>>>>>>>> evergreen_cs_track_validate_stencil()), even if the actual
>>>>>>>>> calculated value assigned to local 'offset' variable is missing
>>>>>>>>> similar proper expansion.
>>>>>>>>>
>>>>>>>>> Mitigate the problem by casting the type of right operands to the
>>>>>>>>> wider type of corresponding left ones in all such cases.
>>>>>>>>>
>>>>>>>>> Found by Linux Verification Center (linuxtesting.org) with static
>>>>>>>>> analysis tool SVACE.
>>>>>>>>>
>>>>>>>>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni
>>>>>>>>> tiling informations v11")
>>>>>>>>> Cc: [e-mail]
>>>>>>>> Well first of all the long cast doesn't makes the value 64bit, it
>>>>>>>> depends on the architecture.
>>>>>>>>
>>>>>>>> Then IIRC the underlying hw can only handle a 32bit address
>>>>>>>> space so
>>>>>>>> having the offset as long is incorrect to begin with.
>>>>>>> Evergreen chips support a 36 bit internal address space and NI and
>>>>>>> newer support a 40 bit one, so this is applicable.
>>>>>> In that case I strongly suggest that we replace the unsigned long
>>>>>> with
>>>>>> u64 or otherwise we get different behavior on 32 and 64bit machines.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>> To be clear, I'll prepare v2 patch that changes 'offset' to u64 as
>>>>> well
>>>>> as the cast of 'track->db_z_read_offset' (and the likes) to u64 too.
>>>>>
>>>>> On the other note, should I also include casting to wider type of the
>>>>> expression surf.layer_size * mslice (example down below) in
>>>>> evergreen_cs_track_validate_cb() and other similar functions? I can't
>>>>> properly gauge if the result will definitively fit into u32, maybe it
>>>>> makes sense to expand it as well?
>>>> The integer overflows caused by shifts are irrelevant and doesn't need
>>>> any fixing in the first place.
>>> Wait a second.
>>>
>>> Thinking more about it the integer overflows are actually necessary
>>> because that is exactly what happens in the hardware as well.
>>>
>>> If you don't overflow those shifts you actually create a security
>>> problem because the HW the might access at a different offset then you
>>> calculated here.
>>>
>>> We need to use something like a mask or use lower_32_bits() here.
>> Christian,
>>
>> My apologies, I may be getting a bit confused here.
>>
>> If integer overflows caused by shifts are predictable and constitute
>> normal behavior in this case, and there is no need to "fix" them, does
>> it still make sense to use any mitigations at all, i.e. masks or macros?
> 
> Well you stumbled over that somehow, so some automated checker things
> that this is a bad idea.
> 
>> Leaving these shifts to u32 variables as they are now will achieve the
>> same result as, for example, doing something along the lines of:
>>
>> offset = lower_32_bits((u64)track->cb_color_bo_offset[id] << 8);
>>
>> which seems clunky and unnecessary, even if it suppresses some static
>> analyzer triggers (and that seems overboard).
>> Or am I missing something obvious here?
> 
> No, it's just about suppressing the static checker warnings.
> 
> I'm also not 100% sure how that old hw works. Alex mentioned that it is
> using 36bits internally.
> 
> So it could be that we need to switch to using u64 here. I need to
> double check that with Alex.
> 
> But using unsigned long is certainly incorrect cause we then get
> different behavior based on the CPU architecture.
> 
> Thanks for pointing this out,
> Christian.
> 

Hi,

Christian, did you get a chance to go over hw specifics with Alex?
I'd really like to get on with v2 patch but I can't really start
properly if I don't know what (and how) exactly to fix.

I am also hesitant to split the fix into parts and I'd rather do the
whole int overflow mitigation in one set.

Thanks,
Nikita

>>> Regards,
>>> Christian.
>>>
>>>> The point is rather that we need to avoid multiplication overflows and
>>>> the security problems which come with those.
>>>>
>>>>> 441         }
>>>>> 442
>>>>> 443         offset += surf.layer_size * mslice;
>>>> In other words that here needs to be validated correctly.
>>>>
>> Agreed, I think either casting right operand to u64 (once 'offset' is
>> also changed from unsigned long to u64) or using mul_u32_u32() here and
>> in other places should suffice.
>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> 444         if (offset > radeon_bo_size(track->cb_color_bo[id])) {
>>>>> 445                 /* old ddx are broken they allocate bo with
>>>>> w*h*bpp
>>>>>
>>>>> Regards,
>>>>> Nikita
>>>>>>> Alex
>>>>>>>
>>>>>>>> And finally that is absolutely not material for stable.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>> Signed-off-by: Nikita Zhandarovich <[e-mail]>
>>>>>>>>> ---
>>>>>>>>> P.S. While I am not certain that track->cb_color_bo_offset[id]
>>>>>>>>> actually ends up taking values high enough to cause an overflow,
>>>>>>>>> nonetheless I thought it prudent to cast it to ulong as well.
>>>>>>>>>
>>>>>>>>>      drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>>>>>>>>>      1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>> b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>> index 1fe6e0d883c7..d734d221e2da 100644
>>>>>>>>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>> @@ -433,7 +433,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_cb(struct
>>>>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>>>>                  return r;
>>>>>>>>>          }
>>>>>>>>>
>>>>>>>>> -     offset = track->cb_color_bo_offset[id] << 8;
>>>>>>>>> +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>>                  dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not
>>>>>>>>> aligned with %ld\n",
>>>>>>>>>                           __func__, __LINE__, id, offset,
>>>>>>>>> surf.base_align);
>>>>>>>>> @@ -455,7 +455,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_cb(struct
>>>>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>>>>                                  min = surf.nby - 8;
>>>>>>>>>                          }
>>>>>>>>>                          bsize =
>>>>>>>>> radeon_bo_size(track->cb_color_bo[id]);
>>>>>>>>> -                     tmp = track->cb_color_bo_offset[id] << 8;
>>>>>>>>> +                     tmp = (unsigned
>>>>>>>>> long)track->cb_color_bo_offset[id] << 8;
>>>>>>>>>                          for (nby = surf.nby; nby > min; nby--) {
>>>>>>>>>                                  size = nby * surf.nbx *
>>>>>>>>> surf.bpe *
>>>>>>>>> surf.nsamples;
>>>>>>>>>                                  if ((tmp + size * mslice) <=
>>>>>>>>> bsize) {
>>>>>>>>> @@ -476,10 +476,10 @@ static int
>>>>>>>>> evergreen_cs_track_validate_cb(struct radeon_cs_parser *p,
>>>>>>>>> unsigned i
>>>>>>>>>                          }
>>>>>>>>>                  }
>>>>>>>>>                  dev_warn(p->dev, "%s:%d cb[%d] bo too small
>>>>>>>>> (layer
>>>>>>>>> size %d, "
>>>>>>>>> -                      "offset %d, max layer %d, bo size %ld,
>>>>>>>>> slice
>>>>>>>>> %d)\n",
>>>>>>>>> +                      "offset %ld, max layer %d, bo size %ld,
>>>>>>>>> slice
>>>>>>>>> %d)\n",
>>>>>>>>>                           __func__, __LINE__, id, surf.layer_size,
>>>>>>>>> -                     track->cb_color_bo_offset[id] << 8, mslice,
>>>>>>>>> - radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>>>> +                     (unsigned long)track->cb_color_bo_offset[id]
>>>>>>>>> << 8,
>>>>>>>>> +                     mslice,
>>>>>>>>> radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>>>>                  dev_warn(p->dev, "%s:%d problematic surf: (%d %d)
>>>>>>>>> (%d
>>>>>>>>> %d %d %d %d %d %d)\n",
>>>>>>>>>                           __func__, __LINE__, surf.nbx, surf.nby,
>>>>>>>>>                          surf.mode, surf.bpe, surf.nsamples,
>>>>>>>>> @@ -608,7 +608,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>>>>                  return r;
>>>>>>>>>          }
>>>>>>>>>
>>>>>>>>> -     offset = track->db_s_read_offset << 8;
>>>>>>>>> +     offset = (unsigned long)track->db_s_read_offset << 8;
>>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil read bo base
>>>>>>>>> %ld not
>>>>>>>>> aligned with %ld\n",
>>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>>> surf.base_align);
>>>>>>>>> @@ -627,7 +627,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>>>>                  return -EINVAL;
>>>>>>>>>          }
>>>>>>>>>
>>>>>>>>> -     offset = track->db_s_write_offset << 8;
>>>>>>>>> +     offset = (unsigned long)track->db_s_write_offset << 8;
>>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil write bo base %ld
>>>>>>>>> not
>>>>>>>>> aligned with %ld\n",
>>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>>> surf.base_align);
>>>>>>>>> @@ -706,7 +706,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>>>>                  return r;
>>>>>>>>>          }
>>>>>>>>>
>>>>>>>>> -     offset = track->db_z_read_offset << 8;
>>>>>>>>> +     offset = (unsigned long)track->db_z_read_offset << 8;
>>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil read bo base
>>>>>>>>> %ld not
>>>>>>>>> aligned with %ld\n",
>>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>>> surf.base_align);
>>>>>>>>> @@ -722,7 +722,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>>>>                  return -EINVAL;
>>>>>>>>>          }
>>>>>>>>>
>>>>>>>>> -     offset = track->db_z_write_offset << 8;
>>>>>>>>> +     offset = (unsigned long)track->db_z_write_offset << 8;
>>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil write bo base %ld
>>>>>>>>> not
>>>>>>>>> aligned with %ld\n",
>>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>>> surf.base_align);
>

Alex Deucher Aug. 5, 2024, 2:36 p.m. UTC | #12

On Mon, Aug 5, 2024 at 3:34 AM Nikita Zhandarovich
<[e-mail]> wrote:
>
>
>
> On 7/30/24 23:56, Christian König wrote:
> > Am 30.07.24 um 19:36 schrieb Nikita Zhandarovich:
> >> On 7/29/24 11:12, Christian König wrote:
> >>> Am 29.07.24 um 20:04 schrieb Christian König:
> >>>> Am 29.07.24 um 19:26 schrieb Nikita Zhandarovich:
> >>>>> Hi,
> >>>>>
> >>>>> On 7/29/24 02:23, Christian König wrote:
> >>>>>> Am 26.07.24 um 14:52 schrieb Alex Deucher:
> >>>>>>> On Fri, Jul 26, 2024 at 3:05 AM Christian König
> >>>>>>> <[e-mail]> wrote:
> >>>>>>>> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
> >>>>>>>>> Several cs track offsets (such as 'track->db_s_read_offset')
> >>>>>>>>> either are initialized with or plainly take big enough values
> >>>>>>>>> that,
> >>>>>>>>> once shifted 8 bits left, may be hit with integer overflow if the
> >>>>>>>>> resulting values end up going over u32 limit.
> >>>>>>>>>
> >>>>>>>>> Some debug prints take this into account (see according
> >>>>>>>>> dev_warn() in
> >>>>>>>>> evergreen_cs_track_validate_stencil()), even if the actual
> >>>>>>>>> calculated value assigned to local 'offset' variable is missing
> >>>>>>>>> similar proper expansion.
> >>>>>>>>>
> >>>>>>>>> Mitigate the problem by casting the type of right operands to the
> >>>>>>>>> wider type of corresponding left ones in all such cases.
> >>>>>>>>>
> >>>>>>>>> Found by Linux Verification Center (linuxtesting.org) with static
> >>>>>>>>> analysis tool SVACE.
> >>>>>>>>>
> >>>>>>>>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni
> >>>>>>>>> tiling informations v11")
> >>>>>>>>> Cc: [e-mail]
> >>>>>>>> Well first of all the long cast doesn't makes the value 64bit, it
> >>>>>>>> depends on the architecture.
> >>>>>>>>
> >>>>>>>> Then IIRC the underlying hw can only handle a 32bit address
> >>>>>>>> space so
> >>>>>>>> having the offset as long is incorrect to begin with.
> >>>>>>> Evergreen chips support a 36 bit internal address space and NI and
> >>>>>>> newer support a 40 bit one, so this is applicable.
> >>>>>> In that case I strongly suggest that we replace the unsigned long
> >>>>>> with
> >>>>>> u64 or otherwise we get different behavior on 32 and 64bit machines.
> >>>>>>
> >>>>>> Regards,
> >>>>>> Christian.
> >>>>>>
> >>>>> To be clear, I'll prepare v2 patch that changes 'offset' to u64 as
> >>>>> well
> >>>>> as the cast of 'track->db_z_read_offset' (and the likes) to u64 too.
> >>>>>
> >>>>> On the other note, should I also include casting to wider type of the
> >>>>> expression surf.layer_size * mslice (example down below) in
> >>>>> evergreen_cs_track_validate_cb() and other similar functions? I can't
> >>>>> properly gauge if the result will definitively fit into u32, maybe it
> >>>>> makes sense to expand it as well?
> >>>> The integer overflows caused by shifts are irrelevant and doesn't need
> >>>> any fixing in the first place.
> >>> Wait a second.
> >>>
> >>> Thinking more about it the integer overflows are actually necessary
> >>> because that is exactly what happens in the hardware as well.
> >>>
> >>> If you don't overflow those shifts you actually create a security
> >>> problem because the HW the might access at a different offset then you
> >>> calculated here.
> >>>
> >>> We need to use something like a mask or use lower_32_bits() here.
> >> Christian,
> >>
> >> My apologies, I may be getting a bit confused here.
> >>
> >> If integer overflows caused by shifts are predictable and constitute
> >> normal behavior in this case, and there is no need to "fix" them, does
> >> it still make sense to use any mitigations at all, i.e. masks or macros?
> >
> > Well you stumbled over that somehow, so some automated checker things
> > that this is a bad idea.
> >
> >> Leaving these shifts to u32 variables as they are now will achieve the
> >> same result as, for example, doing something along the lines of:
> >>
> >> offset = lower_32_bits((u64)track->cb_color_bo_offset[id] << 8);
> >>
> >> which seems clunky and unnecessary, even if it suppresses some static
> >> analyzer triggers (and that seems overboard).
> >> Or am I missing something obvious here?
> >
> > No, it's just about suppressing the static checker warnings.
> >
> > I'm also not 100% sure how that old hw works. Alex mentioned that it is
> > using 36bits internally.
> >
> > So it could be that we need to switch to using u64 here. I need to
> > double check that with Alex.
> >
> > But using unsigned long is certainly incorrect cause we then get
> > different behavior based on the CPU architecture.
> >
> > Thanks for pointing this out,
> > Christian.> >
>
> Hi,
>
> Christian, did you get a chance to go over hw specifics with Alex?
> I'd really like to get on with v2 patch but I can't really start
> properly if I don't know what (and how) exactly to fix.
>
> I am also hesitant to split the fix into parts and I'd rather do the
> whole int overflow mitigation in one set.

The HW has a 36 or 40 bit internal address space depending on the
generation, so we should be using 64 bit data types.  The addresses
stored in the registers are 256 byte aligned so they can fit the
address into a 32 bit register.

Alex


>
> Thanks,
> Nikita
>
> >>> Regards,
> >>> Christian.
> >>>
> >>>> The point is rather that we need to avoid multiplication overflows and
> >>>> the security problems which come with those.
> >>>>
> >>>>> 441         }
> >>>>> 442
> >>>>> 443         offset += surf.layer_size * mslice;
> >>>> In other words that here needs to be validated correctly.
> >>>>
> >> Agreed, I think either casting right operand to u64 (once 'offset' is
> >> also changed from unsigned long to u64) or using mul_u32_u32() here and
> >> in other places should suffice.
> >>
> >>>> Regards,
> >>>> Christian.
> >>>>
> >>>>> 444         if (offset > radeon_bo_size(track->cb_color_bo[id])) {
> >>>>> 445                 /* old ddx are broken they allocate bo with
> >>>>> w*h*bpp
> >>>>>
> >>>>> Regards,
> >>>>> Nikita
> >>>>>>> Alex
> >>>>>>>
> >>>>>>>> And finally that is absolutely not material for stable.
> >>>>>>>>
> >>>>>>>> Regards,
> >>>>>>>> Christian.
> >>>>>>>>
> >>>>>>>>> Signed-off-by: Nikita Zhandarovich <[e-mail]>
> >>>>>>>>> ---
> >>>>>>>>> P.S. While I am not certain that track->cb_color_bo_offset[id]
> >>>>>>>>> actually ends up taking values high enough to cause an overflow,
> >>>>>>>>> nonetheless I thought it prudent to cast it to ulong as well.
> >>>>>>>>>
> >>>>>>>>>      drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
> >>>>>>>>>      1 file changed, 9 insertions(+), 9 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c
> >>>>>>>>> b/drivers/gpu/drm/radeon/evergreen_cs.c
> >>>>>>>>> index 1fe6e0d883c7..d734d221e2da 100644
> >>>>>>>>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
> >>>>>>>>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
> >>>>>>>>> @@ -433,7 +433,7 @@ static int
> >>>>>>>>> evergreen_cs_track_validate_cb(struct
> >>>>>>>>> radeon_cs_parser *p, unsigned i
> >>>>>>>>>                  return r;
> >>>>>>>>>          }
> >>>>>>>>>
> >>>>>>>>> -     offset = track->cb_color_bo_offset[id] << 8;
> >>>>>>>>> +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
> >>>>>>>>>          if (offset & (surf.base_align - 1)) {
> >>>>>>>>>                  dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not
> >>>>>>>>> aligned with %ld\n",
> >>>>>>>>>                           __func__, __LINE__, id, offset,
> >>>>>>>>> surf.base_align);
> >>>>>>>>> @@ -455,7 +455,7 @@ static int
> >>>>>>>>> evergreen_cs_track_validate_cb(struct
> >>>>>>>>> radeon_cs_parser *p, unsigned i
> >>>>>>>>>                                  min = surf.nby - 8;
> >>>>>>>>>                          }
> >>>>>>>>>                          bsize =
> >>>>>>>>> radeon_bo_size(track->cb_color_bo[id]);
> >>>>>>>>> -                     tmp = track->cb_color_bo_offset[id] << 8;
> >>>>>>>>> +                     tmp = (unsigned
> >>>>>>>>> long)track->cb_color_bo_offset[id] << 8;
> >>>>>>>>>                          for (nby = surf.nby; nby > min; nby--) {
> >>>>>>>>>                                  size = nby * surf.nbx *
> >>>>>>>>> surf.bpe *
> >>>>>>>>> surf.nsamples;
> >>>>>>>>>                                  if ((tmp + size * mslice) <=
> >>>>>>>>> bsize) {
> >>>>>>>>> @@ -476,10 +476,10 @@ static int
> >>>>>>>>> evergreen_cs_track_validate_cb(struct radeon_cs_parser *p,
> >>>>>>>>> unsigned i
> >>>>>>>>>                          }
> >>>>>>>>>                  }
> >>>>>>>>>                  dev_warn(p->dev, "%s:%d cb[%d] bo too small
> >>>>>>>>> (layer
> >>>>>>>>> size %d, "
> >>>>>>>>> -                      "offset %d, max layer %d, bo size %ld,
> >>>>>>>>> slice
> >>>>>>>>> %d)\n",
> >>>>>>>>> +                      "offset %ld, max layer %d, bo size %ld,
> >>>>>>>>> slice
> >>>>>>>>> %d)\n",
> >>>>>>>>>                           __func__, __LINE__, id, surf.layer_size,
> >>>>>>>>> -                     track->cb_color_bo_offset[id] << 8, mslice,
> >>>>>>>>> - radeon_bo_size(track->cb_color_bo[id]), slice);
> >>>>>>>>> +                     (unsigned long)track->cb_color_bo_offset[id]
> >>>>>>>>> << 8,
> >>>>>>>>> +                     mslice,
> >>>>>>>>> radeon_bo_size(track->cb_color_bo[id]), slice);
> >>>>>>>>>                  dev_warn(p->dev, "%s:%d problematic surf: (%d %d)
> >>>>>>>>> (%d
> >>>>>>>>> %d %d %d %d %d %d)\n",
> >>>>>>>>>                           __func__, __LINE__, surf.nbx, surf.nby,
> >>>>>>>>>                          surf.mode, surf.bpe, surf.nsamples,
> >>>>>>>>> @@ -608,7 +608,7 @@ static int
> >>>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
> >>>>>>>>>                  return r;
> >>>>>>>>>          }
> >>>>>>>>>
> >>>>>>>>> -     offset = track->db_s_read_offset << 8;
> >>>>>>>>> +     offset = (unsigned long)track->db_s_read_offset << 8;
> >>>>>>>>>          if (offset & (surf.base_align - 1)) {
> >>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil read bo base
> >>>>>>>>> %ld not
> >>>>>>>>> aligned with %ld\n",
> >>>>>>>>>                           __func__, __LINE__, offset,
> >>>>>>>>> surf.base_align);
> >>>>>>>>> @@ -627,7 +627,7 @@ static int
> >>>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
> >>>>>>>>>                  return -EINVAL;
> >>>>>>>>>          }
> >>>>>>>>>
> >>>>>>>>> -     offset = track->db_s_write_offset << 8;
> >>>>>>>>> +     offset = (unsigned long)track->db_s_write_offset << 8;
> >>>>>>>>>          if (offset & (surf.base_align - 1)) {
> >>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil write bo base %ld
> >>>>>>>>> not
> >>>>>>>>> aligned with %ld\n",
> >>>>>>>>>                           __func__, __LINE__, offset,
> >>>>>>>>> surf.base_align);
> >>>>>>>>> @@ -706,7 +706,7 @@ static int
> >>>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
> >>>>>>>>>                  return r;
> >>>>>>>>>          }
> >>>>>>>>>
> >>>>>>>>> -     offset = track->db_z_read_offset << 8;
> >>>>>>>>> +     offset = (unsigned long)track->db_z_read_offset << 8;
> >>>>>>>>>          if (offset & (surf.base_align - 1)) {
> >>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil read bo base
> >>>>>>>>> %ld not
> >>>>>>>>> aligned with %ld\n",
> >>>>>>>>>                           __func__, __LINE__, offset,
> >>>>>>>>> surf.base_align);
> >>>>>>>>> @@ -722,7 +722,7 @@ static int
> >>>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
> >>>>>>>>>                  return -EINVAL;
> >>>>>>>>>          }
> >>>>>>>>>
> >>>>>>>>> -     offset = track->db_z_write_offset << 8;
> >>>>>>>>> +     offset = (unsigned long)track->db_z_write_offset << 8;
> >>>>>>>>>          if (offset & (surf.base_align - 1)) {
> >>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil write bo base %ld
> >>>>>>>>> not
> >>>>>>>>> aligned with %ld\n",
> >>>>>>>>>                           __func__, __LINE__, offset,
> >>>>>>>>> surf.base_align);
> >

Christian König Aug. 19, 2024, 8:23 a.m. UTC | #13

Am 05.08.24 um 09:34 schrieb Nikita Zhandarovich:
>
> On 7/30/24 23:56, Christian König wrote:
>> Am 30.07.24 um 19:36 schrieb Nikita Zhandarovich:
>>> On 7/29/24 11:12, Christian König wrote:
>>>> Am 29.07.24 um 20:04 schrieb Christian König:
>>>>> Am 29.07.24 um 19:26 schrieb Nikita Zhandarovich:
>>>>>> Hi,
>>>>>>
>>>>>> On 7/29/24 02:23, Christian König wrote:
>>>>>>> Am 26.07.24 um 14:52 schrieb Alex Deucher:
>>>>>>>> On Fri, Jul 26, 2024 at 3:05 AM Christian König
>>>>>>>> <[e-mail]> wrote:
>>>>>>>>> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
>>>>>>>>>> Several cs track offsets (such as 'track->db_s_read_offset')
>>>>>>>>>> either are initialized with or plainly take big enough values
>>>>>>>>>> that,
>>>>>>>>>> once shifted 8 bits left, may be hit with integer overflow if the
>>>>>>>>>> resulting values end up going over u32 limit.
>>>>>>>>>>
>>>>>>>>>> Some debug prints take this into account (see according
>>>>>>>>>> dev_warn() in
>>>>>>>>>> evergreen_cs_track_validate_stencil()), even if the actual
>>>>>>>>>> calculated value assigned to local 'offset' variable is missing
>>>>>>>>>> similar proper expansion.
>>>>>>>>>>
>>>>>>>>>> Mitigate the problem by casting the type of right operands to the
>>>>>>>>>> wider type of corresponding left ones in all such cases.
>>>>>>>>>>
>>>>>>>>>> Found by Linux Verification Center (linuxtesting.org) with static
>>>>>>>>>> analysis tool SVACE.
>>>>>>>>>>
>>>>>>>>>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni
>>>>>>>>>> tiling informations v11")
>>>>>>>>>> Cc: [e-mail]
>>>>>>>>> Well first of all the long cast doesn't makes the value 64bit, it
>>>>>>>>> depends on the architecture.
>>>>>>>>>
>>>>>>>>> Then IIRC the underlying hw can only handle a 32bit address
>>>>>>>>> space so
>>>>>>>>> having the offset as long is incorrect to begin with.
>>>>>>>> Evergreen chips support a 36 bit internal address space and NI and
>>>>>>>> newer support a 40 bit one, so this is applicable.
>>>>>>> In that case I strongly suggest that we replace the unsigned long
>>>>>>> with
>>>>>>> u64 or otherwise we get different behavior on 32 and 64bit machines.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>> To be clear, I'll prepare v2 patch that changes 'offset' to u64 as
>>>>>> well
>>>>>> as the cast of 'track->db_z_read_offset' (and the likes) to u64 too.
>>>>>>
>>>>>> On the other note, should I also include casting to wider type of the
>>>>>> expression surf.layer_size * mslice (example down below) in
>>>>>> evergreen_cs_track_validate_cb() and other similar functions? I can't
>>>>>> properly gauge if the result will definitively fit into u32, maybe it
>>>>>> makes sense to expand it as well?
>>>>> The integer overflows caused by shifts are irrelevant and doesn't need
>>>>> any fixing in the first place.
>>>> Wait a second.
>>>>
>>>> Thinking more about it the integer overflows are actually necessary
>>>> because that is exactly what happens in the hardware as well.
>>>>
>>>> If you don't overflow those shifts you actually create a security
>>>> problem because the HW the might access at a different offset then you
>>>> calculated here.
>>>>
>>>> We need to use something like a mask or use lower_32_bits() here.
>>> Christian,
>>>
>>> My apologies, I may be getting a bit confused here.
>>>
>>> If integer overflows caused by shifts are predictable and constitute
>>> normal behavior in this case, and there is no need to "fix" them, does
>>> it still make sense to use any mitigations at all, i.e. masks or macros?
>> Well you stumbled over that somehow, so some automated checker things
>> that this is a bad idea.
>>
>>> Leaving these shifts to u32 variables as they are now will achieve the
>>> same result as, for example, doing something along the lines of:
>>>
>>> offset = lower_32_bits((u64)track->cb_color_bo_offset[id] << 8);
>>>
>>> which seems clunky and unnecessary, even if it suppresses some static
>>> analyzer triggers (and that seems overboard).
>>> Or am I missing something obvious here?
>> No, it's just about suppressing the static checker warnings.
>>
>> I'm also not 100% sure how that old hw works. Alex mentioned that it is
>> using 36bits internally.
>>
>> So it could be that we need to switch to using u64 here. I need to
>> double check that with Alex.
>>
>> But using unsigned long is certainly incorrect cause we then get
>> different behavior based on the CPU architecture.
>>
>> Thanks for pointing this out,
>> Christian.
>>
> Hi,
>
> Christian, did you get a chance to go over hw specifics with Alex?

Sorry I'm just back from vacation. Give me a week to dig through my 
mails and talk with Alex.

Thanks,
Christian.

> I'd really like to get on with v2 patch but I can't really start
> properly if I don't know what (and how) exactly to fix.
>
> I am also hesitant to split the fix into parts and I'd rather do the
> whole int overflow mitigation in one set.
>
> Thanks,
> Nikita
>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> The point is rather that we need to avoid multiplication overflows and
>>>>> the security problems which come with those.
>>>>>
>>>>>> 441         }
>>>>>> 442
>>>>>> 443         offset += surf.layer_size * mslice;
>>>>> In other words that here needs to be validated correctly.
>>>>>
>>> Agreed, I think either casting right operand to u64 (once 'offset' is
>>> also changed from unsigned long to u64) or using mul_u32_u32() here and
>>> in other places should suffice.
>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> 444         if (offset > radeon_bo_size(track->cb_color_bo[id])) {
>>>>>> 445                 /* old ddx are broken they allocate bo with
>>>>>> w*h*bpp
>>>>>>
>>>>>> Regards,
>>>>>> Nikita
>>>>>>>> Alex
>>>>>>>>
>>>>>>>>> And finally that is absolutely not material for stable.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Christian.
>>>>>>>>>
>>>>>>>>>> Signed-off-by: Nikita Zhandarovich <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>> P.S. While I am not certain that track->cb_color_bo_offset[id]
>>>>>>>>>> actually ends up taking values high enough to cause an overflow,
>>>>>>>>>> nonetheless I thought it prudent to cast it to ulong as well.
>>>>>>>>>>
>>>>>>>>>>       drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>>>>>>>>>>       1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>>> b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>>> index 1fe6e0d883c7..d734d221e2da 100644
>>>>>>>>>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>>> @@ -433,7 +433,7 @@ static int
>>>>>>>>>> evergreen_cs_track_validate_cb(struct
>>>>>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>>>>>                   return r;
>>>>>>>>>>           }
>>>>>>>>>>
>>>>>>>>>> -     offset = track->cb_color_bo_offset[id] << 8;
>>>>>>>>>> +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>>>>>>>>>           if (offset & (surf.base_align - 1)) {
>>>>>>>>>>                   dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not
>>>>>>>>>> aligned with %ld\n",
>>>>>>>>>>                            __func__, __LINE__, id, offset,
>>>>>>>>>> surf.base_align);
>>>>>>>>>> @@ -455,7 +455,7 @@ static int
>>>>>>>>>> evergreen_cs_track_validate_cb(struct
>>>>>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>>>>>                                   min = surf.nby - 8;
>>>>>>>>>>                           }
>>>>>>>>>>                           bsize =
>>>>>>>>>> radeon_bo_size(track->cb_color_bo[id]);
>>>>>>>>>> -                     tmp = track->cb_color_bo_offset[id] << 8;
>>>>>>>>>> +                     tmp = (unsigned
>>>>>>>>>> long)track->cb_color_bo_offset[id] << 8;
>>>>>>>>>>                           for (nby = surf.nby; nby > min; nby--) {
>>>>>>>>>>                                   size = nby * surf.nbx *
>>>>>>>>>> surf.bpe *
>>>>>>>>>> surf.nsamples;
>>>>>>>>>>                                   if ((tmp + size * mslice) <=
>>>>>>>>>> bsize) {
>>>>>>>>>> @@ -476,10 +476,10 @@ static int
>>>>>>>>>> evergreen_cs_track_validate_cb(struct radeon_cs_parser *p,
>>>>>>>>>> unsigned i
>>>>>>>>>>                           }
>>>>>>>>>>                   }
>>>>>>>>>>                   dev_warn(p->dev, "%s:%d cb[%d] bo too small
>>>>>>>>>> (layer
>>>>>>>>>> size %d, "
>>>>>>>>>> -                      "offset %d, max layer %d, bo size %ld,
>>>>>>>>>> slice
>>>>>>>>>> %d)\n",
>>>>>>>>>> +                      "offset %ld, max layer %d, bo size %ld,
>>>>>>>>>> slice
>>>>>>>>>> %d)\n",
>>>>>>>>>>                            __func__, __LINE__, id, surf.layer_size,
>>>>>>>>>> -                     track->cb_color_bo_offset[id] << 8, mslice,
>>>>>>>>>> - radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>>>>> +                     (unsigned long)track->cb_color_bo_offset[id]
>>>>>>>>>> << 8,
>>>>>>>>>> +                     mslice,
>>>>>>>>>> radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>>>>>                   dev_warn(p->dev, "%s:%d problematic surf: (%d %d)
>>>>>>>>>> (%d
>>>>>>>>>> %d %d %d %d %d %d)\n",
>>>>>>>>>>                            __func__, __LINE__, surf.nbx, surf.nby,
>>>>>>>>>>                           surf.mode, surf.bpe, surf.nsamples,
>>>>>>>>>> @@ -608,7 +608,7 @@ static int
>>>>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>>>>>                   return r;
>>>>>>>>>>           }
>>>>>>>>>>
>>>>>>>>>> -     offset = track->db_s_read_offset << 8;
>>>>>>>>>> +     offset = (unsigned long)track->db_s_read_offset << 8;
>>>>>>>>>>           if (offset & (surf.base_align - 1)) {
>>>>>>>>>>                   dev_warn(p->dev, "%s:%d stencil read bo base
>>>>>>>>>> %ld not
>>>>>>>>>> aligned with %ld\n",
>>>>>>>>>>                            __func__, __LINE__, offset,
>>>>>>>>>> surf.base_align);
>>>>>>>>>> @@ -627,7 +627,7 @@ static int
>>>>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>>>>>                   return -EINVAL;
>>>>>>>>>>           }
>>>>>>>>>>
>>>>>>>>>> -     offset = track->db_s_write_offset << 8;
>>>>>>>>>> +     offset = (unsigned long)track->db_s_write_offset << 8;
>>>>>>>>>>           if (offset & (surf.base_align - 1)) {
>>>>>>>>>>                   dev_warn(p->dev, "%s:%d stencil write bo base %ld
>>>>>>>>>> not
>>>>>>>>>> aligned with %ld\n",
>>>>>>>>>>                            __func__, __LINE__, offset,
>>>>>>>>>> surf.base_align);
>>>>>>>>>> @@ -706,7 +706,7 @@ static int
>>>>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>>>>>                   return r;
>>>>>>>>>>           }
>>>>>>>>>>
>>>>>>>>>> -     offset = track->db_z_read_offset << 8;
>>>>>>>>>> +     offset = (unsigned long)track->db_z_read_offset << 8;
>>>>>>>>>>           if (offset & (surf.base_align - 1)) {
>>>>>>>>>>                   dev_warn(p->dev, "%s:%d stencil read bo base
>>>>>>>>>> %ld not
>>>>>>>>>> aligned with %ld\n",
>>>>>>>>>>                            __func__, __LINE__, offset,
>>>>>>>>>> surf.base_align);
>>>>>>>>>> @@ -722,7 +722,7 @@ static int
>>>>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>>>>>                   return -EINVAL;
>>>>>>>>>>           }
>>>>>>>>>>
>>>>>>>>>> -     offset = track->db_z_write_offset << 8;
>>>>>>>>>> +     offset = (unsigned long)track->db_z_write_offset << 8;
>>>>>>>>>>           if (offset & (surf.base_align - 1)) {
>>>>>>>>>>                   dev_warn(p->dev, "%s:%d stencil write bo base %ld
>>>>>>>>>> not
>>>>>>>>>> aligned with %ld\n",
>>>>>>>>>>                            __func__, __LINE__, offset,
>>>>>>>>>> surf.base_align);

Patch

diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c b/drivers/gpu/drm/radeon/evergreen_cs.c
index 1fe6e0d883c7..d734d221e2da 100644
--- a/drivers/gpu/drm/radeon/evergreen_cs.c
+++ b/drivers/gpu/drm/radeon/evergreen_cs.c
@@ -433,7 +433,7 @@  static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
 		return r;
 	}
 
-	offset = track->cb_color_bo_offset[id] << 8;
+	offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
 	if (offset & (surf.base_align - 1)) {
 		dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not aligned with %ld\n",
 			 __func__, __LINE__, id, offset, surf.base_align);
@@ -455,7 +455,7 @@  static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
 				min = surf.nby - 8;
 			}
 			bsize = radeon_bo_size(track->cb_color_bo[id]);
-			tmp = track->cb_color_bo_offset[id] << 8;
+			tmp = (unsigned long)track->cb_color_bo_offset[id] << 8;
 			for (nby = surf.nby; nby > min; nby--) {
 				size = nby * surf.nbx * surf.bpe * surf.nsamples;
 				if ((tmp + size * mslice) <= bsize) {
@@ -476,10 +476,10 @@  static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
 			}
 		}
 		dev_warn(p->dev, "%s:%d cb[%d] bo too small (layer size %d, "
-			 "offset %d, max layer %d, bo size %ld, slice %d)\n",
+			 "offset %ld, max layer %d, bo size %ld, slice %d)\n",
 			 __func__, __LINE__, id, surf.layer_size,
-			track->cb_color_bo_offset[id] << 8, mslice,
-			radeon_bo_size(track->cb_color_bo[id]), slice);
+			(unsigned long)track->cb_color_bo_offset[id] << 8,
+			mslice,	radeon_bo_size(track->cb_color_bo[id]), slice);
 		dev_warn(p->dev, "%s:%d problematic surf: (%d %d) (%d %d %d %d %d %d %d)\n",
 			 __func__, __LINE__, surf.nbx, surf.nby,
 			surf.mode, surf.bpe, surf.nsamples,
@@ -608,7 +608,7 @@  static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
 		return r;
 	}
 
-	offset = track->db_s_read_offset << 8;
+	offset = (unsigned long)track->db_s_read_offset << 8;
 	if (offset & (surf.base_align - 1)) {
 		dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
 			 __func__, __LINE__, offset, surf.base_align);
@@ -627,7 +627,7 @@  static int evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
 		return -EINVAL;
 	}
 
-	offset = track->db_s_write_offset << 8;
+	offset = (unsigned long)track->db_s_write_offset << 8;
 	if (offset & (surf.base_align - 1)) {
 		dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
 			 __func__, __LINE__, offset, surf.base_align);
@@ -706,7 +706,7 @@  static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
 		return r;
 	}
 
-	offset = track->db_z_read_offset << 8;
+	offset = (unsigned long)track->db_z_read_offset << 8;
 	if (offset & (surf.base_align - 1)) {
 		dev_warn(p->dev, "%s:%d stencil read bo base %ld not aligned with %ld\n",
 			 __func__, __LINE__, offset, surf.base_align);
@@ -722,7 +722,7 @@  static int evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
 		return -EINVAL;
 	}
 
-	offset = track->db_z_write_offset << 8;
+	offset = (unsigned long)track->db_z_write_offset << 8;
 	if (offset & (surf.base_align - 1)) {
 		dev_warn(p->dev, "%s:%d stencil write bo base %ld not aligned with %ld\n",
 			 __func__, __LINE__, offset, surf.base_align);