From: Luben Tuikov <luben.tuikov@amd.com>
To: Alex Deucher <alexdeucher@gmail.com>,
Christian Koenig <christian.koenig@amd.com>
Cc: "Arnd Bergmann" <arnd@arndb.de>,
"David Airlie" <airlied@linux.ie>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Felix Kuehling" <Felix.Kuehling@amd.com>,
LKML <linux-kernel@vger.kernel.org>,
"amd-gfx list" <amd-gfx@lists.freedesktop.org>,
"Peilin Ye" <yepeilin.cs@gmail.com>,
"Marek Olšák" <marek.olsak@amd.com>,
"Hans de Goede" <hdegoede@redhat.com>,
linux-kernel-mentees@lists.linuxfoundation.org,
Trek <trek00@inbox.ru>,
"Maling list - DRI developers" <dri-devel@lists.freedesktop.org>,
"Thomas Zimmermann" <tzimmermann@suse.de>,
"Alex Deucher" <alexander.deucher@amd.com>,
"Evan Quan" <evan.quan@amd.com>, "Leo Liu" <leo.liu@amd.com>,
"Nicholas Kazlauskas" <nicholas.kazlauskas@amd.com>,
"Dan Carpenter" <dan.carpenter@oracle.com>,
"Xiaojie Yuan" <xiaojie.yuan@amd.com>
Subject: Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
Date: Thu, 30 Jul 2020 17:09:07 -0400 [thread overview]
Message-ID: <8c5cf518-12d2-7495-7822-c7ebf8e61972@amd.com> (raw)
In-Reply-To: <CADnq5_NXOiAc7q5gQqF_wwtJD1o6nHjXM4O3gY6EwAQe9iOtXw@mail.gmail.com>
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.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7C801b15acd01d4ae785cb08d833c646d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637316274006319686&sdata=x3X0UlMW%2FnzTkmjHUTIyTEgQKC8m%2BrpqXBBMFLBhbuc%3D&reserved=0
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7C801b15acd01d4ae785cb08d833c646d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637316274006319686&sdata=x3X0UlMW%2FnzTkmjHUTIyTEgQKC8m%2BrpqXBBMFLBhbuc%3D&reserved=0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-07-30 21:09 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-28 19:29 [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl() Peilin Ye
2020-07-29 8:11 ` Christian König
2020-07-29 9:00 ` Daniel Vetter
2020-07-29 13:49 ` Alex Deucher
2020-07-30 21:09 ` Luben Tuikov [this message]
2020-07-31 6:53 ` Greg Kroah-Hartman
2020-07-31 6:57 ` Christian König
2020-07-31 7:10 ` Greg Kroah-Hartman
2020-07-31 7:57 ` Christian König
2020-07-31 7:34 ` Arnd Bergmann
2020-07-29 21:55 ` Alex Deucher
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8c5cf518-12d2-7495-7822-c7ebf8e61972@amd.com \
--to=luben.tuikov@amd.com \
--cc=Felix.Kuehling@amd.com \
--cc=airlied@linux.ie \
--cc=alexander.deucher@amd.com \
--cc=alexdeucher@gmail.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=arnd@arndb.de \
--cc=christian.koenig@amd.com \
--cc=dan.carpenter@oracle.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=evan.quan@amd.com \
--cc=gregkh@linuxfoundation.org \
--cc=hdegoede@redhat.com \
--cc=leo.liu@amd.com \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marek.olsak@amd.com \
--cc=nicholas.kazlauskas@amd.com \
--cc=trek00@inbox.ru \
--cc=tzimmermann@suse.de \
--cc=xiaojie.yuan@amd.com \
--cc=yepeilin.cs@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).