From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: "Christian König" <christian.koenig@amd.com>
Cc: "Arnd Bergmann" <arnd@arndb.de>,
"David Airlie" <airlied@linux.ie>,
linux-kernel-mentees@lists.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>,
"Luben Tuikov" <luben.tuikov@amd.com>, 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: Fri, 31 Jul 2020 09:10:52 +0200 [thread overview]
Message-ID: <20200731071052.GA1522097@kroah.com> (raw)
In-Reply-To: <690213fd-d3d2-2253-dcb2-367a65b34f38@amd.com>
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
_______________________________________________
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-31 7:11 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
2020-07-31 6:53 ` Greg Kroah-Hartman
2020-07-31 6:57 ` Christian König
2020-07-31 7:10 ` Greg Kroah-Hartman [this message]
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=20200731071052.GA1522097@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=Felix.Kuehling@amd.com \
--cc=airlied@linux.ie \
--cc=alexander.deucher@amd.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=hdegoede@redhat.com \
--cc=leo.liu@amd.com \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luben.tuikov@amd.com \
--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).