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(-)
--
2.34.1
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 <[email protected]>
Cc: Alex Deucher <[email protected]>
Cc: "Christian König" <[email protected]>
Cc: "Pan, Xinhui" <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Xiaojian Du <[email protected]>
Cc: Huang Rui <[email protected]>
Cc: Kevin Wang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
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 v3d_perfmon.
[1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Emma Anholt <[email protected]>
Cc: Melissa Wen <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
drivers/gpu/drm/v3d/v3d_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 7f664a4b2a75..106454f28956 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -59,7 +59,7 @@ struct v3d_perfmon {
* values can't be reset, but you can fake a reset by
* destroying the perfmon and creating a new one.
*/
- u64 values[];
+ u64 values[] __counted_by(ncounters);
};
struct v3d_dev {
--
2.34.1
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 <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index bf66499765fb..ab61e96e7e14 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -76,7 +76,7 @@ struct vc4_perfmon {
* Note that counter values can't be reset, but you can fake a reset by
* destroying the perfmon and creating a new one.
*/
- u64 counters[];
+ u64 counters[] __counted_by(ncounters);
};
struct vc4_dev {
--
2.34.1
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 perf_series.
[1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Jani Nikula <[email protected]>
Cc: Joonas Lahtinen <[email protected]>
Cc: Rodrigo Vivi <[email protected]>
Cc: Tvrtko Ursulin <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Chris Wilson <[email protected]>
Cc: John Harrison <[email protected]>
Cc: Andi Shyti <[email protected]>
Cc: Matthew Brost <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
drivers/gpu/drm/i915/selftests/i915_request.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c b/drivers/gpu/drm/i915/selftests/i915_request.c
index a9b79888c193..acae30a04a94 100644
--- a/drivers/gpu/drm/i915/selftests/i915_request.c
+++ b/drivers/gpu/drm/i915/selftests/i915_request.c
@@ -1924,7 +1924,7 @@ struct perf_stats {
struct perf_series {
struct drm_i915_private *i915;
unsigned int nengines;
- struct intel_context *ce[];
+ struct intel_context *ce[] __counted_by(nengines);
};
static int cmp_u32(const void *A, const void *B)
--
2.34.1
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 <[email protected]>
Cc: VMware Graphics Reviewers <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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);
--
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 vc4_perfmon.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Emma Anholt <[email protected]>
> Cc: Maxime Ripard <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Gustavo A. R. Silva <[email protected]>
Thanks
--
Gustavo
> ---
> drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index bf66499765fb..ab61e96e7e14 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -76,7 +76,7 @@ struct vc4_perfmon {
> * Note that counter values can't be reset, but you can fake a reset by
> * destroying the perfmon and creating a new one.
> */
> - u64 counters[];
> + u64 counters[] __counted_by(ncounters);
> };
>
> struct vc4_dev {
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 dpu_hw_intr.
[1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Rob Clark <[email protected]>
Cc: Abhinav Kumar <[email protected]>
Cc: Dmitry Baryshkov <[email protected]>
Cc: Sean Paul <[email protected]>
Cc: Marijn Suijten <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
index dab761e54863..50cf9523d367 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
@@ -61,7 +61,7 @@ struct dpu_hw_intr {
void (*cb)(void *arg, int irq_idx);
void *arg;
atomic_t count;
- } irq_tbl[];
+ } irq_tbl[] __counted_by(total_irqs);
};
/**
--
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 <[email protected]>
> Cc: Alex Deucher <[email protected]>
> Cc: "Christian König" <[email protected]>
> Cc: "Pan, Xinhui" <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Xiaojian Du <[email protected]>
> Cc: Huang Rui <[email protected]>
> Cc: Kevin Wang <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Gustavo A. R. Silva <[email protected]>
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 {
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 vmw_surface_dirty.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Zack Rusin <[email protected]>
> Cc: VMware Graphics Reviewers <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Gustavo A. R. Silva <[email protected]>
Thanks
--
Gustavo
> ---
> 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);
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 nvkm_perfdom.
[1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Ben Skeggs <[email protected]>
Cc: Karol Herbst <[email protected]>
Cc: Lyude Paul <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h b/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
index 6ae25d3e7f45..c011227f7052 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
@@ -82,7 +82,7 @@ struct nvkm_perfdom {
u8 mode;
u32 clk;
u16 signal_nr;
- struct nvkm_perfsig signal[];
+ struct nvkm_perfsig signal[] __counted_by(signal_nr);
};
struct nvkm_funcdom {
--
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 nvkm_perfdom.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Ben Skeggs <[email protected]>
> Cc: Karol Herbst <[email protected]>
> Cc: Lyude Paul <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Gustavo A. R. Silva <[email protected]>
Thanks
--
Gustavo
> ---
> drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h b/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
> index 6ae25d3e7f45..c011227f7052 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
> @@ -82,7 +82,7 @@ struct nvkm_perfdom {
> u8 mode;
> u32 clk;
> u16 signal_nr;
> - struct nvkm_perfsig signal[];
> + struct nvkm_perfsig signal[] __counted_by(signal_nr);
> };
>
> struct nvkm_funcdom {
On 22.09.2023 19: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 perf_series.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Jani Nikula <[email protected]>
> Cc: Joonas Lahtinen <[email protected]>
> Cc: Rodrigo Vivi <[email protected]>
> Cc: Tvrtko Ursulin <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Chris Wilson <[email protected]>
> Cc: John Harrison <[email protected]>
> Cc: Andi Shyti <[email protected]>
> Cc: Matthew Brost <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
I am surprised this is the only finding in i915, I would expected more.
Anyway:
Reviewed-by: Andrzej Hajda <[email protected]>
Regards
Andrzej
> ---
> drivers/gpu/drm/i915/selftests/i915_request.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c b/drivers/gpu/drm/i915/selftests/i915_request.c
> index a9b79888c193..acae30a04a94 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_request.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_request.c
> @@ -1924,7 +1924,7 @@ struct perf_stats {
> struct perf_series {
> struct drm_i915_private *i915;
> unsigned int nengines;
> - struct intel_context *ce[];
> + struct intel_context *ce[] __counted_by(nengines);
> };
>
> static int cmp_u32(const void *A, const void *B)
Hi Kees,
On Fri, Sep 22, 2023 at 10:32:08AM -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 perf_series.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Jani Nikula <[email protected]>
> Cc: Joonas Lahtinen <[email protected]>
> Cc: Rodrigo Vivi <[email protected]>
> Cc: Tvrtko Ursulin <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Chris Wilson <[email protected]>
> Cc: John Harrison <[email protected]>
> Cc: Andi Shyti <[email protected]>
> Cc: Matthew Brost <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
Thanks,
Andi
On Mon, Sep 25, 2023 at 12:08:36PM +0200, Andrzej Hajda wrote:
>
>
> On 22.09.2023 19: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 perf_series.
> >
> > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> >
> > Cc: Jani Nikula <[email protected]>
> > Cc: Joonas Lahtinen <[email protected]>
> > Cc: Rodrigo Vivi <[email protected]>
> > Cc: Tvrtko Ursulin <[email protected]>
> > Cc: David Airlie <[email protected]>
> > Cc: Daniel Vetter <[email protected]>
> > Cc: Chris Wilson <[email protected]>
> > Cc: John Harrison <[email protected]>
> > Cc: Andi Shyti <[email protected]>
> > Cc: Matthew Brost <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
>
> I am surprised this is the only finding in i915, I would expected more.
I'm sure there are more, but it's likely my Coccinelle pattern didn't
catch it. There are many many flexible arrays in drm. :)
$ grep -nRH '\[\];$' drivers/gpu/drm include/uapi/drm | grep -v :extern | wc -l
122
If anyone has some patterns I can add to the Coccinelle script, I can
take another pass at it.
> Anyway:
>
> Reviewed-by: Andrzej Hajda <[email protected]>
Thank you!
-Kees
--
Kees Cook
Hi Kees,
On 9/22/23 14: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 v3d_perfmon.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Emma Anholt <[email protected]>
> Cc: Melissa Wen <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Maíra Canal <[email protected]>
Best Regards,
- Maíra
> ---
> drivers/gpu/drm/v3d/v3d_drv.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
> index 7f664a4b2a75..106454f28956 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> @@ -59,7 +59,7 @@ struct v3d_perfmon {
> * values can't be reset, but you can fake a reset by
> * destroying the perfmon and creating a new one.
> */
> - u64 values[];
> + u64 values[] __counted_by(ncounters);
> };
>
> struct v3d_dev {
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,
--
Kees Cook
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,
>
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
>> <[email protected]> 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
> > > <[email protected]> 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
--
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
> <[email protected]> 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,
> > >
> >
--
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
> > > > > <[email protected]> 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
--
Kees Cook
On Mon, Oct 2, 2023 at 5:20 AM Christian König
<[email protected]> 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,
> >
>
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
>>>> <[email protected]> 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
>
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
>>>>>> <[email protected]> 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
> > > > > > > <[email protected]> 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.
--
Kees Cook