Re: [Nouveau] [PATCH 1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
On Mon, Sep 25, 2023 at 1:52 PM Kees Cook 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 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 > > > > Cc: Alex Deucher > > > > Cc: "Christian König" > > > > Cc: "Pan, Xinhui" > > > > Cc: David Airlie > > > > Cc: Daniel Vetter > > > > Cc: Xiaojian Du > > > > Cc: Huang Rui > > > > Cc: Kevin Wang > > > > Cc: amd-...@lists.freedesktop.org > > > > Cc: dri-de...@lists.freedesktop.org > > > > Signed-off-by: Kees Cook > > > Acked-by: Alex Deucher > > > > 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
Re: [Nouveau] [PATCH 1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
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 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 > > > Cc: Alex Deucher > > > Cc: "Christian König" > > > Cc: "Pan, Xinhui" > > > Cc: David Airlie > > > Cc: Daniel Vetter > > > Cc: Xiaojian Du > > > Cc: Huang Rui > > > Cc: Kevin Wang > > > Cc: amd-...@lists.freedesktop.org > > > Cc: dri-de...@lists.freedesktop.org > > > Signed-off-by: Kees Cook > > Acked-by: Alex Deucher > > 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. :) -- Kees Cook
Re: [Nouveau] [PATCH 1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
On Mon, Sep 25, 2023 at 10:07 AM Alex Deucher wrote: > > On Mon, Sep 25, 2023 at 2:30 AM 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 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 > > >> Cc: Alex Deucher > > >> Cc: "Christian König" > > >> Cc: "Pan, Xinhui" > > >> Cc: David Airlie > > >> Cc: Daniel Vetter > > >> Cc: Xiaojian Du > > >> Cc: Huang Rui > > >> Cc: Kevin Wang > > >> Cc: amd-...@lists.freedesktop.org > > >> Cc: dri-de...@lists.freedesktop.org > > >> Signed-off-by: Kees Cook > > > Acked-by: Alex Deucher > > > > 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. > > True. Good point. Same for the other amdgpu patch. Actually the other patch is fine. That's just a local structure. Alex > > Alex > > > > > Regards, > > Christian. > > > > > > > >> --- > > >> drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h > > >> b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h > > >> index 808e0ecbe1f0..42adc2a3dcbc 100644 > > >> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h > > >> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h > > >> @@ -192,7 +192,7 @@ struct smu10_clock_voltage_dependency_record { > > >> > > >> struct smu10_voltage_dependency_table { > > >> uint32_t count; > > >> - struct smu10_clock_voltage_dependency_record entries[]; > > >> + struct smu10_clock_voltage_dependency_record entries[] > > >> __counted_by(count); > > >> }; > > >> > > >> struct smu10_clock_voltage_information { > > >> -- > > >> 2.34.1 > > >> > >
Re: [Nouveau] [PATCH 1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
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 smu10_voltage_dependency_table. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Evan Quan Cc: Alex Deucher Cc: "Christian König" Cc: "Pan, Xinhui" Cc: David Airlie Cc: Daniel Vetter Cc: Xiaojian Du Cc: Huang Rui Cc: Kevin Wang Cc: amd-...@lists.freedesktop.org Cc: dri-de...@lists.freedesktop.org Signed-off-by: Kees Cook Reviewed-by: Gustavo A. R. Silva Thanks -- Gustavo --- drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h index 808e0ecbe1f0..42adc2a3dcbc 100644 --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h @@ -192,7 +192,7 @@ struct smu10_clock_voltage_dependency_record { struct smu10_voltage_dependency_table { uint32_t count; - struct smu10_clock_voltage_dependency_record entries[]; + struct smu10_clock_voltage_dependency_record entries[] __counted_by(count); }; struct smu10_clock_voltage_information {