linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
@ 2020-07-28 19:29 Peilin Ye
  2020-07-29  8:11 ` Christian König
  0 siblings, 1 reply; 11+ messages in thread
From: Peilin Ye @ 2020-07-28 19:29 UTC (permalink / raw)
  To: Alex Deucher, Christian König, David Airlie, Daniel Vetter
  Cc: Arnd Bergmann, Felix Kuehling, linux-kernel, amd-gfx,
	Nicholas Kazlauskas, Marek Olšák, Hans de Goede,
	linux-kernel-mentees, Trek, dri-devel, Thomas Zimmermann,
	Evan Quan, Leo Liu, Peilin Ye, Dan Carpenter, Xiaojie Yuan

Compiler leaves a 4-byte hole near the end of `dev_info`, causing
amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
when `size` is greater than 356.

In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
unfortunately does not initialize that 4-byte hole. Fix it by using
memset() instead.

Cc: stable@vger.kernel.org
Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
$ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o
struct drm_amdgpu_info_device {
	__u32                      device_id;            /*     0     4 */
	__u32                      chip_rev;             /*     4     4 */
	__u32                      external_rev;         /*     8     4 */
	__u32                      pci_rev;              /*    12     4 */
	__u32                      family;               /*    16     4 */
	__u32                      num_shader_engines;   /*    20     4 */
	__u32                      num_shader_arrays_per_engine; /*    24     4 */
	__u32                      gpu_counter_freq;     /*    28     4 */
	__u64                      max_engine_clock;     /*    32     8 */
	__u64                      max_memory_clock;     /*    40     8 */
	__u32                      cu_active_number;     /*    48     4 */
	__u32                      cu_ao_mask;           /*    52     4 */
	__u32                      cu_bitmap[4][4];      /*    56    64 */
	/* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */
	__u32                      enabled_rb_pipes_mask; /*   120     4 */
	__u32                      num_rb_pipes;         /*   124     4 */
	/* --- cacheline 2 boundary (128 bytes) --- */
	__u32                      num_hw_gfx_contexts;  /*   128     4 */
	__u32                      _pad;                 /*   132     4 */
	__u64                      ids_flags;            /*   136     8 */
	__u64                      virtual_address_offset; /*   144     8 */
	__u64                      virtual_address_max;  /*   152     8 */
	__u32                      virtual_address_alignment; /*   160     4 */
	__u32                      pte_fragment_size;    /*   164     4 */
	__u32                      gart_page_size;       /*   168     4 */
	__u32                      ce_ram_size;          /*   172     4 */
	__u32                      vram_type;            /*   176     4 */
	__u32                      vram_bit_width;       /*   180     4 */
	__u32                      vce_harvest_config;   /*   184     4 */
	__u32                      gc_double_offchip_lds_buf; /*   188     4 */
	/* --- cacheline 3 boundary (192 bytes) --- */
	__u64                      prim_buf_gpu_addr;    /*   192     8 */
	__u64                      pos_buf_gpu_addr;     /*   200     8 */
	__u64                      cntl_sb_buf_gpu_addr; /*   208     8 */
	__u64                      param_buf_gpu_addr;   /*   216     8 */
	__u32                      prim_buf_size;        /*   224     4 */
	__u32                      pos_buf_size;         /*   228     4 */
	__u32                      cntl_sb_buf_size;     /*   232     4 */
	__u32                      param_buf_size;       /*   236     4 */
	__u32                      wave_front_size;      /*   240     4 */
	__u32                      num_shader_visible_vgprs; /*   244     4 */
	__u32                      num_cu_per_sh;        /*   248     4 */
	__u32                      num_tcc_blocks;       /*   252     4 */
	/* --- cacheline 4 boundary (256 bytes) --- */
	__u32                      gs_vgt_table_depth;   /*   256     4 */
	__u32                      gs_prim_buffer_depth; /*   260     4 */
	__u32                      max_gs_waves_per_vgt; /*   264     4 */
	__u32                      _pad1;                /*   268     4 */
	__u32                      cu_ao_bitmap[4][4];   /*   272    64 */
	/* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
	__u64                      high_va_offset;       /*   336     8 */
	__u64                      high_va_max;          /*   344     8 */
	__u32                      pa_sc_tile_steering_override; /*   352     4 */

	/* XXX 4 bytes hole, try to pack */

	__u64                      tcc_disabled_mask;    /*   360     8 */

	/* size: 368, cachelines: 6, members: 49 */
	/* sum members: 364, holes: 1, sum holes: 4 */
	/* last cacheline: 48 bytes */
};

 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index a8c47aecd342..0047da06041f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
 		return n ? -EFAULT : 0;
 	}
 	case AMDGPU_INFO_DEV_INFO: {
-		struct drm_amdgpu_info_device dev_info = {};
+		struct drm_amdgpu_info_device dev_info;
 		uint64_t vm_size;
 
+		memset(&dev_info, 0, sizeof(dev_info));
 		dev_info.device_id = dev->pdev->device;
 		dev_info.chip_rev = adev->rev_id;
 		dev_info.external_rev = adev->external_rev_id;
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
  2020-07-28 19:29 [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl() Peilin Ye
@ 2020-07-29  8:11 ` Christian König
  2020-07-29  9:00   ` Daniel Vetter
                     ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Christian König @ 2020-07-29  8:11 UTC (permalink / raw)
  To: Peilin Ye, Alex Deucher, Christian König, David Airlie,
	Daniel Vetter
  Cc: Leo Liu, Arnd Bergmann, Felix Kuehling, linux-kernel, amd-gfx,
	Marek Olšák, Hans de Goede, Trek, dri-devel,
	Thomas Zimmermann, Evan Quan, linux-kernel-mentees,
	Nicholas Kazlauskas, Dan Carpenter, Xiaojie Yuan

Am 28.07.20 um 21:29 schrieb Peilin Ye:
> Compiler leaves a 4-byte hole near the end of `dev_info`, causing
> amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
> when `size` is greater than 356.
>
> In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
> unfortunately does not initialize that 4-byte hole. Fix it by using
> memset() instead.
>
> Cc: stable@vger.kernel.org
> Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

I can't count how many of those we have fixed over the years.

At some point we should probably document that using "= {}" or "= { 0 }" 
in the kernel is a really bad idea and should be avoided.

Thanks,
Christian.

> ---
> $ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o
> struct drm_amdgpu_info_device {
> 	__u32                      device_id;            /*     0     4 */
> 	__u32                      chip_rev;             /*     4     4 */
> 	__u32                      external_rev;         /*     8     4 */
> 	__u32                      pci_rev;              /*    12     4 */
> 	__u32                      family;               /*    16     4 */
> 	__u32                      num_shader_engines;   /*    20     4 */
> 	__u32                      num_shader_arrays_per_engine; /*    24     4 */
> 	__u32                      gpu_counter_freq;     /*    28     4 */
> 	__u64                      max_engine_clock;     /*    32     8 */
> 	__u64                      max_memory_clock;     /*    40     8 */
> 	__u32                      cu_active_number;     /*    48     4 */
> 	__u32                      cu_ao_mask;           /*    52     4 */
> 	__u32                      cu_bitmap[4][4];      /*    56    64 */
> 	/* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */
> 	__u32                      enabled_rb_pipes_mask; /*   120     4 */
> 	__u32                      num_rb_pipes;         /*   124     4 */
> 	/* --- cacheline 2 boundary (128 bytes) --- */
> 	__u32                      num_hw_gfx_contexts;  /*   128     4 */
> 	__u32                      _pad;                 /*   132     4 */
> 	__u64                      ids_flags;            /*   136     8 */
> 	__u64                      virtual_address_offset; /*   144     8 */
> 	__u64                      virtual_address_max;  /*   152     8 */
> 	__u32                      virtual_address_alignment; /*   160     4 */
> 	__u32                      pte_fragment_size;    /*   164     4 */
> 	__u32                      gart_page_size;       /*   168     4 */
> 	__u32                      ce_ram_size;          /*   172     4 */
> 	__u32                      vram_type;            /*   176     4 */
> 	__u32                      vram_bit_width;       /*   180     4 */
> 	__u32                      vce_harvest_config;   /*   184     4 */
> 	__u32                      gc_double_offchip_lds_buf; /*   188     4 */
> 	/* --- cacheline 3 boundary (192 bytes) --- */
> 	__u64                      prim_buf_gpu_addr;    /*   192     8 */
> 	__u64                      pos_buf_gpu_addr;     /*   200     8 */
> 	__u64                      cntl_sb_buf_gpu_addr; /*   208     8 */
> 	__u64                      param_buf_gpu_addr;   /*   216     8 */
> 	__u32                      prim_buf_size;        /*   224     4 */
> 	__u32                      pos_buf_size;         /*   228     4 */
> 	__u32                      cntl_sb_buf_size;     /*   232     4 */
> 	__u32                      param_buf_size;       /*   236     4 */
> 	__u32                      wave_front_size;      /*   240     4 */
> 	__u32                      num_shader_visible_vgprs; /*   244     4 */
> 	__u32                      num_cu_per_sh;        /*   248     4 */
> 	__u32                      num_tcc_blocks;       /*   252     4 */
> 	/* --- cacheline 4 boundary (256 bytes) --- */
> 	__u32                      gs_vgt_table_depth;   /*   256     4 */
> 	__u32                      gs_prim_buffer_depth; /*   260     4 */
> 	__u32                      max_gs_waves_per_vgt; /*   264     4 */
> 	__u32                      _pad1;                /*   268     4 */
> 	__u32                      cu_ao_bitmap[4][4];   /*   272    64 */
> 	/* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
> 	__u64                      high_va_offset;       /*   336     8 */
> 	__u64                      high_va_max;          /*   344     8 */
> 	__u32                      pa_sc_tile_steering_override; /*   352     4 */
>
> 	/* XXX 4 bytes hole, try to pack */
>
> 	__u64                      tcc_disabled_mask;    /*   360     8 */
>
> 	/* size: 368, cachelines: 6, members: 49 */
> 	/* sum members: 364, holes: 1, sum holes: 4 */
> 	/* last cacheline: 48 bytes */
> };
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index a8c47aecd342..0047da06041f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>   		return n ? -EFAULT : 0;
>   	}
>   	case AMDGPU_INFO_DEV_INFO: {
> -		struct drm_amdgpu_info_device dev_info = {};
> +		struct drm_amdgpu_info_device dev_info;
>   		uint64_t vm_size;
>   
> +		memset(&dev_info, 0, sizeof(dev_info));
>   		dev_info.device_id = dev->pdev->device;
>   		dev_info.chip_rev = adev->rev_id;
>   		dev_info.external_rev = adev->external_rev_id;

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
  2020-07-29  8:11 ` Christian König
@ 2020-07-29  9:00   ` Daniel Vetter
  2020-07-29 13:49   ` Alex Deucher
  2020-07-29 21:55   ` Alex Deucher
  2 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2020-07-29  9:00 UTC (permalink / raw)
  To: Christian König
  Cc: Leo Liu, Arnd Bergmann, David Airlie, Felix Kuehling,
	Linux Kernel Mailing List, amd-gfx list, Nicholas Kazlauskas,
	Marek Olšák, Hans de Goede, Trek, dri-devel,
	Thomas Zimmermann, Alex Deucher, Evan Quan, linux-kernel-mentees,
	Peilin Ye, Dan Carpenter, Xiaojie Yuan

On Wed, Jul 29, 2020 at 10:11 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 28.07.20 um 21:29 schrieb Peilin Ye:
> > Compiler leaves a 4-byte hole near the end of `dev_info`, causing
> > amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
> > when `size` is greater than 356.
> >
> > In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
> > unfortunately does not initialize that 4-byte hole. Fix it by using
> > memset() instead.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
> > Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
> I can't count how many of those we have fixed over the years.
>
> At some point we should probably document that using "= {}" or "= { 0 }"
> in the kernel is a really bad idea and should be avoided.

I think the rule is also "don't create uapi structs with holes in
them", but we've also fumbled that one quite a few times :-/
-Daniel

>
> Thanks,
> Christian.
>
> > ---
> > $ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o
> > struct drm_amdgpu_info_device {
> >       __u32                      device_id;            /*     0     4 */
> >       __u32                      chip_rev;             /*     4     4 */
> >       __u32                      external_rev;         /*     8     4 */
> >       __u32                      pci_rev;              /*    12     4 */
> >       __u32                      family;               /*    16     4 */
> >       __u32                      num_shader_engines;   /*    20     4 */
> >       __u32                      num_shader_arrays_per_engine; /*    24     4 */
> >       __u32                      gpu_counter_freq;     /*    28     4 */
> >       __u64                      max_engine_clock;     /*    32     8 */
> >       __u64                      max_memory_clock;     /*    40     8 */
> >       __u32                      cu_active_number;     /*    48     4 */
> >       __u32                      cu_ao_mask;           /*    52     4 */
> >       __u32                      cu_bitmap[4][4];      /*    56    64 */
> >       /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */
> >       __u32                      enabled_rb_pipes_mask; /*   120     4 */
> >       __u32                      num_rb_pipes;         /*   124     4 */
> >       /* --- cacheline 2 boundary (128 bytes) --- */
> >       __u32                      num_hw_gfx_contexts;  /*   128     4 */
> >       __u32                      _pad;                 /*   132     4 */
> >       __u64                      ids_flags;            /*   136     8 */
> >       __u64                      virtual_address_offset; /*   144     8 */
> >       __u64                      virtual_address_max;  /*   152     8 */
> >       __u32                      virtual_address_alignment; /*   160     4 */
> >       __u32                      pte_fragment_size;    /*   164     4 */
> >       __u32                      gart_page_size;       /*   168     4 */
> >       __u32                      ce_ram_size;          /*   172     4 */
> >       __u32                      vram_type;            /*   176     4 */
> >       __u32                      vram_bit_width;       /*   180     4 */
> >       __u32                      vce_harvest_config;   /*   184     4 */
> >       __u32                      gc_double_offchip_lds_buf; /*   188     4 */
> >       /* --- cacheline 3 boundary (192 bytes) --- */
> >       __u64                      prim_buf_gpu_addr;    /*   192     8 */
> >       __u64                      pos_buf_gpu_addr;     /*   200     8 */
> >       __u64                      cntl_sb_buf_gpu_addr; /*   208     8 */
> >       __u64                      param_buf_gpu_addr;   /*   216     8 */
> >       __u32                      prim_buf_size;        /*   224     4 */
> >       __u32                      pos_buf_size;         /*   228     4 */
> >       __u32                      cntl_sb_buf_size;     /*   232     4 */
> >       __u32                      param_buf_size;       /*   236     4 */
> >       __u32                      wave_front_size;      /*   240     4 */
> >       __u32                      num_shader_visible_vgprs; /*   244     4 */
> >       __u32                      num_cu_per_sh;        /*   248     4 */
> >       __u32                      num_tcc_blocks;       /*   252     4 */
> >       /* --- cacheline 4 boundary (256 bytes) --- */
> >       __u32                      gs_vgt_table_depth;   /*   256     4 */
> >       __u32                      gs_prim_buffer_depth; /*   260     4 */
> >       __u32                      max_gs_waves_per_vgt; /*   264     4 */
> >       __u32                      _pad1;                /*   268     4 */
> >       __u32                      cu_ao_bitmap[4][4];   /*   272    64 */
> >       /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
> >       __u64                      high_va_offset;       /*   336     8 */
> >       __u64                      high_va_max;          /*   344     8 */
> >       __u32                      pa_sc_tile_steering_override; /*   352     4 */
> >
> >       /* XXX 4 bytes hole, try to pack */
> >
> >       __u64                      tcc_disabled_mask;    /*   360     8 */
> >
> >       /* size: 368, cachelines: 6, members: 49 */
> >       /* sum members: 364, holes: 1, sum holes: 4 */
> >       /* last cacheline: 48 bytes */
> > };
> >
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > index a8c47aecd342..0047da06041f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
> >               return n ? -EFAULT : 0;
> >       }
> >       case AMDGPU_INFO_DEV_INFO: {
> > -             struct drm_amdgpu_info_device dev_info = {};
> > +             struct drm_amdgpu_info_device dev_info;
> >               uint64_t vm_size;
> >
> > +             memset(&dev_info, 0, sizeof(dev_info));
> >               dev_info.device_id = dev->pdev->device;
> >               dev_info.chip_rev = adev->rev_id;
> >               dev_info.external_rev = adev->external_rev_id;
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
  2020-07-29  8:11 ` Christian König
  2020-07-29  9:00   ` Daniel Vetter
@ 2020-07-29 13:49   ` Alex Deucher
  2020-07-30 21:09     ` Luben Tuikov
  2020-07-29 21:55   ` Alex Deucher
  2 siblings, 1 reply; 11+ messages in thread
From: Alex Deucher @ 2020-07-29 13:49 UTC (permalink / raw)
  To: Christian Koenig
  Cc: Xiaojie Yuan, Thomas Zimmermann, Arnd Bergmann, David Airlie,
	Felix Kuehling, LKML, amd-gfx list, Nicholas Kazlauskas,
	Marek Olšák, Hans de Goede, Trek,
	Maling list - DRI developers, Daniel Vetter, Alex Deucher,
	Evan Quan, Leo Liu, Peilin Ye, Dan Carpenter,
	linux-kernel-mentees

On Wed, Jul 29, 2020 at 4:11 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 28.07.20 um 21:29 schrieb Peilin Ye:
> > Compiler leaves a 4-byte hole near the end of `dev_info`, causing
> > amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
> > when `size` is greater than 356.
> >
> > In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
> > unfortunately does not initialize that 4-byte hole. Fix it by using
> > memset() instead.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
> > Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
> I can't count how many of those we have fixed over the years.
>
> At some point we should probably document that using "= {}" or "= { 0 }"
> in the kernel is a really bad idea and should be avoided.

Moreover, it seems like different compilers seem to behave relatively
differently with these and we often get reports of warnings with these
on clang.  When in doubt, memset.

Alex

>
> Thanks,
> Christian.
>
> > ---
> > $ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o
> > struct drm_amdgpu_info_device {
> >       __u32                      device_id;            /*     0     4 */
> >       __u32                      chip_rev;             /*     4     4 */
> >       __u32                      external_rev;         /*     8     4 */
> >       __u32                      pci_rev;              /*    12     4 */
> >       __u32                      family;               /*    16     4 */
> >       __u32                      num_shader_engines;   /*    20     4 */
> >       __u32                      num_shader_arrays_per_engine; /*    24     4 */
> >       __u32                      gpu_counter_freq;     /*    28     4 */
> >       __u64                      max_engine_clock;     /*    32     8 */
> >       __u64                      max_memory_clock;     /*    40     8 */
> >       __u32                      cu_active_number;     /*    48     4 */
> >       __u32                      cu_ao_mask;           /*    52     4 */
> >       __u32                      cu_bitmap[4][4];      /*    56    64 */
> >       /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */
> >       __u32                      enabled_rb_pipes_mask; /*   120     4 */
> >       __u32                      num_rb_pipes;         /*   124     4 */
> >       /* --- cacheline 2 boundary (128 bytes) --- */
> >       __u32                      num_hw_gfx_contexts;  /*   128     4 */
> >       __u32                      _pad;                 /*   132     4 */
> >       __u64                      ids_flags;            /*   136     8 */
> >       __u64                      virtual_address_offset; /*   144     8 */
> >       __u64                      virtual_address_max;  /*   152     8 */
> >       __u32                      virtual_address_alignment; /*   160     4 */
> >       __u32                      pte_fragment_size;    /*   164     4 */
> >       __u32                      gart_page_size;       /*   168     4 */
> >       __u32                      ce_ram_size;          /*   172     4 */
> >       __u32                      vram_type;            /*   176     4 */
> >       __u32                      vram_bit_width;       /*   180     4 */
> >       __u32                      vce_harvest_config;   /*   184     4 */
> >       __u32                      gc_double_offchip_lds_buf; /*   188     4 */
> >       /* --- cacheline 3 boundary (192 bytes) --- */
> >       __u64                      prim_buf_gpu_addr;    /*   192     8 */
> >       __u64                      pos_buf_gpu_addr;     /*   200     8 */
> >       __u64                      cntl_sb_buf_gpu_addr; /*   208     8 */
> >       __u64                      param_buf_gpu_addr;   /*   216     8 */
> >       __u32                      prim_buf_size;        /*   224     4 */
> >       __u32                      pos_buf_size;         /*   228     4 */
> >       __u32                      cntl_sb_buf_size;     /*   232     4 */
> >       __u32                      param_buf_size;       /*   236     4 */
> >       __u32                      wave_front_size;      /*   240     4 */
> >       __u32                      num_shader_visible_vgprs; /*   244     4 */
> >       __u32                      num_cu_per_sh;        /*   248     4 */
> >       __u32                      num_tcc_blocks;       /*   252     4 */
> >       /* --- cacheline 4 boundary (256 bytes) --- */
> >       __u32                      gs_vgt_table_depth;   /*   256     4 */
> >       __u32                      gs_prim_buffer_depth; /*   260     4 */
> >       __u32                      max_gs_waves_per_vgt; /*   264     4 */
> >       __u32                      _pad1;                /*   268     4 */
> >       __u32                      cu_ao_bitmap[4][4];   /*   272    64 */
> >       /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
> >       __u64                      high_va_offset;       /*   336     8 */
> >       __u64                      high_va_max;          /*   344     8 */
> >       __u32                      pa_sc_tile_steering_override; /*   352     4 */
> >
> >       /* XXX 4 bytes hole, try to pack */
> >
> >       __u64                      tcc_disabled_mask;    /*   360     8 */
> >
> >       /* size: 368, cachelines: 6, members: 49 */
> >       /* sum members: 364, holes: 1, sum holes: 4 */
> >       /* last cacheline: 48 bytes */
> > };
> >
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > index a8c47aecd342..0047da06041f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
> >               return n ? -EFAULT : 0;
> >       }
> >       case AMDGPU_INFO_DEV_INFO: {
> > -             struct drm_amdgpu_info_device dev_info = {};
> > +             struct drm_amdgpu_info_device dev_info;
> >               uint64_t vm_size;
> >
> > +             memset(&dev_info, 0, sizeof(dev_info));
> >               dev_info.device_id = dev->pdev->device;
> >               dev_info.chip_rev = adev->rev_id;
> >               dev_info.external_rev = adev->external_rev_id;
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
  2020-07-29  8:11 ` Christian König
  2020-07-29  9:00   ` Daniel Vetter
  2020-07-29 13:49   ` Alex Deucher
@ 2020-07-29 21:55   ` Alex Deucher
  2 siblings, 0 replies; 11+ messages in thread
From: Alex Deucher @ 2020-07-29 21:55 UTC (permalink / raw)
  To: Christian Koenig
  Cc: Xiaojie Yuan, Thomas Zimmermann, Arnd Bergmann, David Airlie,
	Felix Kuehling, LKML, amd-gfx list, Nicholas Kazlauskas,
	Marek Olšák, Hans de Goede, Trek,
	Maling list - DRI developers, Daniel Vetter, Alex Deucher,
	Evan Quan, Leo Liu, Peilin Ye, Dan Carpenter,
	linux-kernel-mentees

Applied.  Thanks!

Alex

On Wed, Jul 29, 2020 at 4:11 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 28.07.20 um 21:29 schrieb Peilin Ye:
> > Compiler leaves a 4-byte hole near the end of `dev_info`, causing
> > amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
> > when `size` is greater than 356.
> >
> > In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
> > unfortunately does not initialize that 4-byte hole. Fix it by using
> > memset() instead.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
> > Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
> I can't count how many of those we have fixed over the years.
>
> At some point we should probably document that using "= {}" or "= { 0 }"
> in the kernel is a really bad idea and should be avoided.
>
> Thanks,
> Christian.
>
> > ---
> > $ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o
> > struct drm_amdgpu_info_device {
> >       __u32                      device_id;            /*     0     4 */
> >       __u32                      chip_rev;             /*     4     4 */
> >       __u32                      external_rev;         /*     8     4 */
> >       __u32                      pci_rev;              /*    12     4 */
> >       __u32                      family;               /*    16     4 */
> >       __u32                      num_shader_engines;   /*    20     4 */
> >       __u32                      num_shader_arrays_per_engine; /*    24     4 */
> >       __u32                      gpu_counter_freq;     /*    28     4 */
> >       __u64                      max_engine_clock;     /*    32     8 */
> >       __u64                      max_memory_clock;     /*    40     8 */
> >       __u32                      cu_active_number;     /*    48     4 */
> >       __u32                      cu_ao_mask;           /*    52     4 */
> >       __u32                      cu_bitmap[4][4];      /*    56    64 */
> >       /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */
> >       __u32                      enabled_rb_pipes_mask; /*   120     4 */
> >       __u32                      num_rb_pipes;         /*   124     4 */
> >       /* --- cacheline 2 boundary (128 bytes) --- */
> >       __u32                      num_hw_gfx_contexts;  /*   128     4 */
> >       __u32                      _pad;                 /*   132     4 */
> >       __u64                      ids_flags;            /*   136     8 */
> >       __u64                      virtual_address_offset; /*   144     8 */
> >       __u64                      virtual_address_max;  /*   152     8 */
> >       __u32                      virtual_address_alignment; /*   160     4 */
> >       __u32                      pte_fragment_size;    /*   164     4 */
> >       __u32                      gart_page_size;       /*   168     4 */
> >       __u32                      ce_ram_size;          /*   172     4 */
> >       __u32                      vram_type;            /*   176     4 */
> >       __u32                      vram_bit_width;       /*   180     4 */
> >       __u32                      vce_harvest_config;   /*   184     4 */
> >       __u32                      gc_double_offchip_lds_buf; /*   188     4 */
> >       /* --- cacheline 3 boundary (192 bytes) --- */
> >       __u64                      prim_buf_gpu_addr;    /*   192     8 */
> >       __u64                      pos_buf_gpu_addr;     /*   200     8 */
> >       __u64                      cntl_sb_buf_gpu_addr; /*   208     8 */
> >       __u64                      param_buf_gpu_addr;   /*   216     8 */
> >       __u32                      prim_buf_size;        /*   224     4 */
> >       __u32                      pos_buf_size;         /*   228     4 */
> >       __u32                      cntl_sb_buf_size;     /*   232     4 */
> >       __u32                      param_buf_size;       /*   236     4 */
> >       __u32                      wave_front_size;      /*   240     4 */
> >       __u32                      num_shader_visible_vgprs; /*   244     4 */
> >       __u32                      num_cu_per_sh;        /*   248     4 */
> >       __u32                      num_tcc_blocks;       /*   252     4 */
> >       /* --- cacheline 4 boundary (256 bytes) --- */
> >       __u32                      gs_vgt_table_depth;   /*   256     4 */
> >       __u32                      gs_prim_buffer_depth; /*   260     4 */
> >       __u32                      max_gs_waves_per_vgt; /*   264     4 */
> >       __u32                      _pad1;                /*   268     4 */
> >       __u32                      cu_ao_bitmap[4][4];   /*   272    64 */
> >       /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
> >       __u64                      high_va_offset;       /*   336     8 */
> >       __u64                      high_va_max;          /*   344     8 */
> >       __u32                      pa_sc_tile_steering_override; /*   352     4 */
> >
> >       /* XXX 4 bytes hole, try to pack */
> >
> >       __u64                      tcc_disabled_mask;    /*   360     8 */
> >
> >       /* size: 368, cachelines: 6, members: 49 */
> >       /* sum members: 364, holes: 1, sum holes: 4 */
> >       /* last cacheline: 48 bytes */
> > };
> >
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > index a8c47aecd342..0047da06041f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
> >               return n ? -EFAULT : 0;
> >       }
> >       case AMDGPU_INFO_DEV_INFO: {
> > -             struct drm_amdgpu_info_device dev_info = {};
> > +             struct drm_amdgpu_info_device dev_info;
> >               uint64_t vm_size;
> >
> > +             memset(&dev_info, 0, sizeof(dev_info));
> >               dev_info.device_id = dev->pdev->device;
> >               dev_info.chip_rev = adev->rev_id;
> >               dev_info.external_rev = adev->external_rev_id;
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
  2020-07-29 13:49   ` Alex Deucher
@ 2020-07-30 21:09     ` Luben Tuikov
  2020-07-31  6:53       ` Greg Kroah-Hartman
  2020-07-31  7:34       ` Arnd Bergmann
  0 siblings, 2 replies; 11+ messages in thread
From: Luben Tuikov @ 2020-07-30 21:09 UTC (permalink / raw)
  To: Alex Deucher, Christian Koenig
  Cc: Daniel Vetter, Arnd Bergmann, David Airlie, Felix Kuehling, LKML,
	amd-gfx list, Peilin Ye, Marek Olšák, Hans de Goede,
	linux-kernel-mentees, Trek, Maling list - DRI developers,
	Thomas Zimmermann, Alex Deucher, Evan Quan, Leo Liu,
	Nicholas Kazlauskas, Dan Carpenter, Xiaojie Yuan

On 2020-07-29 9:49 a.m., Alex Deucher wrote:
> On Wed, Jul 29, 2020 at 4:11 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>>
>> Am 28.07.20 um 21:29 schrieb Peilin Ye:
>>> Compiler leaves a 4-byte hole near the end of `dev_info`, causing
>>> amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
>>> when `size` is greater than 356.
>>>
>>> In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
>>> unfortunately does not initialize that 4-byte hole. Fix it by using
>>> memset() instead.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>>> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
>>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>
>> I can't count how many of those we have fixed over the years.
>>
>> At some point we should probably document that using "= {}" or "= { 0 }"
>> in the kernel is a really bad idea and should be avoided.
> 
> Moreover, it seems like different compilers seem to behave relatively
> differently with these and we often get reports of warnings with these
> on clang.  When in doubt, memset.

There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/"
drm*.c files,

$find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *(|NULL|0) *}" \{\} \+ | wc -l
374
$_

Out of which only 16 are of the non-ISO C variety, "= {}",

$find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *}" \{\} \+ | wc -l
16
$_

Perhaps the latter are the more pressing ones, since it is a C++ initializer and not a ISO C one.

Regards,
Luben



> 
> Alex
> 
>>
>> Thanks,
>> Christian.
>>
>>> ---
>>> $ pahole -C "drm_amdgpu_info_device" drivers/gpu/drm/amd/amdgpu/amdgpu_kms.o
>>> struct drm_amdgpu_info_device {
>>>       __u32                      device_id;            /*     0     4 */
>>>       __u32                      chip_rev;             /*     4     4 */
>>>       __u32                      external_rev;         /*     8     4 */
>>>       __u32                      pci_rev;              /*    12     4 */
>>>       __u32                      family;               /*    16     4 */
>>>       __u32                      num_shader_engines;   /*    20     4 */
>>>       __u32                      num_shader_arrays_per_engine; /*    24     4 */
>>>       __u32                      gpu_counter_freq;     /*    28     4 */
>>>       __u64                      max_engine_clock;     /*    32     8 */
>>>       __u64                      max_memory_clock;     /*    40     8 */
>>>       __u32                      cu_active_number;     /*    48     4 */
>>>       __u32                      cu_ao_mask;           /*    52     4 */
>>>       __u32                      cu_bitmap[4][4];      /*    56    64 */
>>>       /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */
>>>       __u32                      enabled_rb_pipes_mask; /*   120     4 */
>>>       __u32                      num_rb_pipes;         /*   124     4 */
>>>       /* --- cacheline 2 boundary (128 bytes) --- */
>>>       __u32                      num_hw_gfx_contexts;  /*   128     4 */
>>>       __u32                      _pad;                 /*   132     4 */
>>>       __u64                      ids_flags;            /*   136     8 */
>>>       __u64                      virtual_address_offset; /*   144     8 */
>>>       __u64                      virtual_address_max;  /*   152     8 */
>>>       __u32                      virtual_address_alignment; /*   160     4 */
>>>       __u32                      pte_fragment_size;    /*   164     4 */
>>>       __u32                      gart_page_size;       /*   168     4 */
>>>       __u32                      ce_ram_size;          /*   172     4 */
>>>       __u32                      vram_type;            /*   176     4 */
>>>       __u32                      vram_bit_width;       /*   180     4 */
>>>       __u32                      vce_harvest_config;   /*   184     4 */
>>>       __u32                      gc_double_offchip_lds_buf; /*   188     4 */
>>>       /* --- cacheline 3 boundary (192 bytes) --- */
>>>       __u64                      prim_buf_gpu_addr;    /*   192     8 */
>>>       __u64                      pos_buf_gpu_addr;     /*   200     8 */
>>>       __u64                      cntl_sb_buf_gpu_addr; /*   208     8 */
>>>       __u64                      param_buf_gpu_addr;   /*   216     8 */
>>>       __u32                      prim_buf_size;        /*   224     4 */
>>>       __u32                      pos_buf_size;         /*   228     4 */
>>>       __u32                      cntl_sb_buf_size;     /*   232     4 */
>>>       __u32                      param_buf_size;       /*   236     4 */
>>>       __u32                      wave_front_size;      /*   240     4 */
>>>       __u32                      num_shader_visible_vgprs; /*   244     4 */
>>>       __u32                      num_cu_per_sh;        /*   248     4 */
>>>       __u32                      num_tcc_blocks;       /*   252     4 */
>>>       /* --- cacheline 4 boundary (256 bytes) --- */
>>>       __u32                      gs_vgt_table_depth;   /*   256     4 */
>>>       __u32                      gs_prim_buffer_depth; /*   260     4 */
>>>       __u32                      max_gs_waves_per_vgt; /*   264     4 */
>>>       __u32                      _pad1;                /*   268     4 */
>>>       __u32                      cu_ao_bitmap[4][4];   /*   272    64 */
>>>       /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
>>>       __u64                      high_va_offset;       /*   336     8 */
>>>       __u64                      high_va_max;          /*   344     8 */
>>>       __u32                      pa_sc_tile_steering_override; /*   352     4 */
>>>
>>>       /* XXX 4 bytes hole, try to pack */
>>>
>>>       __u64                      tcc_disabled_mask;    /*   360     8 */
>>>
>>>       /* size: 368, cachelines: 6, members: 49 */
>>>       /* sum members: 364, holes: 1, sum holes: 4 */
>>>       /* last cacheline: 48 bytes */
>>> };
>>>
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index a8c47aecd342..0047da06041f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -707,9 +707,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>>>               return n ? -EFAULT : 0;
>>>       }
>>>       case AMDGPU_INFO_DEV_INFO: {
>>> -             struct drm_amdgpu_info_device dev_info = {};
>>> +             struct drm_amdgpu_info_device dev_info;
>>>               uint64_t vm_size;
>>>
>>> +             memset(&dev_info, 0, sizeof(dev_info));
>>>               dev_info.device_id = dev->pdev->device;
>>>               dev_info.chip_rev = adev->rev_id;
>>>               dev_info.external_rev = adev->external_rev_id;
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cluben.tuikov%40amd.com%7C801b15acd01d4ae785cb08d833c646d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637316274006319686&amp;sdata=x3X0UlMW%2FnzTkmjHUTIyTEgQKC8m%2BrpqXBBMFLBhbuc%3D&amp;reserved=0
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cluben.tuikov%40amd.com%7C801b15acd01d4ae785cb08d833c646d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637316274006319686&amp;sdata=x3X0UlMW%2FnzTkmjHUTIyTEgQKC8m%2BrpqXBBMFLBhbuc%3D&amp;reserved=0
> 

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
  2020-07-30 21:09     ` Luben Tuikov
@ 2020-07-31  6:53       ` Greg Kroah-Hartman
  2020-07-31  6:57         ` Christian König
  2020-07-31  7:34       ` Arnd Bergmann
  1 sibling, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-31  6:53 UTC (permalink / raw)
  To: Luben Tuikov
  Cc: Daniel Vetter, Arnd Bergmann, David Airlie, linux-kernel-mentees,
	Felix Kuehling, LKML, amd-gfx list, Nicholas Kazlauskas,
	Marek Olšák, Hans de Goede, Trek,
	Maling list - DRI developers, Thomas Zimmermann, Peilin Ye,
	Alex Deucher, Alex Deucher, Evan Quan, Leo Liu, Christian Koenig,
	Dan Carpenter, Xiaojie Yuan

On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
> On 2020-07-29 9:49 a.m., Alex Deucher wrote:
> > On Wed, Jul 29, 2020 at 4:11 AM Christian König
> > <ckoenig.leichtzumerken@gmail.com> wrote:
> >>
> >> Am 28.07.20 um 21:29 schrieb Peilin Ye:
> >>> Compiler leaves a 4-byte hole near the end of `dev_info`, causing
> >>> amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
> >>> when `size` is greater than 356.
> >>>
> >>> In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
> >>> unfortunately does not initialize that 4-byte hole. Fix it by using
> >>> memset() instead.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
> >>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> >>> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> >>
> >> Reviewed-by: Christian König <christian.koenig@amd.com>
> >>
> >> I can't count how many of those we have fixed over the years.
> >>
> >> At some point we should probably document that using "= {}" or "= { 0 }"
> >> in the kernel is a really bad idea and should be avoided.
> > 
> > Moreover, it seems like different compilers seem to behave relatively
> > differently with these and we often get reports of warnings with these
> > on clang.  When in doubt, memset.
> 
> There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/"
> drm*.c files,
> 
> $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *(|NULL|0) *}" \{\} \+ | wc -l
> 374
> $_
> 
> Out of which only 16 are of the non-ISO C variety, "= {}",
> 
> $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *}" \{\} \+ | wc -l
> 16
> $_
> 
> Perhaps the latter are the more pressing ones, since it is a C++ initializer and not a ISO C one.

It only matters when we care copying the data to userspace, if it all
stays in the kernel, all is fine.

thanks,

greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
  2020-07-31  6:53       ` Greg Kroah-Hartman
@ 2020-07-31  6:57         ` Christian König
  2020-07-31  7:10           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2020-07-31  6:57 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Luben Tuikov
  Cc: Daniel Vetter, Arnd Bergmann, David Airlie, linux-kernel-mentees,
	Felix Kuehling, LKML, amd-gfx list, Peilin Ye,
	Marek Olšák, Hans de Goede, Trek,
	Maling list - DRI developers, Thomas Zimmermann, Alex Deucher,
	Alex Deucher, Evan Quan, Leo Liu, Nicholas Kazlauskas,
	Dan Carpenter, Xiaojie Yuan

Am 31.07.20 um 08:53 schrieb Greg Kroah-Hartman:
> On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
>> On 2020-07-29 9:49 a.m., Alex Deucher wrote:
>>> On Wed, Jul 29, 2020 at 4:11 AM Christian König
>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>> Am 28.07.20 um 21:29 schrieb Peilin Ye:
>>>>> Compiler leaves a 4-byte hole near the end of `dev_info`, causing
>>>>> amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
>>>>> when `size` is greater than 356.
>>>>>
>>>>> In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
>>>>> unfortunately does not initialize that 4-byte hole. Fix it by using
>>>>> memset() instead.
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
>>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>>>>> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>
>>>> I can't count how many of those we have fixed over the years.
>>>>
>>>> At some point we should probably document that using "= {}" or "= { 0 }"
>>>> in the kernel is a really bad idea and should be avoided.
>>> Moreover, it seems like different compilers seem to behave relatively
>>> differently with these and we often get reports of warnings with these
>>> on clang.  When in doubt, memset.
>> There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/"
>> drm*.c files,
>>
>> $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *(|NULL|0) *}" \{\} \+ | wc -l
>> 374
>> $_
>>
>> Out of which only 16 are of the non-ISO C variety, "= {}",
>>
>> $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *}" \{\} \+ | wc -l
>> 16
>> $_
>>
>> Perhaps the latter are the more pressing ones, since it is a C++ initializer and not a ISO C one.
> It only matters when we care copying the data to userspace, if it all
> stays in the kernel, all is fine.

Well only as long as you don't try to compute a CRC32, MD5 or any 
fingerprint for a hash from the bytes from the structure.

Then it fails horrible and you wonder why the code doesn't works as 
expected.

Regards,
Christian.

>
> thanks,
>
> greg k-h

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
  2020-07-31  6:57         ` Christian König
@ 2020-07-31  7:10           ` Greg Kroah-Hartman
  2020-07-31  7:57             ` Christian König
  0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-31  7:10 UTC (permalink / raw)
  To: Christian König
  Cc: Daniel Vetter, Arnd Bergmann, David Airlie, linux-kernel-mentees,
	Felix Kuehling, LKML, amd-gfx list, Peilin Ye,
	Marek Olšák, Hans de Goede, Luben Tuikov, Trek,
	Maling list - DRI developers, Thomas Zimmermann, Alex Deucher,
	Alex Deucher, Evan Quan, Leo Liu, Nicholas Kazlauskas,
	Dan Carpenter, Xiaojie Yuan

On Fri, Jul 31, 2020 at 08:57:53AM +0200, Christian König wrote:
> Am 31.07.20 um 08:53 schrieb Greg Kroah-Hartman:
> > On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
> > > On 2020-07-29 9:49 a.m., Alex Deucher wrote:
> > > > On Wed, Jul 29, 2020 at 4:11 AM Christian König
> > > > <ckoenig.leichtzumerken@gmail.com> wrote:
> > > > > Am 28.07.20 um 21:29 schrieb Peilin Ye:
> > > > > > Compiler leaves a 4-byte hole near the end of `dev_info`, causing
> > > > > > amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
> > > > > > when `size` is greater than 356.
> > > > > > 
> > > > > > In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
> > > > > > unfortunately does not initialize that 4-byte hole. Fix it by using
> > > > > > memset() instead.
> > > > > > 
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
> > > > > > Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> > > > > > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> > > > > Reviewed-by: Christian König <christian.koenig@amd.com>
> > > > > 
> > > > > I can't count how many of those we have fixed over the years.
> > > > > 
> > > > > At some point we should probably document that using "= {}" or "= { 0 }"
> > > > > in the kernel is a really bad idea and should be avoided.
> > > > Moreover, it seems like different compilers seem to behave relatively
> > > > differently with these and we often get reports of warnings with these
> > > > on clang.  When in doubt, memset.
> > > There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/"
> > > drm*.c files,
> > > 
> > > $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *(|NULL|0) *}" \{\} \+ | wc -l
> > > 374
> > > $_
> > > 
> > > Out of which only 16 are of the non-ISO C variety, "= {}",
> > > 
> > > $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *}" \{\} \+ | wc -l
> > > 16
> > > $_
> > > 
> > > Perhaps the latter are the more pressing ones, since it is a C++ initializer and not a ISO C one.
> > It only matters when we care copying the data to userspace, if it all
> > stays in the kernel, all is fine.
> 
> Well only as long as you don't try to compute a CRC32, MD5 or any
> fingerprint for a hash from the bytes from the structure.
> 
> Then it fails horrible and you wonder why the code doesn't works as
> expected.

True, but the number of times I have ever needed to do that to a
structure for a driver is, um, never...

If a structure ever needs to have that happen to it, I would sure hope
the developer was aware of padding fields, otherwise, well, someone
needs to take away their C language certification :)

thanks,

greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
  2020-07-30 21:09     ` Luben Tuikov
  2020-07-31  6:53       ` Greg Kroah-Hartman
@ 2020-07-31  7:34       ` Arnd Bergmann
  1 sibling, 0 replies; 11+ messages in thread
From: Arnd Bergmann @ 2020-07-31  7:34 UTC (permalink / raw)
  To: Luben Tuikov
  Cc: Daniel Vetter, David Airlie, Felix Kuehling, LKML, amd-gfx list,
	Nicholas Kazlauskas, Marek Olšák, Hans de Goede,
	linux-kernel-mentees, Trek, Maling list - DRI developers,
	Thomas Zimmermann, Peilin Ye, Alex Deucher, Alex Deucher,
	Evan Quan, Leo Liu, Christian Koenig, Dan Carpenter,
	Xiaojie Yuan

On Thu, Jul 30, 2020 at 11:09 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
> On 2020-07-29 9:49 a.m., Alex Deucher wrote:
> > On Wed, Jul 29, 2020 at 4:11 AM Christian König
> > <ckoenig.leichtzumerken@gmail.com> wrote:
> >>
> >> Am 28.07.20 um 21:29 schrieb Peilin Ye:
> >>> Compiler leaves a 4-byte hole near the end of `dev_info`, causing
> >>> amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
> >>> when `size` is greater than 356.
> >>>
> >>> In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
> >>> unfortunately does not initialize that 4-byte hole. Fix it by using
> >>> memset() instead.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
> >>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> >>> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> >>
> >> Reviewed-by: Christian König <christian.koenig@amd.com>
> >>
> >> I can't count how many of those we have fixed over the years.
> >>
> >> At some point we should probably document that using "= {}" or "= { 0 }"
> >> in the kernel is a really bad idea and should be avoided.
> >
> > Moreover, it seems like different compilers seem to behave relatively
> > differently with these and we often get reports of warnings with these
> > on clang.  When in doubt, memset.
>
> There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/"
> drm*.c files,
>
> $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *(|NULL|0) *}" \{\} \+ | wc -l
> 374
> $_
>
> Out of which only 16 are of the non-ISO C variety, "= {}",
>
> $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *}" \{\} \+ | wc -l
> 16
> $_

That is an unrelated issue, those were introduced to deal with older compilers
that do not accept '{0}' as an initializer for an aggregate whose
first member is
another aggregate. Generally speaking, '= { }' is better to use in the
kernel than
'= { 0 }' because all supported compilers interpret that the same way for all
structures.

     Arnd
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
  2020-07-31  7:10           ` Greg Kroah-Hartman
@ 2020-07-31  7:57             ` Christian König
  0 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2020-07-31  7:57 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Daniel Vetter, Arnd Bergmann, David Airlie, linux-kernel-mentees,
	Felix Kuehling, LKML, amd-gfx list, Peilin Ye,
	Marek Olšák, Hans de Goede, Luben Tuikov, Trek,
	Maling list - DRI developers, Thomas Zimmermann, Alex Deucher,
	Alex Deucher, Evan Quan, Leo Liu, Nicholas Kazlauskas,
	Dan Carpenter, Xiaojie Yuan

Am 31.07.20 um 09:10 schrieb Greg Kroah-Hartman:
> On Fri, Jul 31, 2020 at 08:57:53AM +0200, Christian König wrote:
>> Am 31.07.20 um 08:53 schrieb Greg Kroah-Hartman:
>>> On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
>>>> On 2020-07-29 9:49 a.m., Alex Deucher wrote:
>>>>> On Wed, Jul 29, 2020 at 4:11 AM Christian König
>>>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>>>> Am 28.07.20 um 21:29 schrieb Peilin Ye:
>>>>>>> Compiler leaves a 4-byte hole near the end of `dev_info`, causing
>>>>>>> amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
>>>>>>> when `size` is greater than 356.
>>>>>>>
>>>>>>> In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
>>>>>>> unfortunately does not initialize that 4-byte hole. Fix it by using
>>>>>>> memset() instead.
>>>>>>>
>>>>>>> Cc: stable@vger.kernel.org
>>>>>>> Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
>>>>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>>>>>>> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>>> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>>>
>>>>>> I can't count how many of those we have fixed over the years.
>>>>>>
>>>>>> At some point we should probably document that using "= {}" or "= { 0 }"
>>>>>> in the kernel is a really bad idea and should be avoided.
>>>>> Moreover, it seems like different compilers seem to behave relatively
>>>>> differently with these and we often get reports of warnings with these
>>>>> on clang.  When in doubt, memset.
>>>> There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/"
>>>> drm*.c files,
>>>>
>>>> $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *(|NULL|0) *}" \{\} \+ | wc -l
>>>> 374
>>>> $_
>>>>
>>>> Out of which only 16 are of the non-ISO C variety, "= {}",
>>>>
>>>> $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *}" \{\} \+ | wc -l
>>>> 16
>>>> $_
>>>>
>>>> Perhaps the latter are the more pressing ones, since it is a C++ initializer and not a ISO C one.
>>> It only matters when we care copying the data to userspace, if it all
>>> stays in the kernel, all is fine.
>> Well only as long as you don't try to compute a CRC32, MD5 or any
>> fingerprint for a hash from the bytes from the structure.
>>
>> Then it fails horrible and you wonder why the code doesn't works as
>> expected.
> True, but the number of times I have ever needed to do that to a
> structure for a driver is, um, never...
>
> If a structure ever needs to have that happen to it, I would sure hope
> the developer was aware of padding fields, otherwise, well, someone
> needs to take away their C language certification :)

Well it is very likely that stack allocated structures have the same 
values in the padding bytes most of the time. So the problem is very 
subtle and hard to detect.

We've seen enough problems with that over the last ~10 years that I'm 
clearly in favor of adding something to checkpatch.pl to spill out a 
warning if "= { }" is used for zero initialization.

Alternatively some of the people who know gcc/clang better than I do 
could come up with a warning that you shouldn't cast a structure with 
uninitialized padding to void* or u8*.

I mean KASAN is already doing a great job detecting that kind of stuff, 
but for this you still need to hit the offending code path.

Thanks,
Christian.

>
> thanks,
>
> greg k-h

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-07-31  8:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 19:29 [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl() Peilin Ye
2020-07-29  8:11 ` Christian König
2020-07-29  9:00   ` Daniel Vetter
2020-07-29 13:49   ` Alex Deucher
2020-07-30 21:09     ` Luben Tuikov
2020-07-31  6:53       ` Greg Kroah-Hartman
2020-07-31  6:57         ` Christian König
2020-07-31  7:10           ` Greg Kroah-Hartman
2020-07-31  7:57             ` Christian König
2020-07-31  7:34       ` Arnd Bergmann
2020-07-29 21:55   ` Alex Deucher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).