Re: [ovs-dev] [PATCH 08/14] net: ipa: Annotate struct ipa_power with __counted_by
On Sat, Sep 23, 2023 at 07:09:19AM -0500, Alex Elder wrote: > On 9/22/23 12:28 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 ipa_power. > > Looks good, thanks. > > Reviewed-by: Alex Elder > > Note that there is some interaction between struct ipa_power_data > and struct ipa_power (the former is used to initialize the latter). > Both of these contain flexible arrays counted by another field in > the structure. It seems possible that the way these are initialized > might need slight modification to allow the compiler to do its > enforcement; if that's the case, please reach out to me. I think it's all okay: struct ipa_power_data { u32 core_clock_rate; u32 interconnect_count; /* # entries in interconnect_data[] */ const struct ipa_interconnect_data *interconnect_data; }; "interconnect_data" here is a pointer, not a flexible array. (Yes, __counted_by is expected to be expanded in the future for pointers, but not yet.) Looking at initializers, I didn't see any problems with how struct ipa_power is allocated. Thanks for the heads-up; I'm sure I'll look at this again when we can further expand __counted_by to pointers. :) -Kees -- Kees Cook ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 08/14] net: ipa: Annotate struct ipa_power with __counted_by
On 9/22/23 11:28, 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 ipa_power. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Alex Elder Cc: "David S. Miller" Cc: Eric Dumazet Cc: Jakub Kicinski Cc: Paolo Abeni Cc: net...@vger.kernel.org Signed-off-by: Kees Cook Reviewed-by: Gustavo A. R. Silva Thanks -- Gustavo --- drivers/net/ipa/ipa_power.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c index 0eaa7a7f3343..e223886123ce 100644 --- a/drivers/net/ipa/ipa_power.c +++ b/drivers/net/ipa/ipa_power.c @@ -67,7 +67,7 @@ struct ipa_power { spinlock_t spinlock;/* used with STOPPED/STARTED power flags */ DECLARE_BITMAP(flags, IPA_POWER_FLAG_COUNT); u32 interconnect_count; - struct icc_bulk_data interconnect[]; + struct icc_bulk_data interconnect[] __counted_by(interconnect_count); }; /* Initialize interconnects required for IPA operation */ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 08/14] net: ipa: Annotate struct ipa_power with __counted_by
On 9/22/23 12:28 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 ipa_power. Looks good, thanks. Reviewed-by: Alex Elder Note that there is some interaction between struct ipa_power_data and struct ipa_power (the former is used to initialize the latter). Both of these contain flexible arrays counted by another field in the structure. It seems possible that the way these are initialized might need slight modification to allow the compiler to do its enforcement; if that's the case, please reach out to me. -Alex [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Alex Elder Cc: "David S. Miller" Cc: Eric Dumazet Cc: Jakub Kicinski Cc: Paolo Abeni Cc: net...@vger.kernel.org Signed-off-by: Kees Cook --- drivers/net/ipa/ipa_power.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c index 0eaa7a7f3343..e223886123ce 100644 --- a/drivers/net/ipa/ipa_power.c +++ b/drivers/net/ipa/ipa_power.c @@ -67,7 +67,7 @@ struct ipa_power { spinlock_t spinlock;/* used with STOPPED/STARTED power flags */ DECLARE_BITMAP(flags, IPA_POWER_FLAG_COUNT); u32 interconnect_count; - struct icc_bulk_data interconnect[]; + struct icc_bulk_data interconnect[] __counted_by(interconnect_count); }; /* Initialize interconnects required for IPA operation */ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 08/14] net: ipa: Annotate struct ipa_power with __counted_by
Bleep bloop. Greetings Kees Cook, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 08/14] net: ipa: Annotate struct ipa_power with __counted_by
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 ipa_power. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Alex Elder Cc: "David S. Miller" Cc: Eric Dumazet Cc: Jakub Kicinski Cc: Paolo Abeni Cc: net...@vger.kernel.org Signed-off-by: Kees Cook --- drivers/net/ipa/ipa_power.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c index 0eaa7a7f3343..e223886123ce 100644 --- a/drivers/net/ipa/ipa_power.c +++ b/drivers/net/ipa/ipa_power.c @@ -67,7 +67,7 @@ struct ipa_power { spinlock_t spinlock;/* used with STOPPED/STARTED power flags */ DECLARE_BITMAP(flags, IPA_POWER_FLAG_COUNT); u32 interconnect_count; - struct icc_bulk_data interconnect[]; + struct icc_bulk_data interconnect[] __counted_by(interconnect_count); }; /* Initialize interconnects required for IPA operation */ -- 2.34.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev