* [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value()
@ 2020-08-25 11:18 Dan Carpenter
2020-08-25 11:19 ` [PATCH 2/3] drm/amdgpu/cik: fix buffer overflow in cik_get_register_value() Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Dan Carpenter @ 2020-08-25 11:18 UTC (permalink / raw)
To: Alex Deucher
Cc: Alex Jivin, Frederick Lawler, David Airlie, kernel-janitors,
amd-gfx, Sonny Jiang, Dan Carpenter, Daniel Vetter,
Bjorn Helgaas, Christian König, Monk Liu, Hawking Zhang
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@oracle.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] drm/amdgpu/cik: fix buffer overflow in cik_get_register_value()
2020-08-25 11:18 [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value() Dan Carpenter
@ 2020-08-25 11:19 ` Dan Carpenter
2020-08-25 11:19 ` [PATCH 3/3] drm/amdgpu/vi: fix buffer overflow in vi_get_register_value() Dan Carpenter
2020-08-25 15:53 ` [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value() Alex Deucher
2 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2020-08-25 11:19 UTC (permalink / raw)
To: Alex Deucher
Cc: Frederick Lawler, David Airlie, Marek Olšák,
kernel-janitors, Wenhui Sheng, amd-gfx, Daniel Vetter,
Bjorn Helgaas, Evan Quan, Christian König, Dan Carpenter
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: aca31681b1a5 ("drm/amdgpu: used cached gca values for cik_read_register")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/gpu/drm/amd/amdgpu/cik.c | 195 ++++++++++++++++---------------
1 file changed, 103 insertions(+), 92 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
index 7e71ffbca93d..91ebfa485333 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik.c
@@ -1042,100 +1042,108 @@ static const struct amdgpu_allowed_register_entry cik_allowed_read_registers[] {mmPA_SC_RASTER_CONFIG_1, true},
};
+static int cik_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;
-static uint32_t cik_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;
- case mmPA_SC_RASTER_CONFIG_1:
- return adev->gfx.config.rb_config[se_idx][sh_idx].raster_config_1;
- }
+ if (se_idx >= AMDGPU_GFX_MAX_SE ||
+ sh_idx >= AMDGPU_GFX_MAX_SH_PER_SE)
+ return -EINVAL;
+
+ 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;
+ case mmPA_SC_RASTER_CONFIG_1:
+ *value = adev->gfx.config.rb_config[se_idx][sh_idx].raster_config_1;
+ return 0;
+ }
- mutex_lock(&adev->grbm_idx_mutex);
- if (se_num != 0xffffffff || sh_num != 0xffffffff)
- amdgpu_gfx_select_se_sh(adev, se_num, sh_num, 0xffffffff);
+ mutex_lock(&adev->grbm_idx_mutex);
+ if (se_num != 0xffffffff || sh_num != 0xffffffff)
+ amdgpu_gfx_select_se_sh(adev, se_num, sh_num, 0xffffffff);
- val = RREG32(reg_offset);
+ *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 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];
- case mmGB_MACROTILE_MODE0:
- case mmGB_MACROTILE_MODE1:
- case mmGB_MACROTILE_MODE2:
- case mmGB_MACROTILE_MODE3:
- case mmGB_MACROTILE_MODE4:
- case mmGB_MACROTILE_MODE5:
- case mmGB_MACROTILE_MODE6:
- case mmGB_MACROTILE_MODE7:
- case mmGB_MACROTILE_MODE8:
- case mmGB_MACROTILE_MODE9:
- case mmGB_MACROTILE_MODE10:
- case mmGB_MACROTILE_MODE11:
- case mmGB_MACROTILE_MODE12:
- case mmGB_MACROTILE_MODE13:
- case mmGB_MACROTILE_MODE14:
- case mmGB_MACROTILE_MODE15:
- idx = (reg_offset - mmGB_MACROTILE_MODE0);
- return adev->gfx.config.macrotile_mode_array[idx];
- default:
- return 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 cik_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];
+ case mmGB_MACROTILE_MODE0:
+ case mmGB_MACROTILE_MODE1:
+ case mmGB_MACROTILE_MODE2:
+ case mmGB_MACROTILE_MODE3:
+ case mmGB_MACROTILE_MODE4:
+ case mmGB_MACROTILE_MODE5:
+ case mmGB_MACROTILE_MODE6:
+ case mmGB_MACROTILE_MODE7:
+ case mmGB_MACROTILE_MODE8:
+ case mmGB_MACROTILE_MODE9:
+ case mmGB_MACROTILE_MODE10:
+ case mmGB_MACROTILE_MODE11:
+ case mmGB_MACROTILE_MODE12:
+ case mmGB_MACROTILE_MODE13:
+ case mmGB_MACROTILE_MODE14:
+ case mmGB_MACROTILE_MODE15:
+ idx = (reg_offset - mmGB_MACROTILE_MODE0);
+ return adev->gfx.config.macrotile_mode_array[idx];
+ default:
+ return RREG32(reg_offset);
}
}
@@ -1151,8 +1159,11 @@ static int cik_read_register(struct amdgpu_device *adev, u32 se_num,
if (reg_offset != cik_allowed_read_registers[i].reg_offset)
continue;
- *value = cik_get_register_value(adev, indexed, se_num, sh_num,
- reg_offset);
+ if (indexed)
+ return cik_get_register_value_indexed(adev, se_num, sh_num,
+ reg_offset, value);
+
+ *value = cik_get_register_value(adev, reg_offset);
return 0;
}
return -EINVAL;
--
2.28.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] drm/amdgpu/vi: fix buffer overflow in vi_get_register_value()
2020-08-25 11:18 [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value() Dan Carpenter
2020-08-25 11:19 ` [PATCH 2/3] drm/amdgpu/cik: fix buffer overflow in cik_get_register_value() Dan Carpenter
@ 2020-08-25 11:19 ` Dan Carpenter
2020-08-25 15:53 ` [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value() Alex Deucher
2 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2020-08-25 11:19 UTC (permalink / raw)
To: Alex Deucher
Cc: David Airlie, dri-devel, kernel-janitors, Wenhui Sheng, amd-gfx,
Daniel Vetter, Evan Quan, Christian König, Monk Liu,
Hawking Zhang
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: db9635cc14f3 ("drm/amdgpu: used cached gca values for vi_read_register (v2)")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/gpu/drm/amd/amdgpu/vi.c | 197 +++++++++++++++++---------------
1 file changed, 105 insertions(+), 92 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
index f6f2ed0830b1..fd623ad9d51f 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.c
+++ b/drivers/gpu/drm/amd/amdgpu/vi.c
@@ -527,99 +527,108 @@ static const struct amdgpu_allowed_register_entry vi_allowed_read_registers[] {mmPA_SC_RASTER_CONFIG_1, true},
};
-static uint32_t vi_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;
- case mmPA_SC_RASTER_CONFIG_1:
- return adev->gfx.config.rb_config[se_idx][sh_idx].raster_config_1;
- }
+static int vi_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;
+ case mmPA_SC_RASTER_CONFIG_1:
+ *value = adev->gfx.config.rb_config[se_idx][sh_idx].raster_config_1;
+ 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];
- case mmGB_MACROTILE_MODE0:
- case mmGB_MACROTILE_MODE1:
- case mmGB_MACROTILE_MODE2:
- case mmGB_MACROTILE_MODE3:
- case mmGB_MACROTILE_MODE4:
- case mmGB_MACROTILE_MODE5:
- case mmGB_MACROTILE_MODE6:
- case mmGB_MACROTILE_MODE7:
- case mmGB_MACROTILE_MODE8:
- case mmGB_MACROTILE_MODE9:
- case mmGB_MACROTILE_MODE10:
- case mmGB_MACROTILE_MODE11:
- case mmGB_MACROTILE_MODE12:
- case mmGB_MACROTILE_MODE13:
- case mmGB_MACROTILE_MODE14:
- case mmGB_MACROTILE_MODE15:
- idx = (reg_offset - mmGB_MACROTILE_MODE0);
- return adev->gfx.config.macrotile_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 vi_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];
+ case mmGB_MACROTILE_MODE0:
+ case mmGB_MACROTILE_MODE1:
+ case mmGB_MACROTILE_MODE2:
+ case mmGB_MACROTILE_MODE3:
+ case mmGB_MACROTILE_MODE4:
+ case mmGB_MACROTILE_MODE5:
+ case mmGB_MACROTILE_MODE6:
+ case mmGB_MACROTILE_MODE7:
+ case mmGB_MACROTILE_MODE8:
+ case mmGB_MACROTILE_MODE9:
+ case mmGB_MACROTILE_MODE10:
+ case mmGB_MACROTILE_MODE11:
+ case mmGB_MACROTILE_MODE12:
+ case mmGB_MACROTILE_MODE13:
+ case mmGB_MACROTILE_MODE14:
+ case mmGB_MACROTILE_MODE15:
+ idx = (reg_offset - mmGB_MACROTILE_MODE0);
+ return adev->gfx.config.macrotile_mode_array[idx];
+ default:
+ return RREG32(reg_offset);
}
}
@@ -635,8 +644,12 @@ static int vi_read_register(struct amdgpu_device *adev, u32 se_num,
if (reg_offset != vi_allowed_read_registers[i].reg_offset)
continue;
- *value = vi_get_register_value(adev, indexed, se_num, sh_num,
- reg_offset);
+ if (indexed)
+ return vi_get_register_value_indexed(adev,
+ se_num, sh_num,
+ reg_offset, value);
+
+ *value = vi_get_register_value(adev, reg_offset);
return 0;
}
return -EINVAL;
--
2.28.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value()
2020-08-25 11:18 [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value() Dan Carpenter
2020-08-25 11:19 ` [PATCH 2/3] drm/amdgpu/cik: fix buffer overflow in cik_get_register_value() Dan Carpenter
2020-08-25 11:19 ` [PATCH 3/3] drm/amdgpu/vi: fix buffer overflow in vi_get_register_value() Dan Carpenter
@ 2020-08-25 15:53 ` Alex Deucher
2020-08-25 18:53 ` Dan Carpenter
2 siblings, 1 reply; 5+ messages in thread
From: Alex Deucher @ 2020-08-25 15:53 UTC (permalink / raw)
To: Dan Carpenter
Cc: Alex Jivin, Frederick Lawler, David Airlie, kernel-janitors,
amd-gfx list, Sonny Jiang, Bjorn Helgaas, Daniel Vetter,
Alex Deucher, Christian König, Monk Liu, Hawking Zhang
[-- Attachment #1: Type: text/plain, Size: 8528 bytes --]
On Tue, Aug 25, 2020 at 7:21 AM Dan Carpenter <dan.carpenter@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[-- Attachment #2: 0001-drm-amdgpu-Fix-buffer-overflow-in-INFO-ioctl.patch --]
[-- Type: text/x-patch, Size: 1344 bytes --]
From 2d6c1abaeab54561452b14c5d702f51235ae5a70 Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@amd.com>
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@oracle.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value()
2020-08-25 15:53 ` [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value() Alex Deucher
@ 2020-08-25 18:53 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2020-08-25 18:53 UTC (permalink / raw)
To: Alex Deucher
Cc: Alex Jivin, Frederick Lawler, David Airlie, kernel-janitors,
amd-gfx list, Sonny Jiang, Bjorn Helgaas, Daniel Vetter,
Alex Deucher, Christian König, Monk Liu, Hawking Zhang
On Tue, Aug 25, 2020 at 11:53:25AM -0400, Alex Deucher wrote:
> On Tue, Aug 25, 2020 at 7:21 AM Dan Carpenter <dan.carpenter@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@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.
>
That works too.
Acked-by: Dan Carpenter <dan.carpenter@oracle.com>
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-08-25 18:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-25 11:18 [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value() Dan Carpenter
2020-08-25 11:19 ` [PATCH 2/3] drm/amdgpu/cik: fix buffer overflow in cik_get_register_value() Dan Carpenter
2020-08-25 11:19 ` [PATCH 3/3] drm/amdgpu/vi: fix buffer overflow in vi_get_register_value() Dan Carpenter
2020-08-25 15:53 ` [PATCH 1/3] drm/amdgpu/si: Fix buffer overflow in si_get_register_value() Alex Deucher
2020-08-25 18:53 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).