| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 29 Jul 2020 10:11:13 +0200
From: Christian König <ckoenig.leichtzumerken@...il.com>
To: Peilin Ye <yepeilin.cs@...il.com>,
Alex Deucher <alexander.deucher@....com>,
Christian König <christian.koenig@....com>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>
Cc: Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Felix Kuehling <Felix.Kuehling@....com>,
linux-kernel@...r.kernel.org, amd-gfx@...ts.freedesktop.org,
Nicholas Kazlauskas <nicholas.kazlauskas@....com>,
Marek Olšák <marek.olsak@....com>,
Hans de Goede <hdegoede@...hat.com>,
linux-kernel-mentees@...ts.linuxfoundation.org,
Trek <trek00@...ox.ru>, dri-devel@...ts.freedesktop.org,
Thomas Zimmermann <tzimmermann@...e.de>,
Evan Quan <evan.quan@....com>, Leo Liu <leo.liu@....com>,
Dan Carpenter <dan.carpenter@...cle.com>,
Xiaojie Yuan <xiaojie.yuan@....com>
Subject: Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent
kernel-infoleak in amdgpu_info_ioctl()
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@...r.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@...cle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@...il.com>
Reviewed-by: Christian König <christian.koenig@....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;
Powered by blists - more mailing lists