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@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxx>
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
