[0/9] drm: Annotate structs with __counted_by

  Hi,

This is a batch of patches touching drm for preparing for the coming
implementation by GCC and Clang of the __counted_by attribute. Flexible
array members annotated with __counted_by can have their accesses
bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).

As found with Coccinelle[1], add __counted_by to structs that would
benefit from the annotation.

Since the element count member must be set before accessing the annotated
flexible array member, some patches also move the member's initialization
earlier. (These are noted in the individual patches.)

-Kees

[1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci

Kees Cook (9):
  drm/amd/pm: Annotate struct smu10_voltage_dependency_table with
    __counted_by
  drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by
  drm/i915/selftests: Annotate struct perf_series with __counted_by
  drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by
  drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by
  drm/vc4: Annotate struct vc4_perfmon with __counted_by
  drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by
  drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by
  drm/v3d: Annotate struct v3d_perfmon with __counted_by

 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c        | 2 +-
 drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h | 2 +-
 drivers/gpu/drm/i915/selftests/i915_request.c        | 2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h    | 2 +-
 drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h        | 2 +-
 drivers/gpu/drm/v3d/v3d_drv.h                        | 2 +-
 drivers/gpu/drm/vc4/vc4_drv.h                        | 2 +-
 drivers/gpu/drm/virtio/virtgpu_drv.h                 | 2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c              | 2 +-
 9 files changed, 9 insertions(+), 9 deletions(-)
  

On Fri, 2023-09-22 at 10:32 -0700, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
> 
> As found with Coccinelle[1], add __counted_by for struct vmw_surface_dirty.
> 
> [1]
> https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> 
> Cc: Zack Rusin <zackr@vmware.com>
> Cc: VMware Graphics Reviewers <linux-graphics-maintainer@vmware.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index 5db403ee8261..2d1d857f99ae 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -77,7 +77,7 @@ struct vmw_surface_offset {
>  struct vmw_surface_dirty {
>         struct vmw_surface_cache cache;
>         u32 num_subres;
> -       SVGA3dBox boxes[];
> +       SVGA3dBox boxes[] __counted_by(num_subres);
>  };
>  
>  static void vmw_user_surface_free(struct vmw_resource *res);

Thanks!

Reviewed-by: Zack Rusin <zackr@vmware.com>
  

On 9/22/23 11:32, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
> 
> As found with Coccinelle[1], add __counted_by for struct vc4_perfmon.
> 
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> 
> Cc: Emma Anholt <emma@anholt.net>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks
  

On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote:
> This is a batch of patches touching drm for preparing for the coming
> implementation by GCC and Clang of the __counted_by attribute. Flexible
> array members annotated with __counted_by can have their accesses
> bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
> indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
> 
> As found with Coccinelle[1], add __counted_by to structs that would
> benefit from the annotation.
> 
> [...]

Since this got Acks, I figure I should carry it in my tree. Let me know
if this should go via drm instead.

Applied to for-next/hardening, thanks!

[1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
      https://git.kernel.org/kees/c/a6046ac659d6
[2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by
      https://git.kernel.org/kees/c/4df33089b46f
[3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by
      https://git.kernel.org/kees/c/ffd3f823bdf6
[4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by
      https://git.kernel.org/kees/c/2de35a989b76
[5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by
      https://git.kernel.org/kees/c/188aeb08bfaa
[6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by
      https://git.kernel.org/kees/c/59a54dc896c3
[7/9] drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by
      https://git.kernel.org/kees/c/5cd476de33af
[8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by
      https://git.kernel.org/kees/c/b426f2e5356a
[9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by
      https://git.kernel.org/kees/c/dc662fa1b0e4

Take care,
  

Am 29.09.23 um 21:33 schrieb Kees Cook:
> On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote:
>> This is a batch of patches touching drm for preparing for the coming
>> implementation by GCC and Clang of the __counted_by attribute. Flexible
>> array members annotated with __counted_by can have their accesses
>> bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
>> indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
>>
>> As found with Coccinelle[1], add __counted_by to structs that would
>> benefit from the annotation.
>>
>> [...]
> Since this got Acks, I figure I should carry it in my tree. Let me know
> if this should go via drm instead.
>
> Applied to for-next/hardening, thanks!
>
> [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
>        https://git.kernel.org/kees/c/a6046ac659d6

STOP! In a follow up discussion Alex and I figured out that this won't work.

The value in the structure is byte swapped based on some firmware 
endianness which not necessary matches the CPU endianness.

Please revert that one from going upstream if it's already on it's way.

And because of those reasons I strongly think that patches like this 
should go through the DRM tree :)

Regards,
Christian.

> [2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by
>        https://git.kernel.org/kees/c/4df33089b46f
> [3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by
>        https://git.kernel.org/kees/c/ffd3f823bdf6
> [4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by
>        https://git.kernel.org/kees/c/2de35a989b76
> [5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by
>        https://git.kernel.org/kees/c/188aeb08bfaa
> [6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by
>        https://git.kernel.org/kees/c/59a54dc896c3
> [7/9] drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by
>        https://git.kernel.org/kees/c/5cd476de33af
> [8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by
>        https://git.kernel.org/kees/c/b426f2e5356a
> [9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by
>        https://git.kernel.org/kees/c/dc662fa1b0e4
>
> Take care,
>
  

On Mon, Oct 2, 2023 at 5:20 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 29.09.23 um 21:33 schrieb Kees Cook:
> > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote:
> >> This is a batch of patches touching drm for preparing for the coming
> >> implementation by GCC and Clang of the __counted_by attribute. Flexible
> >> array members annotated with __counted_by can have their accesses
> >> bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
> >> indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
> >>
> >> As found with Coccinelle[1], add __counted_by to structs that would
> >> benefit from the annotation.
> >>
> >> [...]
> > Since this got Acks, I figure I should carry it in my tree. Let me know
> > if this should go via drm instead.
> >
> > Applied to for-next/hardening, thanks!
> >
> > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
> >        https://git.kernel.org/kees/c/a6046ac659d6
>
> STOP! In a follow up discussion Alex and I figured out that this won't work.
>
> The value in the structure is byte swapped based on some firmware
> endianness which not necessary matches the CPU endianness.

SMU10 is APU only so the endianess of the SMU firmware and the CPU
will always match.

Alex

>
> Please revert that one from going upstream if it's already on it's way.
>
> And because of those reasons I strongly think that patches like this
> should go through the DRM tree :)
>
> Regards,
> Christian.
>
> > [2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by
> >        https://git.kernel.org/kees/c/4df33089b46f
> > [3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by
> >        https://git.kernel.org/kees/c/ffd3f823bdf6
> > [4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by
> >        https://git.kernel.org/kees/c/2de35a989b76
> > [5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by
> >        https://git.kernel.org/kees/c/188aeb08bfaa
> > [6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by
> >        https://git.kernel.org/kees/c/59a54dc896c3
> > [7/9] drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by
> >        https://git.kernel.org/kees/c/5cd476de33af
> > [8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by
> >        https://git.kernel.org/kees/c/b426f2e5356a
> > [9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by
> >        https://git.kernel.org/kees/c/dc662fa1b0e4
> >
> > Take care,
> >
>
  

On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote:
> On Mon, Oct 2, 2023 at 5:20 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
> >
> > Am 29.09.23 um 21:33 schrieb Kees Cook:
> > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote:
> > >> This is a batch of patches touching drm for preparing for the coming
> > >> implementation by GCC and Clang of the __counted_by attribute. Flexible
> > >> array members annotated with __counted_by can have their accesses
> > >> bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
> > >> indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
> > >>
> > >> As found with Coccinelle[1], add __counted_by to structs that would
> > >> benefit from the annotation.
> > >>
> > >> [...]
> > > Since this got Acks, I figure I should carry it in my tree. Let me know
> > > if this should go via drm instead.
> > >
> > > Applied to for-next/hardening, thanks!
> > >
> > > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
> > >        https://git.kernel.org/kees/c/a6046ac659d6
> >
> > STOP! In a follow up discussion Alex and I figured out that this won't work.

I'm so confused; from the discussion I saw that Alex said both instances
were false positives?

> >
> > The value in the structure is byte swapped based on some firmware
> > endianness which not necessary matches the CPU endianness.
> 
> SMU10 is APU only so the endianess of the SMU firmware and the CPU
> will always match.

Which I think is what is being said here?

> > Please revert that one from going upstream if it's already on it's way.
> >
> > And because of those reasons I strongly think that patches like this
> > should go through the DRM tree :)

Sure, that's fine -- please let me know. It was others Acked/etc. Who
should carry these patches?

Thanks!

-Kees


> >
> > Regards,
> > Christian.
> >
> > > [2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by
> > >        https://git.kernel.org/kees/c/4df33089b46f
> > > [3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by
> > >        https://git.kernel.org/kees/c/ffd3f823bdf6
> > > [4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by
> > >        https://git.kernel.org/kees/c/2de35a989b76
> > > [5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by
> > >        https://git.kernel.org/kees/c/188aeb08bfaa
> > > [6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by
> > >        https://git.kernel.org/kees/c/59a54dc896c3
> > > [7/9] drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by
> > >        https://git.kernel.org/kees/c/5cd476de33af
> > > [8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by
> > >        https://git.kernel.org/kees/c/b426f2e5356a
> > > [9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by
> > >        https://git.kernel.org/kees/c/dc662fa1b0e4
> > >
> > > Take care,
> > >
> >
  

Am 02.10.23 um 18:53 schrieb Kees Cook:
> On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote:
>> On Mon, Oct 2, 2023 at 5:20 AM Christian König
>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>> Am 29.09.23 um 21:33 schrieb Kees Cook:
>>>> On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote:
>>>>> This is a batch of patches touching drm for preparing for the coming
>>>>> implementation by GCC and Clang of the __counted_by attribute. Flexible
>>>>> array members annotated with __counted_by can have their accesses
>>>>> bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
>>>>> indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
>>>>>
>>>>> As found with Coccinelle[1], add __counted_by to structs that would
>>>>> benefit from the annotation.
>>>>>
>>>>> [...]
>>>> Since this got Acks, I figure I should carry it in my tree. Let me know
>>>> if this should go via drm instead.
>>>>
>>>> Applied to for-next/hardening, thanks!
>>>>
>>>> [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
>>>>         https://git.kernel.org/kees/c/a6046ac659d6
>>> STOP! In a follow up discussion Alex and I figured out that this won't work.
> I'm so confused; from the discussion I saw that Alex said both instances
> were false positives?
>
>>> The value in the structure is byte swapped based on some firmware
>>> endianness which not necessary matches the CPU endianness.
>> SMU10 is APU only so the endianess of the SMU firmware and the CPU
>> will always match.
> Which I think is what is being said here?
>
>>> Please revert that one from going upstream if it's already on it's way.
>>>
>>> And because of those reasons I strongly think that patches like this
>>> should go through the DRM tree :)
> Sure, that's fine -- please let me know. It was others Acked/etc. Who
> should carry these patches?

Probably best if the relevant maintainer pick them up individually.

Some of those structures are filled in by firmware/hardware and only the 
maintainers can judge if that value actually matches what the compiler 
needs.

We have cases where individual bits are used as flags or when the size 
is byte swapped etc...

Even Alex and I didn't immediately say how and where that field is 
actually used and had to dig that up. That's where the confusion came from.

Regards,
Christian.

>
> Thanks!
>
> -Kees
>
>
>>> Regards,
>>> Christian.
>>>
>>>> [2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by
>>>>         https://git.kernel.org/kees/c/4df33089b46f
>>>> [3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by
>>>>         https://git.kernel.org/kees/c/ffd3f823bdf6
>>>> [4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by
>>>>         https://git.kernel.org/kees/c/2de35a989b76
>>>> [5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by
>>>>         https://git.kernel.org/kees/c/188aeb08bfaa
>>>> [6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by
>>>>         https://git.kernel.org/kees/c/59a54dc896c3
>>>> [7/9] drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by
>>>>         https://git.kernel.org/kees/c/5cd476de33af
>>>> [8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by
>>>>         https://git.kernel.org/kees/c/b426f2e5356a
>>>> [9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by
>>>>         https://git.kernel.org/kees/c/dc662fa1b0e4
>>>>
>>>> Take care,
>>>>
  

On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote:
> Am 02.10.23 um 18:53 schrieb Kees Cook:
> > On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote:
> > > On Mon, Oct 2, 2023 at 5:20 AM Christian König
> > > <ckoenig.leichtzumerken@gmail.com> wrote:
> > > > Am 29.09.23 um 21:33 schrieb Kees Cook:
> > > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote:
> > > > > > This is a batch of patches touching drm for preparing for the coming
> > > > > > implementation by GCC and Clang of the __counted_by attribute. Flexible
> > > > > > array members annotated with __counted_by can have their accesses
> > > > > > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
> > > > > > indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
> > > > > > 
> > > > > > As found with Coccinelle[1], add __counted_by to structs that would
> > > > > > benefit from the annotation.
> > > > > > 
> > > > > > [...]
> > > > > Since this got Acks, I figure I should carry it in my tree. Let me know
> > > > > if this should go via drm instead.
> > > > > 
> > > > > Applied to for-next/hardening, thanks!
> > > > > 
> > > > > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
> > > > >         https://git.kernel.org/kees/c/a6046ac659d6
> > > > STOP! In a follow up discussion Alex and I figured out that this won't work.
> > I'm so confused; from the discussion I saw that Alex said both instances
> > were false positives?
> > 
> > > > The value in the structure is byte swapped based on some firmware
> > > > endianness which not necessary matches the CPU endianness.
> > > SMU10 is APU only so the endianess of the SMU firmware and the CPU
> > > will always match.
> > Which I think is what is being said here?
> > 
> > > > Please revert that one from going upstream if it's already on it's way.
> > > > 
> > > > And because of those reasons I strongly think that patches like this
> > > > should go through the DRM tree :)
> > Sure, that's fine -- please let me know. It was others Acked/etc. Who
> > should carry these patches?
> 
> Probably best if the relevant maintainer pick them up individually.
> 
> Some of those structures are filled in by firmware/hardware and only the
> maintainers can judge if that value actually matches what the compiler
> needs.
> 
> We have cases where individual bits are used as flags or when the size is
> byte swapped etc...
> 
> Even Alex and I didn't immediately say how and where that field is actually
> used and had to dig that up. That's where the confusion came from.

Okay, I've dropped them all from my tree. Several had Acks/Reviews, so
hopefully those can get picked up for the DRM tree?

Thanks!

-Kees
  

Am 02.10.23 um 20:08 schrieb Kees Cook:
> On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote:
>> Am 02.10.23 um 18:53 schrieb Kees Cook:
>>> On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote:
>>>> On Mon, Oct 2, 2023 at 5:20 AM Christian König
>>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>>> Am 29.09.23 um 21:33 schrieb Kees Cook:
>>>>>> On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote:
>>>>>>> This is a batch of patches touching drm for preparing for the coming
>>>>>>> implementation by GCC and Clang of the __counted_by attribute. Flexible
>>>>>>> array members annotated with __counted_by can have their accesses
>>>>>>> bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
>>>>>>> indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
>>>>>>>
>>>>>>> As found with Coccinelle[1], add __counted_by to structs that would
>>>>>>> benefit from the annotation.
>>>>>>>
>>>>>>> [...]
>>>>>> Since this got Acks, I figure I should carry it in my tree. Let me know
>>>>>> if this should go via drm instead.
>>>>>>
>>>>>> Applied to for-next/hardening, thanks!
>>>>>>
>>>>>> [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
>>>>>>          https://git.kernel.org/kees/c/a6046ac659d6
>>>>> STOP! In a follow up discussion Alex and I figured out that this won't work.
>>> I'm so confused; from the discussion I saw that Alex said both instances
>>> were false positives?
>>>
>>>>> The value in the structure is byte swapped based on some firmware
>>>>> endianness which not necessary matches the CPU endianness.
>>>> SMU10 is APU only so the endianess of the SMU firmware and the CPU
>>>> will always match.
>>> Which I think is what is being said here?
>>>
>>>>> Please revert that one from going upstream if it's already on it's way.
>>>>>
>>>>> And because of those reasons I strongly think that patches like this
>>>>> should go through the DRM tree :)
>>> Sure, that's fine -- please let me know. It was others Acked/etc. Who
>>> should carry these patches?
>> Probably best if the relevant maintainer pick them up individually.
>>
>> Some of those structures are filled in by firmware/hardware and only the
>> maintainers can judge if that value actually matches what the compiler
>> needs.
>>
>> We have cases where individual bits are used as flags or when the size is
>> byte swapped etc...
>>
>> Even Alex and I didn't immediately say how and where that field is actually
>> used and had to dig that up. That's where the confusion came from.
> Okay, I've dropped them all from my tree. Several had Acks/Reviews, so
> hopefully those can get picked up for the DRM tree?

I will pick those up to go through drm-misc-next.

Going to ping maintainers once more when I'm not sure if stuff is 
correct or not.

Christian.

>
> Thanks!
>
> -Kees
>
  

On Mon, Oct 02, 2023 at 08:11:41PM +0200, Christian König wrote:
> Am 02.10.23 um 20:08 schrieb Kees Cook:
> > On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote:
> > > Am 02.10.23 um 18:53 schrieb Kees Cook:
> > > > On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote:
> > > > > On Mon, Oct 2, 2023 at 5:20 AM Christian König
> > > > > <ckoenig.leichtzumerken@gmail.com> wrote:
> > > > > > Am 29.09.23 um 21:33 schrieb Kees Cook:
> > > > > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote:
> > > > > > > > This is a batch of patches touching drm for preparing for the coming
> > > > > > > > implementation by GCC and Clang of the __counted_by attribute. Flexible
> > > > > > > > array members annotated with __counted_by can have their accesses
> > > > > > > > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
> > > > > > > > indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
> > > > > > > > 
> > > > > > > > As found with Coccinelle[1], add __counted_by to structs that would
> > > > > > > > benefit from the annotation.
> > > > > > > > 
> > > > > > > > [...]
> > > > > > > Since this got Acks, I figure I should carry it in my tree. Let me know
> > > > > > > if this should go via drm instead.
> > > > > > > 
> > > > > > > Applied to for-next/hardening, thanks!
> > > > > > > 
> > > > > > > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
> > > > > > >          https://git.kernel.org/kees/c/a6046ac659d6
> > > > > > STOP! In a follow up discussion Alex and I figured out that this won't work.
> > > > I'm so confused; from the discussion I saw that Alex said both instances
> > > > were false positives?
> > > > 
> > > > > > The value in the structure is byte swapped based on some firmware
> > > > > > endianness which not necessary matches the CPU endianness.
> > > > > SMU10 is APU only so the endianess of the SMU firmware and the CPU
> > > > > will always match.
> > > > Which I think is what is being said here?
> > > > 
> > > > > > Please revert that one from going upstream if it's already on it's way.
> > > > > > 
> > > > > > And because of those reasons I strongly think that patches like this
> > > > > > should go through the DRM tree :)
> > > > Sure, that's fine -- please let me know. It was others Acked/etc. Who
> > > > should carry these patches?
> > > Probably best if the relevant maintainer pick them up individually.
> > > 
> > > Some of those structures are filled in by firmware/hardware and only the
> > > maintainers can judge if that value actually matches what the compiler
> > > needs.
> > > 
> > > We have cases where individual bits are used as flags or when the size is
> > > byte swapped etc...
> > > 
> > > Even Alex and I didn't immediately say how and where that field is actually
> > > used and had to dig that up. That's where the confusion came from.
> > Okay, I've dropped them all from my tree. Several had Acks/Reviews, so
> > hopefully those can get picked up for the DRM tree?
> 
> I will pick those up to go through drm-misc-next.
> 
> Going to ping maintainers once more when I'm not sure if stuff is correct or
> not.

Sounds great; thanks!

-Kees
  

Am 02.10.23 um 20:22 schrieb Kees Cook:
> On Mon, Oct 02, 2023 at 08:11:41PM +0200, Christian König wrote:
>> Am 02.10.23 um 20:08 schrieb Kees Cook:
>>> On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote:
>>>> Am 02.10.23 um 18:53 schrieb Kees Cook:
>>>>> On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote:
>>>>>> On Mon, Oct 2, 2023 at 5:20 AM Christian König
>>>>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>>>>> Am 29.09.23 um 21:33 schrieb Kees Cook:
>>>>>>>> On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote:
>>>>>>>>> This is a batch of patches touching drm for preparing for the coming
>>>>>>>>> implementation by GCC and Clang of the __counted_by attribute. Flexible
>>>>>>>>> array members annotated with __counted_by can have their accesses
>>>>>>>>> bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
>>>>>>>>> indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
>>>>>>>>>
>>>>>>>>> As found with Coccinelle[1], add __counted_by to structs that would
>>>>>>>>> benefit from the annotation.
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>> Since this got Acks, I figure I should carry it in my tree. Let me know
>>>>>>>> if this should go via drm instead.
>>>>>>>>
>>>>>>>> Applied to for-next/hardening, thanks!
>>>>>>>>
>>>>>>>> [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
>>>>>>>>           https://git.kernel.org/kees/c/a6046ac659d6
>>>>>>> STOP! In a follow up discussion Alex and I figured out that this won't work.
>>>>> I'm so confused; from the discussion I saw that Alex said both instances
>>>>> were false positives?
>>>>>
>>>>>>> The value in the structure is byte swapped based on some firmware
>>>>>>> endianness which not necessary matches the CPU endianness.
>>>>>> SMU10 is APU only so the endianess of the SMU firmware and the CPU
>>>>>> will always match.
>>>>> Which I think is what is being said here?
>>>>>
>>>>>>> Please revert that one from going upstream if it's already on it's way.
>>>>>>>
>>>>>>> And because of those reasons I strongly think that patches like this
>>>>>>> should go through the DRM tree :)
>>>>> Sure, that's fine -- please let me know. It was others Acked/etc. Who
>>>>> should carry these patches?
>>>> Probably best if the relevant maintainer pick them up individually.
>>>>
>>>> Some of those structures are filled in by firmware/hardware and only the
>>>> maintainers can judge if that value actually matches what the compiler
>>>> needs.
>>>>
>>>> We have cases where individual bits are used as flags or when the size is
>>>> byte swapped etc...
>>>>
>>>> Even Alex and I didn't immediately say how and where that field is actually
>>>> used and had to dig that up. That's where the confusion came from.
>>> Okay, I've dropped them all from my tree. Several had Acks/Reviews, so
>>> hopefully those can get picked up for the DRM tree?
>> I will pick those up to go through drm-misc-next.
>>
>> Going to ping maintainers once more when I'm not sure if stuff is correct or
>> not.
> Sounds great; thanks!

I wasn't 100% sure for the VC4 patch, but pushed the whole set to 
drm-misc-next anyway.

This also means that the patches are now auto merged into the drm-tip 
integration branch and should any build or unit test go boom we should 
notice immediately and can revert it pretty easily.

Thanks,
Christian.

>
> -Kees
>
  

On Thu, Oct 05, 2023 at 11:42:38AM +0200, Christian König wrote:
> Am 02.10.23 um 20:22 schrieb Kees Cook:
> > On Mon, Oct 02, 2023 at 08:11:41PM +0200, Christian König wrote:
> > > Am 02.10.23 um 20:08 schrieb Kees Cook:
> > > > On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote:
> > > > > Am 02.10.23 um 18:53 schrieb Kees Cook:
> > > > > > On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote:
> > > > > > > On Mon, Oct 2, 2023 at 5:20 AM Christian König
> > > > > > > <ckoenig.leichtzumerken@gmail.com> wrote:
> > > > > > > > Am 29.09.23 um 21:33 schrieb Kees Cook:
> > > > > > > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote:
> > > > > > > > > > This is a batch of patches touching drm for preparing for the coming
> > > > > > > > > > implementation by GCC and Clang of the __counted_by attribute. Flexible
> > > > > > > > > > array members annotated with __counted_by can have their accesses
> > > > > > > > > > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array
> > > > > > > > > > indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
> > > > > > > > > > 
> > > > > > > > > > As found with Coccinelle[1], add __counted_by to structs that would
> > > > > > > > > > benefit from the annotation.
> > > > > > > > > > 
> > > > > > > > > > [...]
> > > > > > > > > Since this got Acks, I figure I should carry it in my tree. Let me know
> > > > > > > > > if this should go via drm instead.
> > > > > > > > > 
> > > > > > > > > Applied to for-next/hardening, thanks!
> > > > > > > > > 
> > > > > > > > > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
> > > > > > > > >           https://git.kernel.org/kees/c/a6046ac659d6
> > > > > > > > STOP! In a follow up discussion Alex and I figured out that this won't work.
> > > > > > I'm so confused; from the discussion I saw that Alex said both instances
> > > > > > were false positives?
> > > > > > 
> > > > > > > > The value in the structure is byte swapped based on some firmware
> > > > > > > > endianness which not necessary matches the CPU endianness.
> > > > > > > SMU10 is APU only so the endianess of the SMU firmware and the CPU
> > > > > > > will always match.
> > > > > > Which I think is what is being said here?
> > > > > > 
> > > > > > > > Please revert that one from going upstream if it's already on it's way.
> > > > > > > > 
> > > > > > > > And because of those reasons I strongly think that patches like this
> > > > > > > > should go through the DRM tree :)
> > > > > > Sure, that's fine -- please let me know. It was others Acked/etc. Who
> > > > > > should carry these patches?
> > > > > Probably best if the relevant maintainer pick them up individually.
> > > > > 
> > > > > Some of those structures are filled in by firmware/hardware and only the
> > > > > maintainers can judge if that value actually matches what the compiler
> > > > > needs.
> > > > > 
> > > > > We have cases where individual bits are used as flags or when the size is
> > > > > byte swapped etc...
> > > > > 
> > > > > Even Alex and I didn't immediately say how and where that field is actually
> > > > > used and had to dig that up. That's where the confusion came from.
> > > > Okay, I've dropped them all from my tree. Several had Acks/Reviews, so
> > > > hopefully those can get picked up for the DRM tree?
> > > I will pick those up to go through drm-misc-next.
> > > 
> > > Going to ping maintainers once more when I'm not sure if stuff is correct or
> > > not.
> > Sounds great; thanks!
> 
> I wasn't 100% sure for the VC4 patch, but pushed the whole set to
> drm-misc-next anyway.
> 
> This also means that the patches are now auto merged into the drm-tip
> integration branch and should any build or unit test go boom we should
> notice immediately and can revert it pretty easily.

Thanks very much; I'll keep an eye out for any reports.