Re: [PATCH 25/64] drm/mga/mga_ioc32: Use struct_group() for memcpy() region

[Daniel Vetter]

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