[PATCH RFC kspp-next 0/3] compiler_types: add Endianness-dependent __counted_by_{le,be}

[PATCH RFC kspp-next 0/3] compiler_types: add Endianness-dependent __counted_by_{le,be}

[Alexander Lobakin]

Some structures contain flexible arrays at the end and the counter for
them, but the counter has explicit Endianness and thus __counted_by()
can't be used directly.

To increase test coverage for potential problems without breaking
anything, introduce __counted_by_{le,be} defined depending on platform's
Endianness to either __counted_by() when applicable or noop otherwise.
The first user will be virtchnl2.h from idpf just as example with 9 flex
structures having Little Endian counters.

Maybe it would be a good idea to introduce such attributes on compiler
level if possible, but for now let's stop on what we have.

Alexander Lobakin (3):
  compiler_types: add Endianness-dependent __counted_by_{le,be}
  idpf: make virtchnl2.h self-contained
  idpf: sprinkle __counted_by{,_le}() in the virtchnl2 header

 include/linux/compiler_types.h              | 11 ++++++++++
 drivers/net/ethernet/intel/idpf/virtchnl2.h | 24 ++++++++++-----------
 2 files changed, 23 insertions(+), 12 deletions(-)

-- 
2.44.0

[PATCH RFC kspp-next 1/3] compiler_types: add Endianness-dependent __counted_by_{le,be}

[Alexander Lobakin]

Some structures contain flexible arrays at the end and the counter for
them, but the counter has explicit Endianness and thus __counted_by()
can't be used directly.
To increase test coverage for potential problems without breaking
anything, introduce __counted_by_{le,be} defined depending on platform's
Endianness to either __counted_by() when applicable or noop otherwise.
Maybe it would be a good idea to introduce such attributes on compiler
level if possible, but for now let's stop on what we have.

Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 include/linux/compiler_types.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 3e64ec0f7ac8..9506efbf2b8c 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -242,6 +242,17 @@ struct ftrace_likely_data {
  */
 #define noinline_for_stack noinline
 
+/*
+ * Apply __counted_by() when the Endianness matches to increase test coverage.
+ */
+#ifdef __LITTLE_ENDIAN
+#define __counted_by_le(member)	__counted_by(member)
+#define __counted_by_be(member)
+#else
+#define __counted_by_le(member)
+#define __counted_by_be(member)	__counted_by(member)
+#endif
+
 /*
  * Sanitizer helper attributes: Because using __always_inline and
  * __no_sanitize_* conflict, provide helper attributes that will either expand
-- 
2.44.0

[PATCH RFC kspp-next 2/3] idpf: make virtchnl2.h self-contained

[Alexander Lobakin]

To ease maintaining of virtchnl2.h, which already is messy enough,
make it self-contained by adding missing if_ether.h include due to
%ETH_ALEN usage.
At the same time, virtchnl2_lan_desc.h is not anywhere in the file,
so remove this include to speed up preprocessing.

Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 drivers/net/ethernet/intel/idpf/virtchnl2.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/virtchnl2.h b/drivers/net/ethernet/intel/idpf/virtchnl2.h
index 4a3c4454d25a..29419211b3d9 100644
--- a/drivers/net/ethernet/intel/idpf/virtchnl2.h
+++ b/drivers/net/ethernet/intel/idpf/virtchnl2.h
@@ -4,6 +4,8 @@
 #ifndef _VIRTCHNL2_H_
 #define _VIRTCHNL2_H_
 
+#include <linux/if_ether.h>
+
 /* All opcodes associated with virtchnl2 are prefixed with virtchnl2 or
  * VIRTCHNL2. Any future opcodes, offloads/capabilities, structures,
  * and defines must be prefixed with virtchnl2 or VIRTCHNL2 to avoid confusion.
@@ -17,8 +19,6 @@
  * must remain unchanged over time, so we specify explicit values for all enums.
  */
 
-#include "virtchnl2_lan_desc.h"
-
 /* This macro is used to generate compilation errors if a structure
  * is not exactly the correct length.
  */
-- 
2.44.0

[PATCH RFC kspp-next 3/3] idpf: sprinkle __counted_by{,_le}() in the virtchnl2 header

[Alexander Lobakin]

Both virtchnl2.h and its consumer idpf_virtchnl.c are very error-prone.
There are 10 structures with flexible arrays at the end, but 9 of them
has flex member counter in Little Endian.
Make the code a bit more robust by applying __counted_by_le() to those
9. LE platforms is the main target for this driver, so they would
receive additional protection.
While we're here, add __counted_by() to virtchnl2_ptype::proto_id, as
its counter is `u8` regardless of the Endianness.
Compile test on x86_64 (LE) didn't reveal any new issues after applying
the attributes.

Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 drivers/net/ethernet/intel/idpf/virtchnl2.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/virtchnl2.h b/drivers/net/ethernet/intel/idpf/virtchnl2.h
index 29419211b3d9..63deb120359c 100644
--- a/drivers/net/ethernet/intel/idpf/virtchnl2.h
+++ b/drivers/net/ethernet/intel/idpf/virtchnl2.h
@@ -555,7 +555,7 @@ VIRTCHNL2_CHECK_STRUCT_LEN(32, virtchnl2_queue_reg_chunk);
 struct virtchnl2_queue_reg_chunks {
 	__le16 num_chunks;
 	u8 pad[6];
-	struct virtchnl2_queue_reg_chunk chunks[];
+	struct virtchnl2_queue_reg_chunk chunks[] __counted_by_le(num_chunks);
 };
 VIRTCHNL2_CHECK_STRUCT_LEN(8, virtchnl2_queue_reg_chunks);
 
@@ -703,7 +703,7 @@ struct virtchnl2_config_tx_queues {
 	__le32 vport_id;
 	__le16 num_qinfo;
 	u8 pad[10];
-	struct virtchnl2_txq_info qinfo[];
+	struct virtchnl2_txq_info qinfo[] __counted_by_le(num_qinfo);
 };
 VIRTCHNL2_CHECK_STRUCT_LEN(16, virtchnl2_config_tx_queues);
 
@@ -782,7 +782,7 @@ struct virtchnl2_config_rx_queues {
 	__le32 vport_id;
 	__le16 num_qinfo;
 	u8 pad[18];
-	struct virtchnl2_rxq_info qinfo[];
+	struct virtchnl2_rxq_info qinfo[] __counted_by_le(num_qinfo);
 };
 VIRTCHNL2_CHECK_STRUCT_LEN(24, virtchnl2_config_rx_queues);
 
@@ -868,7 +868,7 @@ VIRTCHNL2_CHECK_STRUCT_LEN(32, virtchnl2_vector_chunk);
 struct virtchnl2_vector_chunks {
 	__le16 num_vchunks;
 	u8 pad[14];
-	struct virtchnl2_vector_chunk vchunks[];
+	struct virtchnl2_vector_chunk vchunks[] __counted_by_le(num_vchunks);
 };
 VIRTCHNL2_CHECK_STRUCT_LEN(16, virtchnl2_vector_chunks);
 
@@ -912,7 +912,7 @@ struct virtchnl2_rss_lut {
 	__le16 lut_entries_start;
 	__le16 lut_entries;
 	u8 pad[4];
-	__le32 lut[];
+	__le32 lut[] __counted_by_le(lut_entries);
 };
 VIRTCHNL2_CHECK_STRUCT_LEN(12, virtchnl2_rss_lut);
 
@@ -977,7 +977,7 @@ struct virtchnl2_ptype {
 	u8 ptype_id_8;
 	u8 proto_id_count;
 	__le16 pad;
-	__le16 proto_id[];
+	__le16 proto_id[] __counted_by(proto_id_count);
 } __packed __aligned(2);
 VIRTCHNL2_CHECK_STRUCT_LEN(6, virtchnl2_ptype);
 
@@ -1104,7 +1104,7 @@ struct virtchnl2_rss_key {
 	__le32 vport_id;
 	__le16 key_len;
 	u8 pad;
-	u8 key_flex[];
+	u8 key_flex[] __counted_by_le(key_len);
 } __packed;
 VIRTCHNL2_CHECK_STRUCT_LEN(7, virtchnl2_rss_key);
 
@@ -1131,7 +1131,7 @@ VIRTCHNL2_CHECK_STRUCT_LEN(16, virtchnl2_queue_chunk);
 struct virtchnl2_queue_chunks {
 	__le16 num_chunks;
 	u8 pad[6];
-	struct virtchnl2_queue_chunk chunks[];
+	struct virtchnl2_queue_chunk chunks[] __counted_by_le(num_chunks);
 };
 VIRTCHNL2_CHECK_STRUCT_LEN(8, virtchnl2_queue_chunks);
 
@@ -1195,7 +1195,7 @@ struct virtchnl2_queue_vector_maps {
 	__le32 vport_id;
 	__le16 num_qv_maps;
 	u8 pad[10];
-	struct virtchnl2_queue_vector qv_maps[];
+	struct virtchnl2_queue_vector qv_maps[] __counted_by_le(num_qv_maps);
 };
 VIRTCHNL2_CHECK_STRUCT_LEN(16, virtchnl2_queue_vector_maps);
 
@@ -1247,7 +1247,7 @@ struct virtchnl2_mac_addr_list {
 	__le32 vport_id;
 	__le16 num_mac_addr;
 	u8 pad[2];
-	struct virtchnl2_mac_addr mac_addr_list[];
+	struct virtchnl2_mac_addr mac_addr_list[] __counted_by_le(num_mac_addr);
 };
 VIRTCHNL2_CHECK_STRUCT_LEN(8, virtchnl2_mac_addr_list);
 
-- 
2.44.0

Re: [PATCH RFC kspp-next 3/3] idpf: sprinkle __counted_by{,_le}() in the virtchnl2 header

[Simon Horman]

On Mon, Mar 18, 2024 at 02:03:54PM +0100, Alexander Lobakin wrote:
> Both virtchnl2.h and its consumer idpf_virtchnl.c are very error-prone.
> There are 10 structures with flexible arrays at the end, but 9 of them
> has flex member counter in Little Endian.
> Make the code a bit more robust by applying __counted_by_le() to those
> 9. LE platforms is the main target for this driver, so they would
> receive additional protection.
> While we're here, add __counted_by() to virtchnl2_ptype::proto_id, as
> its counter is `u8` regardless of the Endianness.
> Compile test on x86_64 (LE) didn't reveal any new issues after applying
> the attributes.
> 
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>

Hi Alexander,

with this patch applied ./scripts/kernel-doc -none reports the following.
I think that this means that the kernel-doc needs to be taught
about __counted_by_le (and __counted_by_be).

.../virtchnl2.h:559: warning: Excess struct member 'chunks' description in 'virtchnl2_queue_reg_chunks'
.../virtchnl2.h:707: warning: Excess struct member 'qinfo' description in 'virtchnl2_config_tx_queues'
.../virtchnl2.h:786: warning: Excess struct member 'qinfo' description in 'virtchnl2_config_rx_queues'
.../virtchnl2.h:872: warning: Excess struct member 'vchunks' description in 'virtchnl2_vector_chunks'
.../virtchnl2.h:916: warning: Excess struct member 'lut' description in 'virtchnl2_rss_lut'
.../virtchnl2.h:1108: warning: Excess struct member 'key_flex' description in 'virtchnl2_rss_key'
.../virtchnl2.h:1199: warning: Excess struct member 'qv_maps' description in 'virtchnl2_queue_vector_maps'
.../virtchnl2.h:1251: warning: Excess struct member 'mac_addr_list' description in 'virtchnl2_mac_addr_list'

...

Re: [PATCH RFC kspp-next 3/3] idpf: sprinkle __counted_by{,_le}() in the virtchnl2 header

[Kees Cook]

On Tue, Mar 19, 2024 at 06:57:18PM +0000, Simon Horman wrote:
> On Mon, Mar 18, 2024 at 02:03:54PM +0100, Alexander Lobakin wrote:
> > Both virtchnl2.h and its consumer idpf_virtchnl.c are very error-prone.
> > There are 10 structures with flexible arrays at the end, but 9 of them
> > has flex member counter in Little Endian.
> > Make the code a bit more robust by applying __counted_by_le() to those
> > 9. LE platforms is the main target for this driver, so they would
> > receive additional protection.
> > While we're here, add __counted_by() to virtchnl2_ptype::proto_id, as
> > its counter is `u8` regardless of the Endianness.
> > Compile test on x86_64 (LE) didn't reveal any new issues after applying
> > the attributes.
> > 
> > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> 
> Hi Alexander,
> 
> with this patch applied ./scripts/kernel-doc -none reports the following.
> I think that this means that the kernel-doc needs to be taught
> about __counted_by_le (and __counted_by_be).

Oh, yes, I should have remembered that need. Sorry! It should be
addressed by adding them where __counted_by is already listed in
Documentation/conf.py.

-Kees

> 
> .../virtchnl2.h:559: warning: Excess struct member 'chunks' description in 'virtchnl2_queue_reg_chunks'
> .../virtchnl2.h:707: warning: Excess struct member 'qinfo' description in 'virtchnl2_config_tx_queues'
> .../virtchnl2.h:786: warning: Excess struct member 'qinfo' description in 'virtchnl2_config_rx_queues'
> .../virtchnl2.h:872: warning: Excess struct member 'vchunks' description in 'virtchnl2_vector_chunks'
> .../virtchnl2.h:916: warning: Excess struct member 'lut' description in 'virtchnl2_rss_lut'
> .../virtchnl2.h:1108: warning: Excess struct member 'key_flex' description in 'virtchnl2_rss_key'
> .../virtchnl2.h:1199: warning: Excess struct member 'qv_maps' description in 'virtchnl2_queue_vector_maps'
> .../virtchnl2.h:1251: warning: Excess struct member 'mac_addr_list' description in 'virtchnl2_mac_addr_list'
> 
> ...

-- 
Kees Cook

Re: [PATCH RFC kspp-next 3/3] idpf: sprinkle __counted_by{,_le}() in the virtchnl2 header

[Alexander Lobakin]

From: Kees Cook <keescook@chromium.org>
Date: Tue, 19 Mar 2024 14:42:56 -0700

> On Tue, Mar 19, 2024 at 06:57:18PM +0000, Simon Horman wrote:
>> On Mon, Mar 18, 2024 at 02:03:54PM +0100, Alexander Lobakin wrote:
>>> Both virtchnl2.h and its consumer idpf_virtchnl.c are very error-prone.
>>> There are 10 structures with flexible arrays at the end, but 9 of them
>>> has flex member counter in Little Endian.
>>> Make the code a bit more robust by applying __counted_by_le() to those
>>> 9. LE platforms is the main target for this driver, so they would
>>> receive additional protection.
>>> While we're here, add __counted_by() to virtchnl2_ptype::proto_id, as
>>> its counter is `u8` regardless of the Endianness.
>>> Compile test on x86_64 (LE) didn't reveal any new issues after applying
>>> the attributes.
>>>
>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>>
>> Hi Alexander,
>>
>> with this patch applied ./scripts/kernel-doc -none reports the following.
>> I think that this means that the kernel-doc needs to be taught
>> about __counted_by_le (and __counted_by_be).
> 
> Oh, yes, I should have remembered that need. Sorry! It should be
> addressed by adding them where __counted_by is already listed in
> Documentation/conf.py.

Oh, thanks to both of you! I'll do that before sending v1.

> 
> -Kees
> 
>>
>> .../virtchnl2.h:559: warning: Excess struct member 'chunks' description in 'virtchnl2_queue_reg_chunks'
>> .../virtchnl2.h:707: warning: Excess struct member 'qinfo' description in 'virtchnl2_config_tx_queues'
>> .../virtchnl2.h:786: warning: Excess struct member 'qinfo' description in 'virtchnl2_config_rx_queues'
>> .../virtchnl2.h:872: warning: Excess struct member 'vchunks' description in 'virtchnl2_vector_chunks'
>> .../virtchnl2.h:916: warning: Excess struct member 'lut' description in 'virtchnl2_rss_lut'
>> .../virtchnl2.h:1108: warning: Excess struct member 'key_flex' description in 'virtchnl2_rss_key'
>> .../virtchnl2.h:1199: warning: Excess struct member 'qv_maps' description in 'virtchnl2_queue_vector_maps'
>> .../virtchnl2.h:1251: warning: Excess struct member 'mac_addr_list' description in 'virtchnl2_mac_addr_list'
>>
>> ...
> 

Thanks,
Olek

Re: [PATCH RFC kspp-next 0/3] compiler_types: add Endianness-dependent __counted_by_{le,be}

[Kees Cook]

On Mon, Mar 18, 2024 at 02:03:51PM +0100, Alexander Lobakin wrote:
> Some structures contain flexible arrays at the end and the counter for
> them, but the counter has explicit Endianness and thus __counted_by()
> can't be used directly.
> 
> To increase test coverage for potential problems without breaking
> anything, introduce __counted_by_{le,be} defined depending on platform's
> Endianness to either __counted_by() when applicable or noop otherwise.
> The first user will be virtchnl2.h from idpf just as example with 9 flex
> structures having Little Endian counters.

Yeah, okay, that makes good sense. It'll give us as much coverage as we
can get until the compilers gain "expression" support for the
'counted_by' attribute.

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH RFC kspp-next 0/3] compiler_types: add Endianness-dependent __counted_by_{le,be}

[Kees Cook]

On Mon, Mar 18, 2024 at 02:03:51PM +0100, Alexander Lobakin wrote:
>  include/linux/compiler_types.h              | 11 ++++++++++
>  drivers/net/ethernet/intel/idpf/virtchnl2.h | 24 ++++++++++-----------
>  2 files changed, 23 insertions(+), 12 deletions(-)

Oh, I see the Subject says "kspp-next" -- normally I'd expect things
touch net to go through netdev. I'm fine with this going through either
tree. Perhaps better through netdev since that subsystem has the most
users and may gain more using the new macros?

-Kees

-- 
Kees Cook

Re: [PATCH RFC kspp-next 0/3] compiler_types: add Endianness-dependent __counted_by_{le,be}

[Alexander Lobakin]

From: Kees Cook <keescook@chromium.org>
Date: Mon, 18 Mar 2024 10:49:25 -0700

> On Mon, Mar 18, 2024 at 02:03:51PM +0100, Alexander Lobakin wrote:
>>  include/linux/compiler_types.h              | 11 ++++++++++
>>  drivers/net/ethernet/intel/idpf/virtchnl2.h | 24 ++++++++++-----------
>>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> Oh, I see the Subject says "kspp-next" -- normally I'd expect things
> touch net to go through netdev. I'm fine with this going through either
> tree. Perhaps better through netdev since that subsystem has the most
> users and may gain more using the new macros?

Yeah sure. I send it with "kspp-next", so that it would be clear it's a
security feature :>

Thanks for the ack. Re expressions -- Przemek suggested it would be nice
to have something like

	__le32 counter;
	struct a flex[] __counted_by(le32_to_cpu(counter));

but we don't know whether something like this is possible to implement
in the compiler.

> 
> -Kees

Thanks,
Olek