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.leichtzumer...@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: sta...@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.carpen...@oracle.com> > > > > > > Signed-off-by: Peilin Ye <yepeilin...@gmail.com> > > > > > Reviewed-by: Christian König <christian.koe...@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 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx