[PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value()
Alex Deucher
alexdeucher at gmail.com
Tue Aug 25 15:53:25 UTC 2020
On Tue, Aug 25, 2020 at 7:21 AM Dan Carpenter <dan.carpenter at oracle.com> wrote:
>
> The values for "se_num" and "sh_num" come from the user in the ioctl.
> They can be in the 0-255 range but if they're more than
> AMDGPU_GFX_MAX_SE (4) or AMDGPU_GFX_MAX_SH_PER_SE (2) then it results in
> an out of bounds read.
>
> I split this function into to two to make the error handling simpler.
>
> Fixes: dd5dfa61b4ff ("drm/amdgpu: refine si_read_register")
> Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>
Good catch. This is more defensive, but It's a much simpler check to
validate these in the caller. See the attached patch.
Alex
> ---
> drivers/gpu/drm/amd/amdgpu/si.c | 157 +++++++++++++++++---------------
> 1 file changed, 85 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/si.c b/drivers/gpu/drm/amd/amdgpu/si.c
> index e330884edd19..ccf39a6932ae 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si.c
> +++ b/drivers/gpu/drm/amd/amdgpu/si.c
> @@ -1051,81 +1051,90 @@ static struct amdgpu_allowed_register_entry si_allowed_read_registers[] = {
> {PA_SC_RASTER_CONFIG, true},
> };
>
> -static uint32_t si_get_register_value(struct amdgpu_device *adev,
> - bool indexed, u32 se_num,
> - u32 sh_num, u32 reg_offset)
> -{
> - if (indexed) {
> - uint32_t val;
> - unsigned se_idx = (se_num == 0xffffffff) ? 0 : se_num;
> - unsigned sh_idx = (sh_num == 0xffffffff) ? 0 : sh_num;
> -
> - switch (reg_offset) {
> - case mmCC_RB_BACKEND_DISABLE:
> - return adev->gfx.config.rb_config[se_idx][sh_idx].rb_backend_disable;
> - case mmGC_USER_RB_BACKEND_DISABLE:
> - return adev->gfx.config.rb_config[se_idx][sh_idx].user_rb_backend_disable;
> - case mmPA_SC_RASTER_CONFIG:
> - return adev->gfx.config.rb_config[se_idx][sh_idx].raster_config;
> - }
> +static int si_get_register_value_indexed(struct amdgpu_device *adev,
> + u32 se_num, u32 sh_num,
> + u32 reg_offset, u32 *value)
> +{
> + unsigned se_idx = (se_num == 0xffffffff) ? 0 : se_num;
> + unsigned sh_idx = (sh_num == 0xffffffff) ? 0 : sh_num;
>
> - mutex_lock(&adev->grbm_idx_mutex);
> - if (se_num != 0xffffffff || sh_num != 0xffffffff)
> - amdgpu_gfx_select_se_sh(adev, se_num, sh_num, 0xffffffff);
> + if (se_idx >= AMDGPU_GFX_MAX_SE ||
> + sh_idx >= AMDGPU_GFX_MAX_SH_PER_SE)
> + return -EINVAL;
>
> - val = RREG32(reg_offset);
> + switch (reg_offset) {
> + case mmCC_RB_BACKEND_DISABLE:
> + *value = adev->gfx.config.rb_config[se_idx][sh_idx].rb_backend_disable;
> + return 0;
> + case mmGC_USER_RB_BACKEND_DISABLE:
> + *value = adev->gfx.config.rb_config[se_idx][sh_idx].user_rb_backend_disable;
> + return 0;
> + case mmPA_SC_RASTER_CONFIG:
> + *value = adev->gfx.config.rb_config[se_idx][sh_idx].raster_config;
> + return 0;
> + }
>
> - if (se_num != 0xffffffff || sh_num != 0xffffffff)
> - amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff);
> - mutex_unlock(&adev->grbm_idx_mutex);
> - return val;
> - } else {
> - unsigned idx;
> -
> - switch (reg_offset) {
> - case mmGB_ADDR_CONFIG:
> - return adev->gfx.config.gb_addr_config;
> - case mmMC_ARB_RAMCFG:
> - return adev->gfx.config.mc_arb_ramcfg;
> - case mmGB_TILE_MODE0:
> - case mmGB_TILE_MODE1:
> - case mmGB_TILE_MODE2:
> - case mmGB_TILE_MODE3:
> - case mmGB_TILE_MODE4:
> - case mmGB_TILE_MODE5:
> - case mmGB_TILE_MODE6:
> - case mmGB_TILE_MODE7:
> - case mmGB_TILE_MODE8:
> - case mmGB_TILE_MODE9:
> - case mmGB_TILE_MODE10:
> - case mmGB_TILE_MODE11:
> - case mmGB_TILE_MODE12:
> - case mmGB_TILE_MODE13:
> - case mmGB_TILE_MODE14:
> - case mmGB_TILE_MODE15:
> - case mmGB_TILE_MODE16:
> - case mmGB_TILE_MODE17:
> - case mmGB_TILE_MODE18:
> - case mmGB_TILE_MODE19:
> - case mmGB_TILE_MODE20:
> - case mmGB_TILE_MODE21:
> - case mmGB_TILE_MODE22:
> - case mmGB_TILE_MODE23:
> - case mmGB_TILE_MODE24:
> - case mmGB_TILE_MODE25:
> - case mmGB_TILE_MODE26:
> - case mmGB_TILE_MODE27:
> - case mmGB_TILE_MODE28:
> - case mmGB_TILE_MODE29:
> - case mmGB_TILE_MODE30:
> - case mmGB_TILE_MODE31:
> - idx = (reg_offset - mmGB_TILE_MODE0);
> - return adev->gfx.config.tile_mode_array[idx];
> - default:
> - return RREG32(reg_offset);
> - }
> + mutex_lock(&adev->grbm_idx_mutex);
> + if (se_num != 0xffffffff || sh_num != 0xffffffff)
> + amdgpu_gfx_select_se_sh(adev, se_num, sh_num, 0xffffffff);
> +
> + *value = RREG32(reg_offset);
> +
> + if (se_num != 0xffffffff || sh_num != 0xffffffff)
> + amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff);
> + mutex_unlock(&adev->grbm_idx_mutex);
> + return 0;
> +}
> +
> +static uint32_t si_get_register_value(struct amdgpu_device *adev,
> + u32 reg_offset)
> +{
> + unsigned idx;
> +
> + switch (reg_offset) {
> + case mmGB_ADDR_CONFIG:
> + return adev->gfx.config.gb_addr_config;
> + case mmMC_ARB_RAMCFG:
> + return adev->gfx.config.mc_arb_ramcfg;
> + case mmGB_TILE_MODE0:
> + case mmGB_TILE_MODE1:
> + case mmGB_TILE_MODE2:
> + case mmGB_TILE_MODE3:
> + case mmGB_TILE_MODE4:
> + case mmGB_TILE_MODE5:
> + case mmGB_TILE_MODE6:
> + case mmGB_TILE_MODE7:
> + case mmGB_TILE_MODE8:
> + case mmGB_TILE_MODE9:
> + case mmGB_TILE_MODE10:
> + case mmGB_TILE_MODE11:
> + case mmGB_TILE_MODE12:
> + case mmGB_TILE_MODE13:
> + case mmGB_TILE_MODE14:
> + case mmGB_TILE_MODE15:
> + case mmGB_TILE_MODE16:
> + case mmGB_TILE_MODE17:
> + case mmGB_TILE_MODE18:
> + case mmGB_TILE_MODE19:
> + case mmGB_TILE_MODE20:
> + case mmGB_TILE_MODE21:
> + case mmGB_TILE_MODE22:
> + case mmGB_TILE_MODE23:
> + case mmGB_TILE_MODE24:
> + case mmGB_TILE_MODE25:
> + case mmGB_TILE_MODE26:
> + case mmGB_TILE_MODE27:
> + case mmGB_TILE_MODE28:
> + case mmGB_TILE_MODE29:
> + case mmGB_TILE_MODE30:
> + case mmGB_TILE_MODE31:
> + idx = (reg_offset - mmGB_TILE_MODE0);
> + return adev->gfx.config.tile_mode_array[idx];
> + default:
> + return RREG32(reg_offset);
> }
> }
> +
> static int si_read_register(struct amdgpu_device *adev, u32 se_num,
> u32 sh_num, u32 reg_offset, u32 *value)
> {
> @@ -1138,8 +1147,12 @@ static int si_read_register(struct amdgpu_device *adev, u32 se_num,
> if (reg_offset != si_allowed_read_registers[i].reg_offset)
> continue;
>
> - *value = si_get_register_value(adev, indexed, se_num, sh_num,
> - reg_offset);
> + if (indexed)
> + return si_get_register_value_indexed(adev,
> + se_num, sh_num,
> + reg_offset, value);
> +
> + *value = si_get_register_value(adev, reg_offset);
> return 0;
> }
> return -EINVAL;
> --
> 2.28.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-drm-amdgpu-Fix-buffer-overflow-in-INFO-ioctl.patch
Type: text/x-patch
Size: 1344 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20200825/d482b555/attachment-0001.bin>
More information about the amd-gfx
mailing list