[Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
Compiler leaves a 4-byte hole near the end of `dev_info`, causing amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which unfortunately does not initialize that 4-byte hole. Fix it by using memset() instead.
Cc: stable@vger.kernel.org Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Peilin Ye yepeilin.cs@gmail.com --- $ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o struct drm_amdgpu_info_device { __u32 device_id; /* 0 4 */ __u32 chip_rev; /* 4 4 */ __u32 external_rev; /* 8 4 */ __u32 pci_rev; /* 12 4 */ __u32 family; /* 16 4 */ __u32 num_shader_engines; /* 20 4 */ __u32 num_shader_arrays_per_engine; /* 24 4 */ __u32 gpu_counter_freq; /* 28 4 */ __u64 max_engine_clock; /* 32 8 */ __u64 max_memory_clock; /* 40 8 */ __u32 cu_active_number; /* 48 4 */ __u32 cu_ao_mask; /* 52 4 */ __u32 cu_bitmap[4][4]; /* 56 64 */ /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */ __u32 enabled_rb_pipes_mask; /* 120 4 */ __u32 num_rb_pipes; /* 124 4 */ /* --- cacheline 2 boundary (128 bytes) --- */ __u32 num_hw_gfx_contexts; /* 128 4 */ __u32 _pad; /* 132 4 */ __u64 ids_flags; /* 136 8 */ __u64 virtual_address_offset; /* 144 8 */ __u64 virtual_address_max; /* 152 8 */ __u32 virtual_address_alignment; /* 160 4 */ __u32 pte_fragment_size; /* 164 4 */ __u32 gart_page_size; /* 168 4 */ __u32 ce_ram_size; /* 172 4 */ __u32 vram_type; /* 176 4 */ __u32 vram_bit_width; /* 180 4 */ __u32 vce_harvest_config; /* 184 4 */ __u32 gc_double_offchip_lds_buf; /* 188 4 */ /* --- cacheline 3 boundary (192 bytes) --- */ __u64 prim_buf_gpu_addr; /* 192 8 */ __u64 pos_buf_gpu_addr; /* 200 8 */ __u64 cntl_sb_buf_gpu_addr; /* 208 8 */ __u64 param_buf_gpu_addr; /* 216 8 */ __u32 prim_buf_size; /* 224 4 */ __u32 pos_buf_size; /* 228 4 */ __u32 cntl_sb_buf_size; /* 232 4 */ __u32 param_buf_size; /* 236 4 */ __u32 wave_front_size; /* 240 4 */ __u32 num_shader_visible_vgprs; /* 244 4 */ __u32 num_cu_per_sh; /* 248 4 */ __u32 num_tcc_blocks; /* 252 4 */ /* --- cacheline 4 boundary (256 bytes) --- */ __u32 gs_vgt_table_depth; /* 256 4 */ __u32 gs_prim_buffer_depth; /* 260 4 */ __u32 max_gs_waves_per_vgt; /* 264 4 */ __u32 _pad1; /* 268 4 */ __u32 cu_ao_bitmap[4][4]; /* 272 64 */ /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */ __u64 high_va_offset; /* 336 8 */ __u64 high_va_max; /* 344 8 */ __u32 pa_sc_tile_steering_override; /* 352 4 */
/* XXX 4 bytes hole, try to pack */
__u64 tcc_disabled_mask; /* 360 8 */
/* size: 368, cachelines: 6, members: 49 */ /* sum members: 364, holes: 1, sum holes: 4 */ /* last cacheline: 48 bytes */ };
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index a8c47aecd342..0047da06041f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file return n ? -EFAULT : 0; } case AMDGPU_INFO_DEV_INFO: { - struct drm_amdgpu_info_device dev_info = {}; + struct drm_amdgpu_info_device dev_info; uint64_t vm_size;
+ memset(&dev_info, 0, sizeof(dev_info)); dev_info.device_id = dev->pdev->device; dev_info.chip_rev = adev->rev_id; dev_info.external_rev = adev->external_rev_id;
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which unfortunately does not initialize that 4-byte hole. Fix it by using memset() instead.
Cc: stable@vger.kernel.org Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Peilin Ye yepeilin.cs@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }" in the kernel is a really bad idea and should be avoided.
Thanks, Christian.
$ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o struct drm_amdgpu_info_device { __u32 device_id; /* 0 4 */ __u32 chip_rev; /* 4 4 */ __u32 external_rev; /* 8 4 */ __u32 pci_rev; /* 12 4 */ __u32 family; /* 16 4 */ __u32 num_shader_engines; /* 20 4 */ __u32 num_shader_arrays_per_engine; /* 24 4 */ __u32 gpu_counter_freq; /* 28 4 */ __u64 max_engine_clock; /* 32 8 */ __u64 max_memory_clock; /* 40 8 */ __u32 cu_active_number; /* 48 4 */ __u32 cu_ao_mask; /* 52 4 */ __u32 cu_bitmap[4][4]; /* 56 64 */ /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */ __u32 enabled_rb_pipes_mask; /* 120 4 */ __u32 num_rb_pipes; /* 124 4 */ /* --- cacheline 2 boundary (128 bytes) --- */ __u32 num_hw_gfx_contexts; /* 128 4 */ __u32 _pad; /* 132 4 */ __u64 ids_flags; /* 136 8 */ __u64 virtual_address_offset; /* 144 8 */ __u64 virtual_address_max; /* 152 8 */ __u32 virtual_address_alignment; /* 160 4 */ __u32 pte_fragment_size; /* 164 4 */ __u32 gart_page_size; /* 168 4 */ __u32 ce_ram_size; /* 172 4 */ __u32 vram_type; /* 176 4 */ __u32 vram_bit_width; /* 180 4 */ __u32 vce_harvest_config; /* 184 4 */ __u32 gc_double_offchip_lds_buf; /* 188 4 */ /* --- cacheline 3 boundary (192 bytes) --- */ __u64 prim_buf_gpu_addr; /* 192 8 */ __u64 pos_buf_gpu_addr; /* 200 8 */ __u64 cntl_sb_buf_gpu_addr; /* 208 8 */ __u64 param_buf_gpu_addr; /* 216 8 */ __u32 prim_buf_size; /* 224 4 */ __u32 pos_buf_size; /* 228 4 */ __u32 cntl_sb_buf_size; /* 232 4 */ __u32 param_buf_size; /* 236 4 */ __u32 wave_front_size; /* 240 4 */ __u32 num_shader_visible_vgprs; /* 244 4 */ __u32 num_cu_per_sh; /* 248 4 */ __u32 num_tcc_blocks; /* 252 4 */ /* --- cacheline 4 boundary (256 bytes) --- */ __u32 gs_vgt_table_depth; /* 256 4 */ __u32 gs_prim_buffer_depth; /* 260 4 */ __u32 max_gs_waves_per_vgt; /* 264 4 */ __u32 _pad1; /* 268 4 */ __u32 cu_ao_bitmap[4][4]; /* 272 64 */ /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */ __u64 high_va_offset; /* 336 8 */ __u64 high_va_max; /* 344 8 */ __u32 pa_sc_tile_steering_override; /* 352 4 */
/* XXX 4 bytes hole, try to pack */
__u64 tcc_disabled_mask; /* 360 8 */
/* size: 368, cachelines: 6, members: 49 */ /* sum members: 364, holes: 1, sum holes: 4 */ /* last cacheline: 48 bytes */ };
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index a8c47aecd342..0047da06041f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file return n ? -EFAULT : 0; } case AMDGPU_INFO_DEV_INFO: {
struct drm_amdgpu_info_device dev_info = {};
struct drm_amdgpu_info_device dev_info;uint64_t vm_size;
memset(&dev_info, 0, sizeof(dev_info));dev_info.device_id = dev->pdev->device; dev_info.chip_rev = adev->rev_id; dev_info.external_rev = adev->external_rev_id;
On Wed, Jul 29, 2020 at 10:11 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which unfortunately does not initialize that 4-byte hole. Fix it by using memset() instead.
Cc: stable@vger.kernel.org Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Peilin Ye yepeilin.cs@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }" in the kernel is a really bad idea and should be avoided.
I think the rule is also "don't create uapi structs with holes in them", but we've also fumbled that one quite a few times :-/ -Daniel
Thanks, Christian.
$ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o struct drm_amdgpu_info_device { __u32 device_id; /* 0 4 */ __u32 chip_rev; /* 4 4 */ __u32 external_rev; /* 8 4 */ __u32 pci_rev; /* 12 4 */ __u32 family; /* 16 4 */ __u32 num_shader_engines; /* 20 4 */ __u32 num_shader_arrays_per_engine; /* 24 4 */ __u32 gpu_counter_freq; /* 28 4 */ __u64 max_engine_clock; /* 32 8 */ __u64 max_memory_clock; /* 40 8 */ __u32 cu_active_number; /* 48 4 */ __u32 cu_ao_mask; /* 52 4 */ __u32 cu_bitmap[4][4]; /* 56 64 */ /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */ __u32 enabled_rb_pipes_mask; /* 120 4 */ __u32 num_rb_pipes; /* 124 4 */ /* --- cacheline 2 boundary (128 bytes) --- */ __u32 num_hw_gfx_contexts; /* 128 4 */ __u32 _pad; /* 132 4 */ __u64 ids_flags; /* 136 8 */ __u64 virtual_address_offset; /* 144 8 */ __u64 virtual_address_max; /* 152 8 */ __u32 virtual_address_alignment; /* 160 4 */ __u32 pte_fragment_size; /* 164 4 */ __u32 gart_page_size; /* 168 4 */ __u32 ce_ram_size; /* 172 4 */ __u32 vram_type; /* 176 4 */ __u32 vram_bit_width; /* 180 4 */ __u32 vce_harvest_config; /* 184 4 */ __u32 gc_double_offchip_lds_buf; /* 188 4 */ /* --- cacheline 3 boundary (192 bytes) --- */ __u64 prim_buf_gpu_addr; /* 192 8 */ __u64 pos_buf_gpu_addr; /* 200 8 */ __u64 cntl_sb_buf_gpu_addr; /* 208 8 */ __u64 param_buf_gpu_addr; /* 216 8 */ __u32 prim_buf_size; /* 224 4 */ __u32 pos_buf_size; /* 228 4 */ __u32 cntl_sb_buf_size; /* 232 4 */ __u32 param_buf_size; /* 236 4 */ __u32 wave_front_size; /* 240 4 */ __u32 num_shader_visible_vgprs; /* 244 4 */ __u32 num_cu_per_sh; /* 248 4 */ __u32 num_tcc_blocks; /* 252 4 */ /* --- cacheline 4 boundary (256 bytes) --- */ __u32 gs_vgt_table_depth; /* 256 4 */ __u32 gs_prim_buffer_depth; /* 260 4 */ __u32 max_gs_waves_per_vgt; /* 264 4 */ __u32 _pad1; /* 268 4 */ __u32 cu_ao_bitmap[4][4]; /* 272 64 */ /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */ __u64 high_va_offset; /* 336 8 */ __u64 high_va_max; /* 344 8 */ __u32 pa_sc_tile_steering_override; /* 352 4 */
/* XXX 4 bytes hole, try to pack */ __u64 tcc_disabled_mask; /* 360 8 */ /* size: 368, cachelines: 6, members: 49 */ /* sum members: 364, holes: 1, sum holes: 4 */ /* last cacheline: 48 bytes */};
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index a8c47aecd342..0047da06041f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file return n ? -EFAULT : 0; } case AMDGPU_INFO_DEV_INFO: {
struct drm_amdgpu_info_device dev_info = {};
struct drm_amdgpu_info_device dev_info; uint64_t vm_size;memset(&dev_info, 0, sizeof(dev_info)); dev_info.device_id = dev->pdev->device; dev_info.chip_rev = adev->rev_id; dev_info.external_rev = adev->external_rev_id;
On Wed, Jul 29, 2020 at 4:11 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which unfortunately does not initialize that 4-byte hole. Fix it by using memset() instead.
Cc: stable@vger.kernel.org Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Peilin Ye yepeilin.cs@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }" in the kernel is a really bad idea and should be avoided.
Moreover, it seems like different compilers seem to behave relatively differently with these and we often get reports of warnings with these on clang. When in doubt, memset.
Alex
Thanks, Christian.
$ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o struct drm_amdgpu_info_device { __u32 device_id; /* 0 4 */ __u32 chip_rev; /* 4 4 */ __u32 external_rev; /* 8 4 */ __u32 pci_rev; /* 12 4 */ __u32 family; /* 16 4 */ __u32 num_shader_engines; /* 20 4 */ __u32 num_shader_arrays_per_engine; /* 24 4 */ __u32 gpu_counter_freq; /* 28 4 */ __u64 max_engine_clock; /* 32 8 */ __u64 max_memory_clock; /* 40 8 */ __u32 cu_active_number; /* 48 4 */ __u32 cu_ao_mask; /* 52 4 */ __u32 cu_bitmap[4][4]; /* 56 64 */ /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */ __u32 enabled_rb_pipes_mask; /* 120 4 */ __u32 num_rb_pipes; /* 124 4 */ /* --- cacheline 2 boundary (128 bytes) --- */ __u32 num_hw_gfx_contexts; /* 128 4 */ __u32 _pad; /* 132 4 */ __u64 ids_flags; /* 136 8 */ __u64 virtual_address_offset; /* 144 8 */ __u64 virtual_address_max; /* 152 8 */ __u32 virtual_address_alignment; /* 160 4 */ __u32 pte_fragment_size; /* 164 4 */ __u32 gart_page_size; /* 168 4 */ __u32 ce_ram_size; /* 172 4 */ __u32 vram_type; /* 176 4 */ __u32 vram_bit_width; /* 180 4 */ __u32 vce_harvest_config; /* 184 4 */ __u32 gc_double_offchip_lds_buf; /* 188 4 */ /* --- cacheline 3 boundary (192 bytes) --- */ __u64 prim_buf_gpu_addr; /* 192 8 */ __u64 pos_buf_gpu_addr; /* 200 8 */ __u64 cntl_sb_buf_gpu_addr; /* 208 8 */ __u64 param_buf_gpu_addr; /* 216 8 */ __u32 prim_buf_size; /* 224 4 */ __u32 pos_buf_size; /* 228 4 */ __u32 cntl_sb_buf_size; /* 232 4 */ __u32 param_buf_size; /* 236 4 */ __u32 wave_front_size; /* 240 4 */ __u32 num_shader_visible_vgprs; /* 244 4 */ __u32 num_cu_per_sh; /* 248 4 */ __u32 num_tcc_blocks; /* 252 4 */ /* --- cacheline 4 boundary (256 bytes) --- */ __u32 gs_vgt_table_depth; /* 256 4 */ __u32 gs_prim_buffer_depth; /* 260 4 */ __u32 max_gs_waves_per_vgt; /* 264 4 */ __u32 _pad1; /* 268 4 */ __u32 cu_ao_bitmap[4][4]; /* 272 64 */ /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */ __u64 high_va_offset; /* 336 8 */ __u64 high_va_max; /* 344 8 */ __u32 pa_sc_tile_steering_override; /* 352 4 */
/* XXX 4 bytes hole, try to pack */ __u64 tcc_disabled_mask; /* 360 8 */ /* size: 368, cachelines: 6, members: 49 */ /* sum members: 364, holes: 1, sum holes: 4 */ /* last cacheline: 48 bytes */};
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index a8c47aecd342..0047da06041f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file return n ? -EFAULT : 0; } case AMDGPU_INFO_DEV_INFO: {
struct drm_amdgpu_info_device dev_info = {};
struct drm_amdgpu_info_device dev_info; uint64_t vm_size;memset(&dev_info, 0, sizeof(dev_info)); dev_info.device_id = dev->pdev->device; dev_info.chip_rev = adev->rev_id; dev_info.external_rev = adev->external_rev_id;
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 2020-07-29 9:49 a.m., Alex Deucher wrote:
On Wed, Jul 29, 2020 at 4:11 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which unfortunately does not initialize that 4-byte hole. Fix it by using memset() instead.
Cc: stable@vger.kernel.org Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Peilin Ye yepeilin.cs@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }" in the kernel is a really bad idea and should be avoided.
Moreover, it seems like different compilers seem to behave relatively differently with these and we often get reports of warnings with these on clang. When in doubt, memset.
There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/" drm*.c files,
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *(|NULL|0) *}" {} + | wc -l 374 $_
Out of which only 16 are of the non-ISO C variety, "= {}",
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *}" {} + | wc -l 16 $_
Perhaps the latter are the more pressing ones, since it is a C++ initializer and not a ISO C one.
Regards, Luben
Alex
Thanks, Christian.
$ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o struct drm_amdgpu_info_device { __u32 device_id; /* 0 4 */ __u32 chip_rev; /* 4 4 */ __u32 external_rev; /* 8 4 */ __u32 pci_rev; /* 12 4 */ __u32 family; /* 16 4 */ __u32 num_shader_engines; /* 20 4 */ __u32 num_shader_arrays_per_engine; /* 24 4 */ __u32 gpu_counter_freq; /* 28 4 */ __u64 max_engine_clock; /* 32 8 */ __u64 max_memory_clock; /* 40 8 */ __u32 cu_active_number; /* 48 4 */ __u32 cu_ao_mask; /* 52 4 */ __u32 cu_bitmap[4][4]; /* 56 64 */ /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */ __u32 enabled_rb_pipes_mask; /* 120 4 */ __u32 num_rb_pipes; /* 124 4 */ /* --- cacheline 2 boundary (128 bytes) --- */ __u32 num_hw_gfx_contexts; /* 128 4 */ __u32 _pad; /* 132 4 */ __u64 ids_flags; /* 136 8 */ __u64 virtual_address_offset; /* 144 8 */ __u64 virtual_address_max; /* 152 8 */ __u32 virtual_address_alignment; /* 160 4 */ __u32 pte_fragment_size; /* 164 4 */ __u32 gart_page_size; /* 168 4 */ __u32 ce_ram_size; /* 172 4 */ __u32 vram_type; /* 176 4 */ __u32 vram_bit_width; /* 180 4 */ __u32 vce_harvest_config; /* 184 4 */ __u32 gc_double_offchip_lds_buf; /* 188 4 */ /* --- cacheline 3 boundary (192 bytes) --- */ __u64 prim_buf_gpu_addr; /* 192 8 */ __u64 pos_buf_gpu_addr; /* 200 8 */ __u64 cntl_sb_buf_gpu_addr; /* 208 8 */ __u64 param_buf_gpu_addr; /* 216 8 */ __u32 prim_buf_size; /* 224 4 */ __u32 pos_buf_size; /* 228 4 */ __u32 cntl_sb_buf_size; /* 232 4 */ __u32 param_buf_size; /* 236 4 */ __u32 wave_front_size; /* 240 4 */ __u32 num_shader_visible_vgprs; /* 244 4 */ __u32 num_cu_per_sh; /* 248 4 */ __u32 num_tcc_blocks; /* 252 4 */ /* --- cacheline 4 boundary (256 bytes) --- */ __u32 gs_vgt_table_depth; /* 256 4 */ __u32 gs_prim_buffer_depth; /* 260 4 */ __u32 max_gs_waves_per_vgt; /* 264 4 */ __u32 _pad1; /* 268 4 */ __u32 cu_ao_bitmap[4][4]; /* 272 64 */ /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */ __u64 high_va_offset; /* 336 8 */ __u64 high_va_max; /* 344 8 */ __u32 pa_sc_tile_steering_override; /* 352 4 */
/* XXX 4 bytes hole, try to pack */ __u64 tcc_disabled_mask; /* 360 8 */ /* size: 368, cachelines: 6, members: 49 */ /* sum members: 364, holes: 1, sum holes: 4 */ /* last cacheline: 48 bytes */};
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index a8c47aecd342..0047da06041f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file return n ? -EFAULT : 0; } case AMDGPU_INFO_DEV_INFO: {
struct drm_amdgpu_info_device dev_info = {};
struct drm_amdgpu_info_device dev_info; uint64_t vm_size;memset(&dev_info, 0, sizeof(dev_info)); dev_info.device_id = dev->pdev->device; dev_info.chip_rev = adev->rev_id; dev_info.external_rev = adev->external_rev_id;
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...
On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
On 2020-07-29 9:49 a.m., Alex Deucher wrote:
On Wed, Jul 29, 2020 at 4:11 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which unfortunately does not initialize that 4-byte hole. Fix it by using memset() instead.
Cc: stable@vger.kernel.org Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Peilin Ye yepeilin.cs@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }" in the kernel is a really bad idea and should be avoided.
Moreover, it seems like different compilers seem to behave relatively differently with these and we often get reports of warnings with these on clang. When in doubt, memset.
There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/" drm*.c files,
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *(|NULL|0) *}" {} + | wc -l 374 $_
Out of which only 16 are of the non-ISO C variety, "= {}",
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *}" {} + | wc -l 16 $_
Perhaps the latter are the more pressing ones, since it is a C++ initializer and not a ISO C one.
It only matters when we care copying the data to userspace, if it all stays in the kernel, all is fine.
thanks,
greg k-h
Am 31.07.20 um 08:53 schrieb Greg Kroah-Hartman:
On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
On 2020-07-29 9:49 a.m., Alex Deucher wrote:
On Wed, Jul 29, 2020 at 4:11 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which unfortunately does not initialize that 4-byte hole. Fix it by using memset() instead.
Cc: stable@vger.kernel.org Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Peilin Ye yepeilin.cs@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }" in the kernel is a really bad idea and should be avoided.
Moreover, it seems like different compilers seem to behave relatively differently with these and we often get reports of warnings with these on clang. When in doubt, memset.
There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/" drm*.c files,
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *(|NULL|0) *}" {} + | wc -l 374 $_
Out of which only 16 are of the non-ISO C variety, "= {}",
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *}" {} + | wc -l 16 $_
Perhaps the latter are the more pressing ones, since it is a C++ initializer and not a ISO C one.
It only matters when we care copying the data to userspace, if it all stays in the kernel, all is fine.
Well only as long as you don't try to compute a CRC32, MD5 or any fingerprint for a hash from the bytes from the structure.
Then it fails horrible and you wonder why the code doesn't works as expected.
Regards, Christian.
thanks,
greg k-h
On Fri, Jul 31, 2020 at 08:57:53AM +0200, Christian König wrote:
Am 31.07.20 um 08:53 schrieb Greg Kroah-Hartman:
On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
On 2020-07-29 9:49 a.m., Alex Deucher wrote:
On Wed, Jul 29, 2020 at 4:11 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which unfortunately does not initialize that 4-byte hole. Fix it by using memset() instead.
Cc: stable@vger.kernel.org Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Peilin Ye yepeilin.cs@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }" in the kernel is a really bad idea and should be avoided.
Moreover, it seems like different compilers seem to behave relatively differently with these and we often get reports of warnings with these on clang. When in doubt, memset.
There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/" drm*.c files,
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *(|NULL|0) *}" {} + | wc -l 374 $_
Out of which only 16 are of the non-ISO C variety, "= {}",
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *}" {} + | wc -l 16 $_
Perhaps the latter are the more pressing ones, since it is a C++ initializer and not a ISO C one.
It only matters when we care copying the data to userspace, if it all stays in the kernel, all is fine.
Well only as long as you don't try to compute a CRC32, MD5 or any fingerprint for a hash from the bytes from the structure.
Then it fails horrible and you wonder why the code doesn't works as expected.
True, but the number of times I have ever needed to do that to a structure for a driver is, um, never...
If a structure ever needs to have that happen to it, I would sure hope the developer was aware of padding fields, otherwise, well, someone needs to take away their C language certification :)
thanks,
greg k-h
Am 31.07.20 um 09:10 schrieb Greg Kroah-Hartman:
On Fri, Jul 31, 2020 at 08:57:53AM +0200, Christian König wrote:
Am 31.07.20 um 08:53 schrieb Greg Kroah-Hartman:
On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
On 2020-07-29 9:49 a.m., Alex Deucher wrote:
On Wed, Jul 29, 2020 at 4:11 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye: > Compiler leaves a 4-byte hole near the end of `dev_info`, causing > amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace > when `size` is greater than 356. > > In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which > unfortunately does not initialize that 4-byte hole. Fix it by using > memset() instead. > > Cc: stable@vger.kernel.org > Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") > Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") > Suggested-by: Dan Carpenter dan.carpenter@oracle.com > Signed-off-by: Peilin Ye yepeilin.cs@gmail.com Reviewed-by: Christian König christian.koenig@amd.com
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }" in the kernel is a really bad idea and should be avoided.
Moreover, it seems like different compilers seem to behave relatively differently with these and we often get reports of warnings with these on clang. When in doubt, memset.
There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/" drm*.c files,
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *(|NULL|0) *}" {} + | wc -l 374 $_
Out of which only 16 are of the non-ISO C variety, "= {}",
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *}" {} + | wc -l 16 $_
Perhaps the latter are the more pressing ones, since it is a C++ initializer and not a ISO C one.
It only matters when we care copying the data to userspace, if it all stays in the kernel, all is fine.
Well only as long as you don't try to compute a CRC32, MD5 or any fingerprint for a hash from the bytes from the structure.
Then it fails horrible and you wonder why the code doesn't works as expected.
True, but the number of times I have ever needed to do that to a structure for a driver is, um, never...
If a structure ever needs to have that happen to it, I would sure hope the developer was aware of padding fields, otherwise, well, someone needs to take away their C language certification :)
Well it is very likely that stack allocated structures have the same values in the padding bytes most of the time. So the problem is very subtle and hard to detect.
We've seen enough problems with that over the last ~10 years that I'm clearly in favor of adding something to checkpatch.pl to spill out a warning if "= { }" is used for zero initialization.
Alternatively some of the people who know gcc/clang better than I do could come up with a warning that you shouldn't cast a structure with uninitialized padding to void* or u8*.
I mean KASAN is already doing a great job detecting that kind of stuff, but for this you still need to hit the offending code path.
Thanks, Christian.
thanks,
greg k-h
On Thu, Jul 30, 2020 at 11:09 PM Luben Tuikov luben.tuikov@amd.com wrote:
On 2020-07-29 9:49 a.m., Alex Deucher wrote:
On Wed, Jul 29, 2020 at 4:11 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which unfortunately does not initialize that 4-byte hole. Fix it by using memset() instead.
Cc: stable@vger.kernel.org Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Peilin Ye yepeilin.cs@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }" in the kernel is a really bad idea and should be avoided.
Moreover, it seems like different compilers seem to behave relatively differently with these and we often get reports of warnings with these on clang. When in doubt, memset.
There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/" drm*.c files,
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *(|NULL|0) *}" {} + | wc -l 374 $_
Out of which only 16 are of the non-ISO C variety, "= {}",
$find . ( -regex "./drm.*.c" -or -regex "./amd/.*.c" -or -regex "./scheduler/.*.c" ) -exec egrep -n -- " *= *{ *}" {} + | wc -l 16 $_
That is an unrelated issue, those were introduced to deal with older compilers that do not accept '{0}' as an initializer for an aggregate whose first member is another aggregate. Generally speaking, '= { }' is better to use in the kernel than '= { 0 }' because all supported compilers interpret that the same way for all structures.
Arnd
Applied. Thanks!
Alex
On Wed, Jul 29, 2020 at 4:11 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which unfortunately does not initialize that 4-byte hole. Fix it by using memset() instead.
Cc: stable@vger.kernel.org Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()") Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Suggested-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Peilin Ye yepeilin.cs@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }" in the kernel is a really bad idea and should be avoided.
Thanks, Christian.
$ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o struct drm_amdgpu_info_device { __u32 device_id; /* 0 4 */ __u32 chip_rev; /* 4 4 */ __u32 external_rev; /* 8 4 */ __u32 pci_rev; /* 12 4 */ __u32 family; /* 16 4 */ __u32 num_shader_engines; /* 20 4 */ __u32 num_shader_arrays_per_engine; /* 24 4 */ __u32 gpu_counter_freq; /* 28 4 */ __u64 max_engine_clock; /* 32 8 */ __u64 max_memory_clock; /* 40 8 */ __u32 cu_active_number; /* 48 4 */ __u32 cu_ao_mask; /* 52 4 */ __u32 cu_bitmap[4][4]; /* 56 64 */ /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */ __u32 enabled_rb_pipes_mask; /* 120 4 */ __u32 num_rb_pipes; /* 124 4 */ /* --- cacheline 2 boundary (128 bytes) --- */ __u32 num_hw_gfx_contexts; /* 128 4 */ __u32 _pad; /* 132 4 */ __u64 ids_flags; /* 136 8 */ __u64 virtual_address_offset; /* 144 8 */ __u64 virtual_address_max; /* 152 8 */ __u32 virtual_address_alignment; /* 160 4 */ __u32 pte_fragment_size; /* 164 4 */ __u32 gart_page_size; /* 168 4 */ __u32 ce_ram_size; /* 172 4 */ __u32 vram_type; /* 176 4 */ __u32 vram_bit_width; /* 180 4 */ __u32 vce_harvest_config; /* 184 4 */ __u32 gc_double_offchip_lds_buf; /* 188 4 */ /* --- cacheline 3 boundary (192 bytes) --- */ __u64 prim_buf_gpu_addr; /* 192 8 */ __u64 pos_buf_gpu_addr; /* 200 8 */ __u64 cntl_sb_buf_gpu_addr; /* 208 8 */ __u64 param_buf_gpu_addr; /* 216 8 */ __u32 prim_buf_size; /* 224 4 */ __u32 pos_buf_size; /* 228 4 */ __u32 cntl_sb_buf_size; /* 232 4 */ __u32 param_buf_size; /* 236 4 */ __u32 wave_front_size; /* 240 4 */ __u32 num_shader_visible_vgprs; /* 244 4 */ __u32 num_cu_per_sh; /* 248 4 */ __u32 num_tcc_blocks; /* 252 4 */ /* --- cacheline 4 boundary (256 bytes) --- */ __u32 gs_vgt_table_depth; /* 256 4 */ __u32 gs_prim_buffer_depth; /* 260 4 */ __u32 max_gs_waves_per_vgt; /* 264 4 */ __u32 _pad1; /* 268 4 */ __u32 cu_ao_bitmap[4][4]; /* 272 64 */ /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */ __u64 high_va_offset; /* 336 8 */ __u64 high_va_max; /* 344 8 */ __u32 pa_sc_tile_steering_override; /* 352 4 */
/* XXX 4 bytes hole, try to pack */ __u64 tcc_disabled_mask; /* 360 8 */ /* size: 368, cachelines: 6, members: 49 */ /* sum members: 364, holes: 1, sum holes: 4 */ /* last cacheline: 48 bytes */};
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index a8c47aecd342..0047da06041f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file return n ? -EFAULT : 0; } case AMDGPU_INFO_DEV_INFO: {
struct drm_amdgpu_info_device dev_info = {};
struct drm_amdgpu_info_device dev_info; uint64_t vm_size;memset(&dev_info, 0, sizeof(dev_info)); dev_info.device_id = dev->pdev->device; dev_info.chip_rev = adev->rev_id; dev_info.external_rev = adev->external_rev_id;
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Arnd Bergmann -
Christian König -
Christian König -
Daniel Vetter -
Greg Kroah-Hartman -
Luben Tuikov -
Peilin Ye