| Message ID | 20230922173216.3823169-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Annotate structs with __counted_by | expand |
On Fri, Sep 22, 2023 at 1:32 PM Kees Cook <keescook@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@amd.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: "Christian König" <christian.koenig@amd.com> > Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com> > Cc: David Airlie <airlied@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Xiaojian Du <Xiaojian.Du@amd.com> > Cc: Huang Rui <ray.huang@amd.com> > Cc: Kevin Wang <kevin1.wang@amd.com> > Cc: amd-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Kees Cook <keescook@chromium.org> Acked-by: Alex Deucher <alexander.deucher@amd.com> > --- > 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 >
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 <evan.quan@amd.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: "Christian König" <christian.koenig@amd.com> > Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com> > Cc: David Airlie <airlied@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Xiaojian Du <Xiaojian.Du@amd.com> > Cc: Huang Rui <ray.huang@amd.com> > Cc: Kevin Wang <kevin1.wang@amd.com> > Cc: amd-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks
Am 22.09.23 um 19:41 schrieb Alex Deucher: > On Fri, Sep 22, 2023 at 1:32 PM Kees Cook <keescook@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@amd.com> >> Cc: Alex Deucher <alexander.deucher@amd.com> >> Cc: "Christian König" <christian.koenig@amd.com> >> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com> >> Cc: David Airlie <airlied@gmail.com> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Cc: Xiaojian Du <Xiaojian.Du@amd.com> >> Cc: Huang Rui <ray.huang@amd.com> >> Cc: Kevin Wang <kevin1.wang@amd.com> >> Cc: amd-gfx@lists.freedesktop.org >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: Kees Cook <keescook@chromium.org> > Acked-by: Alex Deucher <alexander.deucher@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. 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 >>
On Mon, Sep 25, 2023 at 2:30 AM Christian König <christian.koenig@amd.com> wrote: > > Am 22.09.23 um 19:41 schrieb Alex Deucher: > > On Fri, Sep 22, 2023 at 1:32 PM Kees Cook <keescook@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@amd.com> > >> Cc: Alex Deucher <alexander.deucher@amd.com> > >> Cc: "Christian König" <christian.koenig@amd.com> > >> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com> > >> Cc: David Airlie <airlied@gmail.com> > >> Cc: Daniel Vetter <daniel@ffwll.ch> > >> Cc: Xiaojian Du <Xiaojian.Du@amd.com> > >> Cc: Huang Rui <ray.huang@amd.com> > >> Cc: Kevin Wang <kevin1.wang@amd.com> > >> Cc: amd-gfx@lists.freedesktop.org > >> Cc: dri-devel@lists.freedesktop.org > >> Signed-off-by: Kees Cook <keescook@chromium.org> > > Acked-by: Alex Deucher <alexander.deucher@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. True. Good point. Same for the other amdgpu patch. 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 > >> >
On Mon, Sep 25, 2023 at 10:07 AM Alex Deucher <alexdeucher@gmail.com> wrote: > > On Mon, Sep 25, 2023 at 2:30 AM Christian König > <christian.koenig@amd.com> wrote: > > > > Am 22.09.23 um 19:41 schrieb Alex Deucher: > > > On Fri, Sep 22, 2023 at 1:32 PM Kees Cook <keescook@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@amd.com> > > >> Cc: Alex Deucher <alexander.deucher@amd.com> > > >> Cc: "Christian König" <christian.koenig@amd.com> > > >> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com> > > >> Cc: David Airlie <airlied@gmail.com> > > >> Cc: Daniel Vetter <daniel@ffwll.ch> > > >> Cc: Xiaojian Du <Xiaojian.Du@amd.com> > > >> Cc: Huang Rui <ray.huang@amd.com> > > >> Cc: Kevin Wang <kevin1.wang@amd.com> > > >> Cc: amd-gfx@lists.freedesktop.org > > >> Cc: dri-devel@lists.freedesktop.org > > >> Signed-off-by: Kees Cook <keescook@chromium.org> > > > Acked-by: Alex Deucher <alexander.deucher@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. > > 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 > > >> > >
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@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@amd.com> > > > Cc: Alex Deucher <alexander.deucher@amd.com> > > > Cc: "Christian König" <christian.koenig@amd.com> > > > Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com> > > > Cc: David Airlie <airlied@gmail.com> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Cc: Xiaojian Du <Xiaojian.Du@amd.com> > > > Cc: Huang Rui <ray.huang@amd.com> > > > Cc: Kevin Wang <kevin1.wang@amd.com> > > > Cc: amd-gfx@lists.freedesktop.org > > > Cc: dri-devel@lists.freedesktop.org > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Acked-by: Alex Deucher <alexander.deucher@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. :)
On Mon, Sep 25, 2023 at 1:52 PM Kees Cook <keescook@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@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@amd.com> > > > > Cc: Alex Deucher <alexander.deucher@amd.com> > > > > Cc: "Christian König" <christian.koenig@amd.com> > > > > Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com> > > > > Cc: David Airlie <airlied@gmail.com> > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > Cc: Xiaojian Du <Xiaojian.Du@amd.com> > > > > Cc: Huang Rui <ray.huang@amd.com> > > > > Cc: Kevin Wang <kevin1.wang@amd.com> > > > > Cc: amd-gfx@lists.freedesktop.org > > > > Cc: dri-devel@lists.freedesktop.org > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > Acked-by: Alex Deucher <alexander.deucher@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
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 {
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@amd.com> Cc: Alex Deucher <alexander.deucher@amd.com> Cc: "Christian König" <christian.koenig@amd.com> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com> Cc: David Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Xiaojian Du <Xiaojian.Du@amd.com> Cc: Huang Rui <ray.huang@amd.com> Cc: Kevin Wang <kevin1.wang@amd.com> Cc: amd-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)