On Tue, Aug 25, 2020 at 7:21 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
From 2d6c1abaeab54561452b14c5d702f51235ae5a70 Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@xxxxxxx>
Date: Tue, 25 Aug 2020 11:43:45 -0400
Subject: [PATCH] drm/amdgpu: Fix buffer overflow in INFO ioctl
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.
Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 7ae8b0ee4610..9f80eaeaf0ae 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -669,8 +669,12 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
* in the bitfields */
if (se_num == AMDGPU_INFO_MMR_SE_INDEX_MASK)
se_num = 0xffffffff;
+ else if (se_num >= AMDGPU_GFX_MAX_SE)
+ return -EINVAL;
if (sh_num == AMDGPU_INFO_MMR_SH_INDEX_MASK)
sh_num = 0xffffffff;
+ else if (sh_num >= AMDGPU_GFX_MAX_SH_PER_SE)
+ return -EINVAL;
if (info->read_mmr_reg.count > 128)
return -EINVAL;
--
2.25.4
