| 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
| ||
|
Message-ID: <YQKa76A6XuFqgM03@phenom.ffwll.local>
Date: Thu, 29 Jul 2021 14:11:27 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Kees Cook <keescook@...omium.org>, linux-kbuild@...r.kernel.org,
netdev@...r.kernel.org, linux-staging@...ts.linux.dev,
linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
linux-block@...r.kernel.org, clang-built-linux@...glegroups.com,
Keith Packard <keithpac@...zon.com>,
linux-hardening@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 25/64] drm/mga/mga_ioc32: Use struct_group() for memcpy()
region
On Wed, Jul 28, 2021 at 07:56:40AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jul 27, 2021 at 01:58:16PM -0700, Kees Cook wrote:
> > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > field bounds checking for memcpy(), memmove(), and memset(), avoid
> > intentionally writing across neighboring fields.
> >
> > Use struct_group() in struct drm32_mga_init around members chipset, sgram,
> > maccess, fb_cpp, front_offset, front_pitch, back_offset, back_pitch,
> > depth_cpp, depth_offset, depth_pitch, texture_offset, and texture_size,
> > so they can be referenced together. This will allow memcpy() and sizeof()
> > to more easily reason about sizes, improve readability, and avoid future
> > warnings about writing beyond the end of chipset.
> >
> > "pahole" shows no size nor member offset changes to struct drm32_mga_init.
> > "objdump -d" shows no meaningful object code changes (i.e. only source
> > line number induced differences and optimizations).
> >
> > Note that since this includes a UAPI header, struct_group() has been
> > explicitly redefined local to the header.
> >
> > Signed-off-by: Kees Cook <keescook@...omium.org>
> > ---
> > drivers/gpu/drm/mga/mga_ioc32.c | 30 ++++++++++++++------------
> > include/uapi/drm/mga_drm.h | 37 ++++++++++++++++++++++++---------
> > 2 files changed, 44 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mga/mga_ioc32.c b/drivers/gpu/drm/mga/mga_ioc32.c
> > index 4fd4de16cd32..fbd0329dbd4f 100644
> > --- a/drivers/gpu/drm/mga/mga_ioc32.c
> > +++ b/drivers/gpu/drm/mga/mga_ioc32.c
> > @@ -38,16 +38,21 @@
> > typedef struct drm32_mga_init {
> > int func;
> > u32 sarea_priv_offset;
> > - int chipset;
> > - int sgram;
> > - unsigned int maccess;
> > - unsigned int fb_cpp;
> > - unsigned int front_offset, front_pitch;
> > - unsigned int back_offset, back_pitch;
> > - unsigned int depth_cpp;
> > - unsigned int depth_offset, depth_pitch;
> > - unsigned int texture_offset[MGA_NR_TEX_HEAPS];
> > - unsigned int texture_size[MGA_NR_TEX_HEAPS];
> > + struct_group(always32bit,
> > + int chipset;
> > + int sgram;
> > + unsigned int maccess;
> > + unsigned int fb_cpp;
> > + unsigned int front_offset;
> > + unsigned int front_pitch;
> > + unsigned int back_offset;
> > + unsigned int back_pitch;
> > + unsigned int depth_cpp;
> > + unsigned int depth_offset;
> > + unsigned int depth_pitch;
> > + unsigned int texture_offset[MGA_NR_TEX_HEAPS];
> > + unsigned int texture_size[MGA_NR_TEX_HEAPS];
> > + );
> > u32 fb_offset;
> > u32 mmio_offset;
> > u32 status_offset;
> > @@ -67,9 +72,8 @@ static int compat_mga_init(struct file *file, unsigned int cmd,
> >
> > init.func = init32.func;
> > init.sarea_priv_offset = init32.sarea_priv_offset;
> > - memcpy(&init.chipset, &init32.chipset,
> > - offsetof(drm_mga_init_t, fb_offset) -
> > - offsetof(drm_mga_init_t, chipset));
> > + memcpy(&init.always32bit, &init32.always32bit,
> > + sizeof(init32.always32bit));
> > init.fb_offset = init32.fb_offset;
> > init.mmio_offset = init32.mmio_offset;
> > init.status_offset = init32.status_offset;
> > diff --git a/include/uapi/drm/mga_drm.h b/include/uapi/drm/mga_drm.h
> > index 8c4337548ab5..61612e5ecab2 100644
> > --- a/include/uapi/drm/mga_drm.h
> > +++ b/include/uapi/drm/mga_drm.h
> > @@ -265,6 +265,16 @@ typedef struct _drm_mga_sarea {
> > #define DRM_IOCTL_MGA_WAIT_FENCE DRM_IOWR(DRM_COMMAND_BASE + DRM_MGA_WAIT_FENCE, __u32)
> > #define DRM_IOCTL_MGA_DMA_BOOTSTRAP DRM_IOWR(DRM_COMMAND_BASE + DRM_MGA_DMA_BOOTSTRAP, drm_mga_dma_bootstrap_t)
> >
> > +#define __struct_group(name, fields) \
> > + union { \
> > + struct { \
> > + fields \
> > + }; \
> > + struct { \
> > + fields \
> > + } name; \
> > + }
> > +
> > typedef struct _drm_mga_warp_index {
> > int installed;
> > unsigned long phys_addr;
> > @@ -279,20 +289,25 @@ typedef struct drm_mga_init {
> >
> > unsigned long sarea_priv_offset;
> >
> > - int chipset;
> > - int sgram;
> > + __struct_group(always32bit,
> > + int chipset;
> > + int sgram;
> >
> > - unsigned int maccess;
> > + unsigned int maccess;
> >
> > - unsigned int fb_cpp;
> > - unsigned int front_offset, front_pitch;
> > - unsigned int back_offset, back_pitch;
> > + unsigned int fb_cpp;
> > + unsigned int front_offset;
> > + unsigned int front_pitch;
> > + unsigned int back_offset;
> > + unsigned int back_pitch;
> >
> > - unsigned int depth_cpp;
> > - unsigned int depth_offset, depth_pitch;
> > + unsigned int depth_cpp;
> > + unsigned int depth_offset;
> > + unsigned int depth_pitch;
> >
> > - unsigned int texture_offset[MGA_NR_TEX_HEAPS];
> > - unsigned int texture_size[MGA_NR_TEX_HEAPS];
> > + unsigned int texture_offset[MGA_NR_TEX_HEAPS];
> > + unsigned int texture_size[MGA_NR_TEX_HEAPS];
> > + );
> >
> > unsigned long fb_offset;
> > unsigned long mmio_offset;
> > @@ -302,6 +317,8 @@ typedef struct drm_mga_init {
> > unsigned long buffers_offset;
> > } drm_mga_init_t;
> >
> > +#undef __struct_group
> > +
>
> Why can you use __struct_group in this uapi header, but not the
> networking one?
If there's others, maybe we can stuff the uapi __struct_group into
linux/types.h where all the other __ uapi types hang out?
Anyway mga is very dead, I don't anyone cares.
Acked-by: Daniel Vetter <daniel.vetter@...ll.ch>
I'm assuming this goes in through a topic pull from you?
I'll leave the drm/amd one to figure out between you and Alex.
-Daniel
>
> thanks,
>
> greg k-h
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Powered by blists - more mailing lists