[PATCH 1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by

[Alex Deucher]

On Mon, Sep 25, 2023 at 1:52 PM Kees Cook <keescook at chromium.org> wrote:
>
> On Mon, Sep 25, 2023 at 08:30:30AM +0200, Christian König wrote:
> > Am 22.09.23 um 19:41 schrieb Alex Deucher:
> > > On Fri, Sep 22, 2023 at 1:32 PM Kees Cook <keescook at chromium.org> 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 smu10_voltage_dependency_table.
> > > >
> > > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> > > >
> > > > Cc: Evan Quan <evan.quan at amd.com>
> > > > Cc: Alex Deucher <alexander.deucher at amd.com>
> > > > Cc: "Christian König" <christian.koenig at amd.com>
> > > > Cc: "Pan, Xinhui" <Xinhui.Pan at amd.com>
> > > > Cc: David Airlie <airlied at gmail.com>
> > > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > > Cc: Xiaojian Du <Xiaojian.Du at amd.com>
> > > > Cc: Huang Rui <ray.huang at amd.com>
> > > > Cc: Kevin Wang <kevin1.wang at amd.com>
> > > > Cc: amd-gfx at lists.freedesktop.org
> > > > Cc: dri-devel at lists.freedesktop.org
> > > > Signed-off-by: Kees Cook <keescook at chromium.org>
> > > Acked-by: Alex Deucher <alexander.deucher at amd.com>
> >
> > Mhm, I'm not sure if this is a good idea. That is a structure filled in by
> > the firmware, isn't it?
> >
> > That would imply that we might need to byte swap count before it is
> > checkable.
>
> The script found this instance because of this:
>
> static int smu10_get_clock_voltage_dependency_table(struct pp_hwmgr *hwmgr,
>                         struct smu10_voltage_dependency_table **pptable,
>                         uint32_t num_entry, const DpmClock_t *pclk_dependency_table)
> {
>         uint32_t i;
>         struct smu10_voltage_dependency_table *ptable;
>
>         ptable = kzalloc(struct_size(ptable, entries, num_entry), GFP_KERNEL);
>         if (NULL == ptable)
>                 return -ENOMEM;
>
>         ptable->count = num_entry;
>
> So the implication is that it's native byte order... but you tell me! I
> certainly don't want this annotation if it's going to break stuff. :)

In this case, the code is for an integrated GPU in an x86 CPU so the
firmware and driver endianness match.  You wouldn't find a stand alone
dGPU that uses this structure.  In this case it's ok.  False alarm.

Alex