linux-hardening - Re: [PATCH 25/64] drm/mga/mga_ioc32: Use struct

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YQDxmEYfppJ4wAmD@kroah.com>
Date:   Wed, 28 Jul 2021 07:56:40 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-hardening@...r.kernel.org,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Keith Packard <keithpac@...zon.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-staging@...ts.linux.dev, linux-block@...r.kernel.org,
        linux-kbuild@...r.kernel.org, clang-built-linux@...glegroups.com
Subject: Re: [PATCH 25/64] drm/mga/mga_ioc32: Use struct_group() for memcpy()
 region

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?

thanks,

greg k-h