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?
thanks,
greg k-h
