[PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Gustavo A. R. Silva]

There is a regular need in the kernel to provide a way to declare having a
dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].

Refactor the code according to the use of a flexible-array member in struct
virtchnl_vsi_queue_config_info instead of one-element array, and use the
flex_array_size() helper.

[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays

Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 include/linux/avf/virtchnl.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index b554913804bd..ed9c4998f8ac 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
 	u16 vsi_id;
 	u16 num_queue_pairs;
 	u32 pad;
-	struct virtchnl_queue_pair_info qpair[1];
+	struct virtchnl_queue_pair_info qpair[];
 };
 
-VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
 
 /* VIRTCHNL_OP_REQUEST_QUEUES
  * VF sends this message to request the PF to allocate additional queues to
@@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
 		if (msglen >= valid_len) {
 			struct virtchnl_vsi_queue_config_info *vqc =
 			    (struct virtchnl_vsi_queue_config_info *)msg;
-			valid_len += (vqc->num_queue_pairs *
-				      sizeof(struct
-					     virtchnl_queue_pair_info));
+			valid_len += flex_array_size(vqc, qpair,
+						     vqc->num_queue_pairs);
 			if (vqc->num_queue_pairs == 0)
 				err_msg_format = true;
 		}
-- 
2.27.0

[Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Gustavo A. R. Silva]

There is a regular need in the kernel to provide a way to declare having a
dynamically sized set of trailing elements in a structure. Kernel code
should always use ?flexible array members?[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].

Refactor the code according to the use of a flexible-array member in struct
virtchnl_vsi_queue_config_info instead of one-element array, and use the
flex_array_size() helper.

[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays

Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 include/linux/avf/virtchnl.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index b554913804bd..ed9c4998f8ac 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
 	u16 vsi_id;
 	u16 num_queue_pairs;
 	u32 pad;
-	struct virtchnl_queue_pair_info qpair[1];
+	struct virtchnl_queue_pair_info qpair[];
 };
 
-VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
 
 /* VIRTCHNL_OP_REQUEST_QUEUES
  * VF sends this message to request the PF to allocate additional queues to
@@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
 		if (msglen >= valid_len) {
 			struct virtchnl_vsi_queue_config_info *vqc =
 			    (struct virtchnl_vsi_queue_config_info *)msg;
-			valid_len += (vqc->num_queue_pairs *
-				      sizeof(struct
-					     virtchnl_queue_pair_info));
+			valid_len += flex_array_size(vqc, qpair,
+						     vqc->num_queue_pairs);
 			if (vqc->num_queue_pairs == 0)
 				err_msg_format = true;
 		}
-- 
2.27.0

Re: [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Nguyen, Anthony L]

On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
> There is a regular need in the kernel to provide a way to declare
> having a
> dynamically sized set of trailing elements in a structure. Kernel
> code
> should always use “flexible array members”[1] for these cases. The
> older
> style of one-element or zero-length arrays should no longer be
> used[2].
> 
> Refactor the code according to the use of a flexible-array member in
> struct
> virtchnl_vsi_queue_config_info instead of one-element array, and use
> the
> flex_array_size() helper.
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] 
> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  include/linux/avf/virtchnl.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/avf/virtchnl.h
> b/include/linux/avf/virtchnl.h
> index b554913804bd..ed9c4998f8ac 100644
> --- a/include/linux/avf/virtchnl.h
> +++ b/include/linux/avf/virtchnl.h
> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
>  	u16 vsi_id;
>  	u16 num_queue_pairs;
>  	u32 pad;
> -	struct virtchnl_queue_pair_info qpair[1];
> +	struct virtchnl_queue_pair_info qpair[];
>  };
>  
> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
>  
>  /* VIRTCHNL_OP_REQUEST_QUEUES
>   * VF sends this message to request the PF to allocate additional
> queues to
> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
> virtchnl_version_info *ver, u32 v_opcode,
>  		if (msglen >= valid_len) {
>  			struct virtchnl_vsi_queue_config_info *vqc =
>  			    (struct virtchnl_vsi_queue_config_info
> *)msg;
> -			valid_len += (vqc->num_queue_pairs *
> -				      sizeof(struct
> -					     virtchnl_queue_pair_info))
> ;
> +			valid_len += flex_array_size(vqc, qpair,
> +						     vqc-
> >num_queue_pairs);

The virtchnl file acts as a binary interface between physical and
virtual functions. There's no guaruntee that the PF and VF will both
have the newest version. Thus changing this will break compatibility.
Specifically, the way the size was validated for this op code
incorrectly expects an extra queue pair structure. Some other
structures have similar length calculation flaws. We agree that fixing
this is important, but the fix needs to account that old drivers will
send an off by 1 size. 

To properly handle compatibility we need to introduce a feature flag to
indicate the new behavior. If the feature flag is not set, we acccept
messages with the old format (with the extra size). If both the PF and
VF support the feature flag, we'll use the correct size calculations.
We're looking to add this and would like to do all the virtchnl
structure fixes in one series.

>  			if (vqc->num_queue_pairs == 0)
>  				err_msg_format = true;
>  		}

[Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Nguyen, Anthony L]

On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
> There is a regular need in the kernel to provide a way to declare
> having a
> dynamically sized set of trailing elements in a structure. Kernel
> code
> should always use ?flexible array members?[1] for these cases. The
> older
> style of one-element or zero-length arrays should no longer be
> used[2].
> 
> Refactor the code according to the use of a flexible-array member in
> struct
> virtchnl_vsi_queue_config_info instead of one-element array, and use
> the
> flex_array_size() helper.
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] 
> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  include/linux/avf/virtchnl.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/avf/virtchnl.h
> b/include/linux/avf/virtchnl.h
> index b554913804bd..ed9c4998f8ac 100644
> --- a/include/linux/avf/virtchnl.h
> +++ b/include/linux/avf/virtchnl.h
> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
>  	u16 vsi_id;
>  	u16 num_queue_pairs;
>  	u32 pad;
> -	struct virtchnl_queue_pair_info qpair[1];
> +	struct virtchnl_queue_pair_info qpair[];
>  };
>  
> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
>  
>  /* VIRTCHNL_OP_REQUEST_QUEUES
>   * VF sends this message to request the PF to allocate additional
> queues to
> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
> virtchnl_version_info *ver, u32 v_opcode,
>  		if (msglen >= valid_len) {
>  			struct virtchnl_vsi_queue_config_info *vqc =
>  			    (struct virtchnl_vsi_queue_config_info
> *)msg;
> -			valid_len += (vqc->num_queue_pairs *
> -				      sizeof(struct
> -					     virtchnl_queue_pair_info))
> ;
> +			valid_len += flex_array_size(vqc, qpair,
> +						     vqc-
> >num_queue_pairs);

The virtchnl file acts as a binary interface between physical and
virtual functions. There's no guaruntee that the PF and VF will both
have the newest version. Thus changing this will break compatibility.
Specifically, the way the size was validated for this op code
incorrectly expects an extra queue pair structure. Some other
structures have similar length calculation flaws. We agree that fixing
this is important, but the fix needs to account that old drivers will
send an off by 1 size. 

To properly handle compatibility we need to introduce a feature flag to
indicate the new behavior. If the feature flag is not set, we acccept
messages with the old format (with the extra size). If both the PF and
VF support the feature flag, we'll use the correct size calculations.
We're looking to add this and would like to do all the virtchnl
structure fixes in one series.

>  			if (vqc->num_queue_pairs == 0)
>  				err_msg_format = true;
>  		}

[Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Gustavo A. R. Silva]

On 5/28/21 16:56, Nguyen, Anthony L wrote:
> On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
>> There is a regular need in the kernel to provide a way to declare
>> having a
>> dynamically sized set of trailing elements in a structure. Kernel
>> code
>> should always use ?flexible array members?[1] for these cases. The
>> older
>> style of one-element or zero-length arrays should no longer be
>> used[2].
>>
>> Refactor the code according to the use of a flexible-array member in
>> struct
>> virtchnl_vsi_queue_config_info instead of one-element array, and use
>> the
>> flex_array_size() helper.
>>
>> [1] https://en.wikipedia.org/wiki/Flexible_array_member
>> [2] 
>> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
>>
>> Link: https://github.com/KSPP/linux/issues/79
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>  include/linux/avf/virtchnl.h | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/avf/virtchnl.h
>> b/include/linux/avf/virtchnl.h
>> index b554913804bd..ed9c4998f8ac 100644
>> --- a/include/linux/avf/virtchnl.h
>> +++ b/include/linux/avf/virtchnl.h
>> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
>>  	u16 vsi_id;
>>  	u16 num_queue_pairs;
>>  	u32 pad;
>> -	struct virtchnl_queue_pair_info qpair[1];
>> +	struct virtchnl_queue_pair_info qpair[];
>>  };
>>  
>> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
>> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
>>  
>>  /* VIRTCHNL_OP_REQUEST_QUEUES
>>   * VF sends this message to request the PF to allocate additional
>> queues to
>> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
>> virtchnl_version_info *ver, u32 v_opcode,
>>  		if (msglen >= valid_len) {
>>  			struct virtchnl_vsi_queue_config_info *vqc =
>>  			    (struct virtchnl_vsi_queue_config_info
>> *)msg;
>> -			valid_len += (vqc->num_queue_pairs *
>> -				      sizeof(struct
>> -					     virtchnl_queue_pair_info))
>> ;
>> +			valid_len += flex_array_size(vqc, qpair,
>> +						     vqc-
>>> num_queue_pairs);
> 
> The virtchnl file acts as a binary interface between physical and
> virtual functions. There's no guaruntee that the PF and VF will both
> have the newest version. Thus changing this will break compatibility.
> Specifically, the way the size was validated for this op code
> incorrectly expects an extra queue pair structure. Some other
> structures have similar length calculation flaws. We agree that fixing
> this is important, but the fix needs to account that old drivers will
> send an off by 1 size. 
> 
> To properly handle compatibility we need to introduce a feature flag to
> indicate the new behavior. If the feature flag is not set, we acccept
> messages with the old format (with the extra size). If both the PF and
> VF support the feature flag, we'll use the correct size calculations.
> We're looking to add this and would like to do all the virtchnl
> structure fixes in one series.
> 

Oh OK, I see. In this case, I think something like this might work just
fine:

https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d

What do you think?

Thanks
--
Gustavo

Re: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Gustavo A. R. Silva]

On 5/28/21 16:56, Nguyen, Anthony L wrote:
> On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
>> There is a regular need in the kernel to provide a way to declare
>> having a
>> dynamically sized set of trailing elements in a structure. Kernel
>> code
>> should always use “flexible array members”[1] for these cases. The
>> older
>> style of one-element or zero-length arrays should no longer be
>> used[2].
>>
>> Refactor the code according to the use of a flexible-array member in
>> struct
>> virtchnl_vsi_queue_config_info instead of one-element array, and use
>> the
>> flex_array_size() helper.
>>
>> [1] https://en.wikipedia.org/wiki/Flexible_array_member
>> [2] 
>> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
>>
>> Link: https://github.com/KSPP/linux/issues/79
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>  include/linux/avf/virtchnl.h | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/avf/virtchnl.h
>> b/include/linux/avf/virtchnl.h
>> index b554913804bd..ed9c4998f8ac 100644
>> --- a/include/linux/avf/virtchnl.h
>> +++ b/include/linux/avf/virtchnl.h
>> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
>>  	u16 vsi_id;
>>  	u16 num_queue_pairs;
>>  	u32 pad;
>> -	struct virtchnl_queue_pair_info qpair[1];
>> +	struct virtchnl_queue_pair_info qpair[];
>>  };
>>  
>> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
>> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
>>  
>>  /* VIRTCHNL_OP_REQUEST_QUEUES
>>   * VF sends this message to request the PF to allocate additional
>> queues to
>> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
>> virtchnl_version_info *ver, u32 v_opcode,
>>  		if (msglen >= valid_len) {
>>  			struct virtchnl_vsi_queue_config_info *vqc =
>>  			    (struct virtchnl_vsi_queue_config_info
>> *)msg;
>> -			valid_len += (vqc->num_queue_pairs *
>> -				      sizeof(struct
>> -					     virtchnl_queue_pair_info))
>> ;
>> +			valid_len += flex_array_size(vqc, qpair,
>> +						     vqc-
>>> num_queue_pairs);
> 
> The virtchnl file acts as a binary interface between physical and
> virtual functions. There's no guaruntee that the PF and VF will both
> have the newest version. Thus changing this will break compatibility.
> Specifically, the way the size was validated for this op code
> incorrectly expects an extra queue pair structure. Some other
> structures have similar length calculation flaws. We agree that fixing
> this is important, but the fix needs to account that old drivers will
> send an off by 1 size. 
> 
> To properly handle compatibility we need to introduce a feature flag to
> indicate the new behavior. If the feature flag is not set, we acccept
> messages with the old format (with the extra size). If both the PF and
> VF support the feature flag, we'll use the correct size calculations.
> We're looking to add this and would like to do all the virtchnl
> structure fixes in one series.
> 

Oh OK, I see. In this case, I think something like this might work just
fine:

https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d

What do you think?

Thanks
--
Gustavo

[Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Gustavo A. R. Silva]

On 5/28/21 16:56, Nguyen, Anthony L wrote:
> On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
>> There is a regular need in the kernel to provide a way to declare
>> having a
>> dynamically sized set of trailing elements in a structure. Kernel
>> code
>> should always use ?flexible array members?[1] for these cases. The
>> older
>> style of one-element or zero-length arrays should no longer be
>> used[2].
>>
>> Refactor the code according to the use of a flexible-array member in
>> struct
>> virtchnl_vsi_queue_config_info instead of one-element array, and use
>> the
>> flex_array_size() helper.
>>
>> [1] https://en.wikipedia.org/wiki/Flexible_array_member
>> [2] 
>> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
>>
>> Link: https://github.com/KSPP/linux/issues/79
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>  include/linux/avf/virtchnl.h | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/avf/virtchnl.h
>> b/include/linux/avf/virtchnl.h
>> index b554913804bd..ed9c4998f8ac 100644
>> --- a/include/linux/avf/virtchnl.h
>> +++ b/include/linux/avf/virtchnl.h
>> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
>>  	u16 vsi_id;
>>  	u16 num_queue_pairs;
>>  	u32 pad;
>> -	struct virtchnl_queue_pair_info qpair[1];
>> +	struct virtchnl_queue_pair_info qpair[];
>>  };
>>  
>> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
>> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
>>  
>>  /* VIRTCHNL_OP_REQUEST_QUEUES
>>   * VF sends this message to request the PF to allocate additional
>> queues to
>> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
>> virtchnl_version_info *ver, u32 v_opcode,
>>  		if (msglen >= valid_len) {
>>  			struct virtchnl_vsi_queue_config_info *vqc =
>>  			    (struct virtchnl_vsi_queue_config_info
>> *)msg;
>> -			valid_len += (vqc->num_queue_pairs *
>> -				      sizeof(struct
>> -					     virtchnl_queue_pair_info))
>> ;
>> +			valid_len += flex_array_size(vqc, qpair,
>> +						     vqc-
>>> num_queue_pairs);
> 
> The virtchnl file acts as a binary interface between physical and
> virtual functions. There's no guaruntee that the PF and VF will both
> have the newest version. Thus changing this will break compatibility.
> Specifically, the way the size was validated for this op code
> incorrectly expects an extra queue pair structure. Some other
> structures have similar length calculation flaws. We agree that fixing
> this is important, but the fix needs to account that old drivers will
> send an off by 1 size. 
> 
> To properly handle compatibility we need to introduce a feature flag to
> indicate the new behavior. If the feature flag is not set, we acccept
> messages with the old format (with the extra size). If both the PF and
> VF support the feature flag, we'll use the correct size calculations.
> We're looking to add this and would like to do all the virtchnl
> structure fixes in one series.
> 

Oh OK, I see. In this case, I think something like this might work just
fine:

https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d

What do you think?

Thanks
--
Gustavo

RE: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Keller, Jacob E]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Gustavo A. R. Silva
> Sent: Friday, May 28, 2021 4:05 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; gustavoars@kernel.org
> Cc: intel-wired-lan@lists.osuosl.org; linux-kernel@vger.kernel.org; linux-
> hardening@vger.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array
> in struct virtchnl_vsi_queue_config_info
> 
> 
> 
> On 5/28/21 16:56, Nguyen, Anthony L wrote:
> > On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
> >> There is a regular need in the kernel to provide a way to declare
> >> having a
> >> dynamically sized set of trailing elements in a structure. Kernel
> >> code
> >> should always use “flexible array members”[1] for these cases. The
> >> older
> >> style of one-element or zero-length arrays should no longer be
> >> used[2].
> >>
> >> Refactor the code according to the use of a flexible-array member in
> >> struct
> >> virtchnl_vsi_queue_config_info instead of one-element array, and use
> >> the
> >> flex_array_size() helper.
> >>
> >> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> >> [2]
> >> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-
> length-and-one-element-arrays
> >>
> >> Link: https://github.com/KSPP/linux/issues/79
> >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> >> ---
> >>  include/linux/avf/virtchnl.h | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/linux/avf/virtchnl.h
> >> b/include/linux/avf/virtchnl.h
> >> index b554913804bd..ed9c4998f8ac 100644
> >> --- a/include/linux/avf/virtchnl.h
> >> +++ b/include/linux/avf/virtchnl.h
> >> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
> >>  	u16 vsi_id;
> >>  	u16 num_queue_pairs;
> >>  	u32 pad;
> >> -	struct virtchnl_queue_pair_info qpair[1];
> >> +	struct virtchnl_queue_pair_info qpair[];
> >>  };
> >>
> >> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
> >> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
> >>
> >>  /* VIRTCHNL_OP_REQUEST_QUEUES
> >>   * VF sends this message to request the PF to allocate additional
> >> queues to
> >> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
> >> virtchnl_version_info *ver, u32 v_opcode,
> >>  		if (msglen >= valid_len) {
> >>  			struct virtchnl_vsi_queue_config_info *vqc =
> >>  			    (struct virtchnl_vsi_queue_config_info
> >> *)msg;
> >> -			valid_len += (vqc->num_queue_pairs *
> >> -				      sizeof(struct
> >> -					     virtchnl_queue_pair_info))
> >> ;
> >> +			valid_len += flex_array_size(vqc, qpair,
> >> +						     vqc-
> >>> num_queue_pairs);
> >
> > The virtchnl file acts as a binary interface between physical and
> > virtual functions. There's no guaruntee that the PF and VF will both
> > have the newest version. Thus changing this will break compatibility.
> > Specifically, the way the size was validated for this op code
> > incorrectly expects an extra queue pair structure. Some other
> > structures have similar length calculation flaws. We agree that fixing
> > this is important, but the fix needs to account that old drivers will
> > send an off by 1 size.
> >
> > To properly handle compatibility we need to introduce a feature flag to
> > indicate the new behavior. If the feature flag is not set, we acccept
> > messages with the old format (with the extra size). If both the PF and
> > VF support the feature flag, we'll use the correct size calculations.
> > We're looking to add this and would like to do all the virtchnl
> > structure fixes in one series.
> >
> 
> Oh OK, I see. In this case, I think something like this might work just
> fine:
> 
> https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d
> 
> What do you think?
> 

About half our virtchnl structures correctly validate the length (i.e. enforcing that the number of members including the implicit one are correct). There are maybe 3-4 which don't do that and accidentally allow sizes that are off by 1 member.

We believe the correct fix is to fix the structure definitions to use [] and then introduce a VALIDATE_MSG_V2 feature flag which is negotiated by the VF indicating whether it supports this behavior, and the PF replying to VF if it supports.

In the case where the VF doesn't support this, the PF will notice this and modify its length calculations for the handful of currently broken checks to include one extra member. In the case where the VF supports this but the PF does not, the VF must allocate extra memory and ensure it passes the larger message length. In the case where both PF and VF support the new "feature" we'll correctly switch to using 0 length flexible arrays.

It's actually even slightly more convoluted because another 3-4 ops only mis-validate the size when the length of the flexible array is 0. In that case, they require the full size of the structure, but in the case where it's 1 or more, they require the size to match as you would expect with a 0-sized array.

I'm not sure the union approach is suitable for that? We believe the use of a new capability bit is the best mechanism: we can fix the code to use flexible array definitions everywhere and simply ensure that when communicating with old PF or VF, we add additional padding as necessary to the message.

Thanks,
Jake
 

[Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Keller, Jacob E]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Gustavo A. R. Silva
> Sent: Friday, May 28, 2021 4:05 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; gustavoars at kernel.org
> Cc: intel-wired-lan at lists.osuosl.org; linux-kernel at vger.kernel.org; linux-
> hardening at vger.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array
> in struct virtchnl_vsi_queue_config_info
> 
> 
> 
> On 5/28/21 16:56, Nguyen, Anthony L wrote:
> > On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
> >> There is a regular need in the kernel to provide a way to declare
> >> having a
> >> dynamically sized set of trailing elements in a structure. Kernel
> >> code
> >> should always use ?flexible array members?[1] for these cases. The
> >> older
> >> style of one-element or zero-length arrays should no longer be
> >> used[2].
> >>
> >> Refactor the code according to the use of a flexible-array member in
> >> struct
> >> virtchnl_vsi_queue_config_info instead of one-element array, and use
> >> the
> >> flex_array_size() helper.
> >>
> >> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> >> [2]
> >> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-
> length-and-one-element-arrays
> >>
> >> Link: https://github.com/KSPP/linux/issues/79
> >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> >> ---
> >>  include/linux/avf/virtchnl.h | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/linux/avf/virtchnl.h
> >> b/include/linux/avf/virtchnl.h
> >> index b554913804bd..ed9c4998f8ac 100644
> >> --- a/include/linux/avf/virtchnl.h
> >> +++ b/include/linux/avf/virtchnl.h
> >> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
> >>  	u16 vsi_id;
> >>  	u16 num_queue_pairs;
> >>  	u32 pad;
> >> -	struct virtchnl_queue_pair_info qpair[1];
> >> +	struct virtchnl_queue_pair_info qpair[];
> >>  };
> >>
> >> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
> >> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
> >>
> >>  /* VIRTCHNL_OP_REQUEST_QUEUES
> >>   * VF sends this message to request the PF to allocate additional
> >> queues to
> >> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
> >> virtchnl_version_info *ver, u32 v_opcode,
> >>  		if (msglen >= valid_len) {
> >>  			struct virtchnl_vsi_queue_config_info *vqc =
> >>  			    (struct virtchnl_vsi_queue_config_info
> >> *)msg;
> >> -			valid_len += (vqc->num_queue_pairs *
> >> -				      sizeof(struct
> >> -					     virtchnl_queue_pair_info))
> >> ;
> >> +			valid_len += flex_array_size(vqc, qpair,
> >> +						     vqc-
> >>> num_queue_pairs);
> >
> > The virtchnl file acts as a binary interface between physical and
> > virtual functions. There's no guaruntee that the PF and VF will both
> > have the newest version. Thus changing this will break compatibility.
> > Specifically, the way the size was validated for this op code
> > incorrectly expects an extra queue pair structure. Some other
> > structures have similar length calculation flaws. We agree that fixing
> > this is important, but the fix needs to account that old drivers will
> > send an off by 1 size.
> >
> > To properly handle compatibility we need to introduce a feature flag to
> > indicate the new behavior. If the feature flag is not set, we acccept
> > messages with the old format (with the extra size). If both the PF and
> > VF support the feature flag, we'll use the correct size calculations.
> > We're looking to add this and would like to do all the virtchnl
> > structure fixes in one series.
> >
> 
> Oh OK, I see. In this case, I think something like this might work just
> fine:
> 
> https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d
> 
> What do you think?
> 

About half our virtchnl structures correctly validate the length (i.e. enforcing that the number of members including the implicit one are correct). There are maybe 3-4 which don't do that and accidentally allow sizes that are off by 1 member.

We believe the correct fix is to fix the structure definitions to use [] and then introduce a VALIDATE_MSG_V2 feature flag which is negotiated by the VF indicating whether it supports this behavior, and the PF replying to VF if it supports.

In the case where the VF doesn't support this, the PF will notice this and modify its length calculations for the handful of currently broken checks to include one extra member. In the case where the VF supports this but the PF does not, the VF must allocate extra memory and ensure it passes the larger message length. In the case where both PF and VF support the new "feature" we'll correctly switch to using 0 length flexible arrays.

It's actually even slightly more convoluted because another 3-4 ops only mis-validate the size when the length of the flexible array is 0. In that case, they require the full size of the structure, but in the case where it's 1 or more, they require the size to match as you would expect with a 0-sized array.

I'm not sure the union approach is suitable for that? We believe the use of a new capability bit is the best mechanism: we can fix the code to use flexible array definitions everywhere and simply ensure that when communicating with old PF or VF, we add additional padding as necessary to the message.

Thanks,
Jake
 

Re: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Kees Cook]

On Sat, May 29, 2021 at 12:19:48AM +0000, Keller, Jacob E wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > Gustavo A. R. Silva
> > Sent: Friday, May 28, 2021 4:05 PM
> > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>; gustavoars@kernel.org
> > Cc: intel-wired-lan@lists.osuosl.org; linux-kernel@vger.kernel.org; linux-
> > hardening@vger.kernel.org
> > Subject: Re: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array
> > in struct virtchnl_vsi_queue_config_info
> > 
> > 
> > 
> > On 5/28/21 16:56, Nguyen, Anthony L wrote:
> > > On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
> > >> There is a regular need in the kernel to provide a way to declare
> > >> having a
> > >> dynamically sized set of trailing elements in a structure. Kernel
> > >> code
> > >> should always use “flexible array members”[1] for these cases. The
> > >> older
> > >> style of one-element or zero-length arrays should no longer be
> > >> used[2].
> > >>
> > >> Refactor the code according to the use of a flexible-array member in
> > >> struct
> > >> virtchnl_vsi_queue_config_info instead of one-element array, and use
> > >> the
> > >> flex_array_size() helper.
> > >>
> > >> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > >> [2]
> > >> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-
> > length-and-one-element-arrays
> > >>
> > >> Link: https://github.com/KSPP/linux/issues/79
> > >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > >> ---
> > >>  include/linux/avf/virtchnl.h | 9 ++++-----
> > >>  1 file changed, 4 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/include/linux/avf/virtchnl.h
> > >> b/include/linux/avf/virtchnl.h
> > >> index b554913804bd..ed9c4998f8ac 100644
> > >> --- a/include/linux/avf/virtchnl.h
> > >> +++ b/include/linux/avf/virtchnl.h
> > >> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
> > >>  	u16 vsi_id;
> > >>  	u16 num_queue_pairs;
> > >>  	u32 pad;
> > >> -	struct virtchnl_queue_pair_info qpair[1];
> > >> +	struct virtchnl_queue_pair_info qpair[];
> > >>  };
> > >>
> > >> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
> > >> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
> > >>
> > >>  /* VIRTCHNL_OP_REQUEST_QUEUES
> > >>   * VF sends this message to request the PF to allocate additional
> > >> queues to
> > >> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
> > >> virtchnl_version_info *ver, u32 v_opcode,
> > >>  		if (msglen >= valid_len) {
> > >>  			struct virtchnl_vsi_queue_config_info *vqc =
> > >>  			    (struct virtchnl_vsi_queue_config_info
> > >> *)msg;
> > >> -			valid_len += (vqc->num_queue_pairs *
> > >> -				      sizeof(struct
> > >> -					     virtchnl_queue_pair_info))
> > >> ;
> > >> +			valid_len += flex_array_size(vqc, qpair,
> > >> +						     vqc-
> > >>> num_queue_pairs);
> > >
> > > The virtchnl file acts as a binary interface between physical and
> > > virtual functions. There's no guaruntee that the PF and VF will both
> > > have the newest version. Thus changing this will break compatibility.
> > > Specifically, the way the size was validated for this op code
> > > incorrectly expects an extra queue pair structure. Some other
> > > structures have similar length calculation flaws. We agree that fixing
> > > this is important, but the fix needs to account that old drivers will
> > > send an off by 1 size.
> > >
> > > To properly handle compatibility we need to introduce a feature flag to
> > > indicate the new behavior. If the feature flag is not set, we acccept
> > > messages with the old format (with the extra size). If both the PF and
> > > VF support the feature flag, we'll use the correct size calculations.
> > > We're looking to add this and would like to do all the virtchnl
> > > structure fixes in one series.
> > >
> > 
> > Oh OK, I see. In this case, I think something like this might work just
> > fine:
> > 
> > https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d
> > 
> > What do you think?
> > 
> 
> About half our virtchnl structures correctly validate the length (i.e. enforcing that the number of members including the implicit one are correct). There are maybe 3-4 which don't do that and accidentally allow sizes that are off by 1 member.
> 
> We believe the correct fix is to fix the structure definitions to use [] and then introduce a VALIDATE_MSG_V2 feature flag which is negotiated by the VF indicating whether it supports this behavior, and the PF replying to VF if it supports.
> 
> In the case where the VF doesn't support this, the PF will notice this and modify its length calculations for the handful of currently broken checks to include one extra member. In the case where the VF supports this but the PF does not, the VF must allocate extra memory and ensure it passes the larger message length. In the case where both PF and VF support the new "feature" we'll correctly switch to using 0 length flexible arrays.
> 
> It's actually even slightly more convoluted because another 3-4 ops only mis-validate the size when the length of the flexible array is 0. In that case, they require the full size of the structure, but in the case where it's 1 or more, they require the size to match as you would expect with a 0-sized array.
> 
> I'm not sure the union approach is suitable for that? We believe the use of a new capability bit is the best mechanism: we can fix the code to use flexible array definitions everywhere and simply ensure that when communicating with old PF or VF, we add additional padding as necessary to the message.

It seems like this can all be solved easily without versioning nor
unions. Currently, VIRTCHNL_OP_CONFIG_VSI_QUEUES requires that "msglen"
must be the header (struct virtchnl_vsi_queue_config_info) and at least
1 trailing qpair (struct virtchnl_queue_pair_info). There's no reason to
change this requirement.

We can leave the "over allocation" that is present in
drivers/net/ethernet/intel/iavf/iavf_virtchnl.c too:


diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 0eab3c43bdc5..66c3d1442ced 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -256,7 +256,7 @@ void iavf_configure_queues(struct iavf_adapter *adapter)
 		return;
 	}
 	adapter->current_op = VIRTCHNL_OP_CONFIG_VSI_QUEUES;
-	len = struct_size(vqci, qpair, pairs);
+	len = struct_size(vqci, qpair, pairs + 1);
 	vqci = kzalloc(len, GFP_KERNEL);
 	if (!vqci)
 		return;
diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index 8612f8fc86c1..d8d30dc98cd1 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
 	u16 vsi_id;
 	u16 num_queue_pairs;
 	u32 pad;
-	struct virtchnl_queue_pair_info qpair[1];
+	struct virtchnl_queue_pair_info qpair[0];
 };
 
-VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
 
 /* VIRTCHNL_OP_REQUEST_QUEUES
  * VF sends this message to request the PF to allocate additional queues to
@@ -993,18 +993,19 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
 	case VIRTCHNL_OP_CONFIG_RX_QUEUE:
 		valid_len = sizeof(struct virtchnl_rxq_info);
 		break;
-	case VIRTCHNL_OP_CONFIG_VSI_QUEUES:
-		valid_len = sizeof(struct virtchnl_vsi_queue_config_info);
+	case VIRTCHNL_OP_CONFIG_VSI_QUEUES: {
+		struct virtchnl_vsi_queue_config_info *vqc =
+		    (struct virtchnl_vsi_queue_config_info *)msg;
+
+		valid_len = struct_size(vqc, qpair, 1);
 		if (msglen >= valid_len) {
-			struct virtchnl_vsi_queue_config_info *vqc =
-			    (struct virtchnl_vsi_queue_config_info *)msg;
-			valid_len += (vqc->num_queue_pairs *
-				      sizeof(struct
-					     virtchnl_queue_pair_info));
+			valid_len += flex_array_size(vqc, qpair,
+						     vqc->num_queue_pairs);
 			if (vqc->num_queue_pairs == 0)
 				err_msg_format = true;
 		}
 		break;
+	}
 	case VIRTCHNL_OP_CONFIG_IRQ_MAP:
 		valid_len = sizeof(struct virtchnl_irq_map_info);
 		if (msglen >= valid_len) {



The above is a no-op change, and switches to flex arrays.

Additionally, these should be fixed as well:

struct virtchnl_vf_resource
struct virtchnl_irq_map_info
struct virtchnl_ether_addr_list
struct virtchnl_vlan_filter_list
struct virtchnl_rss_key
struct virtchnl_rss_lut
struct virtchnl_tc_info
struct virtchnl_iwarp_qvlist_info


-- 
Kees Cook

[Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Kees Cook]

On Sat, May 29, 2021 at 12:19:48AM +0000, Keller, Jacob E wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > Gustavo A. R. Silva
> > Sent: Friday, May 28, 2021 4:05 PM
> > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>; gustavoars at kernel.org
> > Cc: intel-wired-lan at lists.osuosl.org; linux-kernel at vger.kernel.org; linux-
> > hardening at vger.kernel.org
> > Subject: Re: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array
> > in struct virtchnl_vsi_queue_config_info
> > 
> > 
> > 
> > On 5/28/21 16:56, Nguyen, Anthony L wrote:
> > > On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
> > >> There is a regular need in the kernel to provide a way to declare
> > >> having a
> > >> dynamically sized set of trailing elements in a structure. Kernel
> > >> code
> > >> should always use ?flexible array members?[1] for these cases. The
> > >> older
> > >> style of one-element or zero-length arrays should no longer be
> > >> used[2].
> > >>
> > >> Refactor the code according to the use of a flexible-array member in
> > >> struct
> > >> virtchnl_vsi_queue_config_info instead of one-element array, and use
> > >> the
> > >> flex_array_size() helper.
> > >>
> > >> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > >> [2]
> > >> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-
> > length-and-one-element-arrays
> > >>
> > >> Link: https://github.com/KSPP/linux/issues/79
> > >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > >> ---
> > >>  include/linux/avf/virtchnl.h | 9 ++++-----
> > >>  1 file changed, 4 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/include/linux/avf/virtchnl.h
> > >> b/include/linux/avf/virtchnl.h
> > >> index b554913804bd..ed9c4998f8ac 100644
> > >> --- a/include/linux/avf/virtchnl.h
> > >> +++ b/include/linux/avf/virtchnl.h
> > >> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
> > >>  	u16 vsi_id;
> > >>  	u16 num_queue_pairs;
> > >>  	u32 pad;
> > >> -	struct virtchnl_queue_pair_info qpair[1];
> > >> +	struct virtchnl_queue_pair_info qpair[];
> > >>  };
> > >>
> > >> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
> > >> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
> > >>
> > >>  /* VIRTCHNL_OP_REQUEST_QUEUES
> > >>   * VF sends this message to request the PF to allocate additional
> > >> queues to
> > >> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
> > >> virtchnl_version_info *ver, u32 v_opcode,
> > >>  		if (msglen >= valid_len) {
> > >>  			struct virtchnl_vsi_queue_config_info *vqc =
> > >>  			    (struct virtchnl_vsi_queue_config_info
> > >> *)msg;
> > >> -			valid_len += (vqc->num_queue_pairs *
> > >> -				      sizeof(struct
> > >> -					     virtchnl_queue_pair_info))
> > >> ;
> > >> +			valid_len += flex_array_size(vqc, qpair,
> > >> +						     vqc-
> > >>> num_queue_pairs);
> > >
> > > The virtchnl file acts as a binary interface between physical and
> > > virtual functions. There's no guaruntee that the PF and VF will both
> > > have the newest version. Thus changing this will break compatibility.
> > > Specifically, the way the size was validated for this op code
> > > incorrectly expects an extra queue pair structure. Some other
> > > structures have similar length calculation flaws. We agree that fixing
> > > this is important, but the fix needs to account that old drivers will
> > > send an off by 1 size.
> > >
> > > To properly handle compatibility we need to introduce a feature flag to
> > > indicate the new behavior. If the feature flag is not set, we acccept
> > > messages with the old format (with the extra size). If both the PF and
> > > VF support the feature flag, we'll use the correct size calculations.
> > > We're looking to add this and would like to do all the virtchnl
> > > structure fixes in one series.
> > >
> > 
> > Oh OK, I see. In this case, I think something like this might work just
> > fine:
> > 
> > https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d
> > 
> > What do you think?
> > 
> 
> About half our virtchnl structures correctly validate the length (i.e. enforcing that the number of members including the implicit one are correct). There are maybe 3-4 which don't do that and accidentally allow sizes that are off by 1 member.
> 
> We believe the correct fix is to fix the structure definitions to use [] and then introduce a VALIDATE_MSG_V2 feature flag which is negotiated by the VF indicating whether it supports this behavior, and the PF replying to VF if it supports.
> 
> In the case where the VF doesn't support this, the PF will notice this and modify its length calculations for the handful of currently broken checks to include one extra member. In the case where the VF supports this but the PF does not, the VF must allocate extra memory and ensure it passes the larger message length. In the case where both PF and VF support the new "feature" we'll correctly switch to using 0 length flexible arrays.
> 
> It's actually even slightly more convoluted because another 3-4 ops only mis-validate the size when the length of the flexible array is 0. In that case, they require the full size of the structure, but in the case where it's 1 or more, they require the size to match as you would expect with a 0-sized array.
> 
> I'm not sure the union approach is suitable for that? We believe the use of a new capability bit is the best mechanism: we can fix the code to use flexible array definitions everywhere and simply ensure that when communicating with old PF or VF, we add additional padding as necessary to the message.

It seems like this can all be solved easily without versioning nor
unions. Currently, VIRTCHNL_OP_CONFIG_VSI_QUEUES requires that "msglen"
must be the header (struct virtchnl_vsi_queue_config_info) and at least
1 trailing qpair (struct virtchnl_queue_pair_info). There's no reason to
change this requirement.

We can leave the "over allocation" that is present in
drivers/net/ethernet/intel/iavf/iavf_virtchnl.c too:


diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 0eab3c43bdc5..66c3d1442ced 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -256,7 +256,7 @@ void iavf_configure_queues(struct iavf_adapter *adapter)
 		return;
 	}
 	adapter->current_op = VIRTCHNL_OP_CONFIG_VSI_QUEUES;
-	len = struct_size(vqci, qpair, pairs);
+	len = struct_size(vqci, qpair, pairs + 1);
 	vqci = kzalloc(len, GFP_KERNEL);
 	if (!vqci)
 		return;
diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index 8612f8fc86c1..d8d30dc98cd1 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
 	u16 vsi_id;
 	u16 num_queue_pairs;
 	u32 pad;
-	struct virtchnl_queue_pair_info qpair[1];
+	struct virtchnl_queue_pair_info qpair[0];
 };
 
-VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
 
 /* VIRTCHNL_OP_REQUEST_QUEUES
  * VF sends this message to request the PF to allocate additional queues to
@@ -993,18 +993,19 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
 	case VIRTCHNL_OP_CONFIG_RX_QUEUE:
 		valid_len = sizeof(struct virtchnl_rxq_info);
 		break;
-	case VIRTCHNL_OP_CONFIG_VSI_QUEUES:
-		valid_len = sizeof(struct virtchnl_vsi_queue_config_info);
+	case VIRTCHNL_OP_CONFIG_VSI_QUEUES: {
+		struct virtchnl_vsi_queue_config_info *vqc =
+		    (struct virtchnl_vsi_queue_config_info *)msg;
+
+		valid_len = struct_size(vqc, qpair, 1);
 		if (msglen >= valid_len) {
-			struct virtchnl_vsi_queue_config_info *vqc =
-			    (struct virtchnl_vsi_queue_config_info *)msg;
-			valid_len += (vqc->num_queue_pairs *
-				      sizeof(struct
-					     virtchnl_queue_pair_info));
+			valid_len += flex_array_size(vqc, qpair,
+						     vqc->num_queue_pairs);
 			if (vqc->num_queue_pairs == 0)
 				err_msg_format = true;
 		}
 		break;
+	}
 	case VIRTCHNL_OP_CONFIG_IRQ_MAP:
 		valid_len = sizeof(struct virtchnl_irq_map_info);
 		if (msglen >= valid_len) {



The above is a no-op change, and switches to flex arrays.

Additionally, these should be fixed as well:

struct virtchnl_vf_resource
struct virtchnl_irq_map_info
struct virtchnl_ether_addr_list
struct virtchnl_vlan_filter_list
struct virtchnl_rss_key
struct virtchnl_rss_lut
struct virtchnl_tc_info
struct virtchnl_iwarp_qvlist_info


-- 
Kees Cook

RE: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Keller, Jacob E]

> -----Original Message-----
> From: Kees Cook <keescook@chromium.org>
> Sent: Wednesday, June 09, 2021 2:45 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; gustavoars@kernel.org; intel-wired-
> lan@lists.osuosl.org; linux-kernel@vger.kernel.org; linux-
> hardening@vger.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array
> in struct virtchnl_vsi_queue_config_info
> 
> On Sat, May 29, 2021 at 12:19:48AM +0000, Keller, Jacob E wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > > Gustavo A. R. Silva
> > > Sent: Friday, May 28, 2021 4:05 PM
> > > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> > > <jesse.brandeburg@intel.com>; gustavoars@kernel.org
> > > Cc: intel-wired-lan@lists.osuosl.org; linux-kernel@vger.kernel.org; linux-
> > > hardening@vger.kernel.org
> > > Subject: Re: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element
> array
> > > in struct virtchnl_vsi_queue_config_info
> > >
> > >
> > >
> > > On 5/28/21 16:56, Nguyen, Anthony L wrote:
> > > > On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
> > > >> There is a regular need in the kernel to provide a way to declare
> > > >> having a
> > > >> dynamically sized set of trailing elements in a structure. Kernel
> > > >> code
> > > >> should always use “flexible array members”[1] for these cases. The
> > > >> older
> > > >> style of one-element or zero-length arrays should no longer be
> > > >> used[2].
> > > >>
> > > >> Refactor the code according to the use of a flexible-array member in
> > > >> struct
> > > >> virtchnl_vsi_queue_config_info instead of one-element array, and use
> > > >> the
> > > >> flex_array_size() helper.
> > > >>
> > > >> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > > >> [2]
> > > >> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-
> > > length-and-one-element-arrays
> > > >>
> > > >> Link: https://github.com/KSPP/linux/issues/79
> > > >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > > >> ---
> > > >>  include/linux/avf/virtchnl.h | 9 ++++-----
> > > >>  1 file changed, 4 insertions(+), 5 deletions(-)
> > > >>
> > > >> diff --git a/include/linux/avf/virtchnl.h
> > > >> b/include/linux/avf/virtchnl.h
> > > >> index b554913804bd..ed9c4998f8ac 100644
> > > >> --- a/include/linux/avf/virtchnl.h
> > > >> +++ b/include/linux/avf/virtchnl.h
> > > >> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
> > > >>  	u16 vsi_id;
> > > >>  	u16 num_queue_pairs;
> > > >>  	u32 pad;
> > > >> -	struct virtchnl_queue_pair_info qpair[1];
> > > >> +	struct virtchnl_queue_pair_info qpair[];
> > > >>  };
> > > >>
> > > >> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
> > > >> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
> > > >>
> > > >>  /* VIRTCHNL_OP_REQUEST_QUEUES
> > > >>   * VF sends this message to request the PF to allocate additional
> > > >> queues to
> > > >> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
> > > >> virtchnl_version_info *ver, u32 v_opcode,
> > > >>  		if (msglen >= valid_len) {
> > > >>  			struct virtchnl_vsi_queue_config_info *vqc =
> > > >>  			    (struct virtchnl_vsi_queue_config_info
> > > >> *)msg;
> > > >> -			valid_len += (vqc->num_queue_pairs *
> > > >> -				      sizeof(struct
> > > >> -					     virtchnl_queue_pair_info))
> > > >> ;
> > > >> +			valid_len += flex_array_size(vqc, qpair,
> > > >> +						     vqc-
> > > >>> num_queue_pairs);
> > > >
> > > > The virtchnl file acts as a binary interface between physical and
> > > > virtual functions. There's no guaruntee that the PF and VF will both
> > > > have the newest version. Thus changing this will break compatibility.
> > > > Specifically, the way the size was validated for this op code
> > > > incorrectly expects an extra queue pair structure. Some other
> > > > structures have similar length calculation flaws. We agree that fixing
> > > > this is important, but the fix needs to account that old drivers will
> > > > send an off by 1 size.
> > > >
> > > > To properly handle compatibility we need to introduce a feature flag to
> > > > indicate the new behavior. If the feature flag is not set, we acccept
> > > > messages with the old format (with the extra size). If both the PF and
> > > > VF support the feature flag, we'll use the correct size calculations.
> > > > We're looking to add this and would like to do all the virtchnl
> > > > structure fixes in one series.
> > > >
> > >
> > > Oh OK, I see. In this case, I think something like this might work just
> > > fine:
> > >
> > > https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d
> > >
> > > What do you think?
> > >
> >
> > About half our virtchnl structures correctly validate the length (i.e. enforcing
> that the number of members including the implicit one are correct). There are
> maybe 3-4 which don't do that and accidentally allow sizes that are off by 1
> member.
> >
> > We believe the correct fix is to fix the structure definitions to use [] and then
> introduce a VALIDATE_MSG_V2 feature flag which is negotiated by the VF
> indicating whether it supports this behavior, and the PF replying to VF if it
> supports.
> >
> > In the case where the VF doesn't support this, the PF will notice this and modify
> its length calculations for the handful of currently broken checks to include one
> extra member. In the case where the VF supports this but the PF does not, the VF
> must allocate extra memory and ensure it passes the larger message length. In
> the case where both PF and VF support the new "feature" we'll correctly switch
> to using 0 length flexible arrays.
> >
> > It's actually even slightly more convoluted because another 3-4 ops only mis-
> validate the size when the length of the flexible array is 0. In that case, they
> require the full size of the structure, but in the case where it's 1 or more, they
> require the size to match as you would expect with a 0-sized array.
> >
> > I'm not sure the union approach is suitable for that? We believe the use of a
> new capability bit is the best mechanism: we can fix the code to use flexible array
> definitions everywhere and simply ensure that when communicating with old PF
> or VF, we add additional padding as necessary to the message.
> 
> It seems like this can all be solved easily without versioning nor
> unions. Currently, VIRTCHNL_OP_CONFIG_VSI_QUEUES requires that "msglen"
> must be the header (struct virtchnl_vsi_queue_config_info) and at least
> 1 trailing qpair (struct virtchnl_queue_pair_info). There's no reason to
> change this requirement.
> 
> We can leave the "over allocation" that is present in
> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c too:
> 
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 0eab3c43bdc5..66c3d1442ced 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -256,7 +256,7 @@ void iavf_configure_queues(struct iavf_adapter
> *adapter)
>  		return;
>  	}
>  	adapter->current_op = VIRTCHNL_OP_CONFIG_VSI_QUEUES;
> -	len = struct_size(vqci, qpair, pairs);
> +	len = struct_size(vqci, qpair, pairs + 1);
>  	vqci = kzalloc(len, GFP_KERNEL);
>  	if (!vqci)
>  		return;
> diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
> index 8612f8fc86c1..d8d30dc98cd1 100644
> --- a/include/linux/avf/virtchnl.h
> +++ b/include/linux/avf/virtchnl.h
> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
>  	u16 vsi_id;
>  	u16 num_queue_pairs;
>  	u32 pad;
> -	struct virtchnl_queue_pair_info qpair[1];
> +	struct virtchnl_queue_pair_info qpair[0];
>  };
> 
> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
> 
>  /* VIRTCHNL_OP_REQUEST_QUEUES
>   * VF sends this message to request the PF to allocate additional queues to
> @@ -993,18 +993,19 @@ virtchnl_vc_validate_vf_msg(struct
> virtchnl_version_info *ver, u32 v_opcode,
>  	case VIRTCHNL_OP_CONFIG_RX_QUEUE:
>  		valid_len = sizeof(struct virtchnl_rxq_info);
>  		break;
> -	case VIRTCHNL_OP_CONFIG_VSI_QUEUES:
> -		valid_len = sizeof(struct virtchnl_vsi_queue_config_info);
> +	case VIRTCHNL_OP_CONFIG_VSI_QUEUES: {
> +		struct virtchnl_vsi_queue_config_info *vqc =
> +		    (struct virtchnl_vsi_queue_config_info *)msg;
> +
> +		valid_len = struct_size(vqc, qpair, 1);
>  		if (msglen >= valid_len) {
> -			struct virtchnl_vsi_queue_config_info *vqc =
> -			    (struct virtchnl_vsi_queue_config_info *)msg;
> -			valid_len += (vqc->num_queue_pairs *
> -				      sizeof(struct
> -					     virtchnl_queue_pair_info));
> +			valid_len += flex_array_size(vqc, qpair,
> +						     vqc->num_queue_pairs);
>  			if (vqc->num_queue_pairs == 0)
>  				err_msg_format = true;
>  		}
>  		break;
> +	}
>  	case VIRTCHNL_OP_CONFIG_IRQ_MAP:
>  		valid_len = sizeof(struct virtchnl_irq_map_info);
>  		if (msglen >= valid_len) {
> 
> 
> 
> The above is a no-op change, and switches to flex arrays.
> 

I think there are three cases, but this approach should work for them all:

1) messages which require the extra allocation regardless of size of the flexible array
2) messages which only require the extra allocation if the size is 0
3) messages which don't have this issue because a size of 0 is invalid and rejected.

As long as we fix them all to correctly enforce the "send 1 extra size" in the right places, I think we are ok.

> Additionally, these should be fixed as well:
> 
> struct virtchnl_vf_resource
> struct virtchnl_irq_map_info
> struct virtchnl_ether_addr_list
> struct virtchnl_vlan_filter_list
> struct virtchnl_rss_key
> struct virtchnl_rss_lut
> struct virtchnl_tc_info
> struct virtchnl_iwarp_qvlist_info
> 
> 
> --
> Kees Cook

[Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info

[Keller, Jacob E]

> -----Original Message-----
> From: Kees Cook <keescook@chromium.org>
> Sent: Wednesday, June 09, 2021 2:45 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; gustavoars at kernel.org; intel-wired-
> lan at lists.osuosl.org; linux-kernel at vger.kernel.org; linux-
> hardening at vger.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element array
> in struct virtchnl_vsi_queue_config_info
> 
> On Sat, May 29, 2021 at 12:19:48AM +0000, Keller, Jacob E wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > > Gustavo A. R. Silva
> > > Sent: Friday, May 28, 2021 4:05 PM
> > > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> > > <jesse.brandeburg@intel.com>; gustavoars at kernel.org
> > > Cc: intel-wired-lan at lists.osuosl.org; linux-kernel at vger.kernel.org; linux-
> > > hardening at vger.kernel.org
> > > Subject: Re: [Intel-wired-lan] [PATCH][next] virtchnl: Replace one-element
> array
> > > in struct virtchnl_vsi_queue_config_info
> > >
> > >
> > >
> > > On 5/28/21 16:56, Nguyen, Anthony L wrote:
> > > > On Tue, 2021-05-25 at 18:16 -0500, Gustavo A. R. Silva wrote:
> > > >> There is a regular need in the kernel to provide a way to declare
> > > >> having a
> > > >> dynamically sized set of trailing elements in a structure. Kernel
> > > >> code
> > > >> should always use ?flexible array members?[1] for these cases. The
> > > >> older
> > > >> style of one-element or zero-length arrays should no longer be
> > > >> used[2].
> > > >>
> > > >> Refactor the code according to the use of a flexible-array member in
> > > >> struct
> > > >> virtchnl_vsi_queue_config_info instead of one-element array, and use
> > > >> the
> > > >> flex_array_size() helper.
> > > >>
> > > >> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > > >> [2]
> > > >> https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-
> > > length-and-one-element-arrays
> > > >>
> > > >> Link: https://github.com/KSPP/linux/issues/79
> > > >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > > >> ---
> > > >>  include/linux/avf/virtchnl.h | 9 ++++-----
> > > >>  1 file changed, 4 insertions(+), 5 deletions(-)
> > > >>
> > > >> diff --git a/include/linux/avf/virtchnl.h
> > > >> b/include/linux/avf/virtchnl.h
> > > >> index b554913804bd..ed9c4998f8ac 100644
> > > >> --- a/include/linux/avf/virtchnl.h
> > > >> +++ b/include/linux/avf/virtchnl.h
> > > >> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
> > > >>  	u16 vsi_id;
> > > >>  	u16 num_queue_pairs;
> > > >>  	u32 pad;
> > > >> -	struct virtchnl_queue_pair_info qpair[1];
> > > >> +	struct virtchnl_queue_pair_info qpair[];
> > > >>  };
> > > >>
> > > >> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
> > > >> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
> > > >>
> > > >>  /* VIRTCHNL_OP_REQUEST_QUEUES
> > > >>   * VF sends this message to request the PF to allocate additional
> > > >> queues to
> > > >> @@ -997,9 +997,8 @@ virtchnl_vc_validate_vf_msg(struct
> > > >> virtchnl_version_info *ver, u32 v_opcode,
> > > >>  		if (msglen >= valid_len) {
> > > >>  			struct virtchnl_vsi_queue_config_info *vqc =
> > > >>  			    (struct virtchnl_vsi_queue_config_info
> > > >> *)msg;
> > > >> -			valid_len += (vqc->num_queue_pairs *
> > > >> -				      sizeof(struct
> > > >> -					     virtchnl_queue_pair_info))
> > > >> ;
> > > >> +			valid_len += flex_array_size(vqc, qpair,
> > > >> +						     vqc-
> > > >>> num_queue_pairs);
> > > >
> > > > The virtchnl file acts as a binary interface between physical and
> > > > virtual functions. There's no guaruntee that the PF and VF will both
> > > > have the newest version. Thus changing this will break compatibility.
> > > > Specifically, the way the size was validated for this op code
> > > > incorrectly expects an extra queue pair structure. Some other
> > > > structures have similar length calculation flaws. We agree that fixing
> > > > this is important, but the fix needs to account that old drivers will
> > > > send an off by 1 size.
> > > >
> > > > To properly handle compatibility we need to introduce a feature flag to
> > > > indicate the new behavior. If the feature flag is not set, we acccept
> > > > messages with the old format (with the extra size). If both the PF and
> > > > VF support the feature flag, we'll use the correct size calculations.
> > > > We're looking to add this and would like to do all the virtchnl
> > > > structure fixes in one series.
> > > >
> > >
> > > Oh OK, I see. In this case, I think something like this might work just
> > > fine:
> > >
> > > https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d
> > >
> > > What do you think?
> > >
> >
> > About half our virtchnl structures correctly validate the length (i.e. enforcing
> that the number of members including the implicit one are correct). There are
> maybe 3-4 which don't do that and accidentally allow sizes that are off by 1
> member.
> >
> > We believe the correct fix is to fix the structure definitions to use [] and then
> introduce a VALIDATE_MSG_V2 feature flag which is negotiated by the VF
> indicating whether it supports this behavior, and the PF replying to VF if it
> supports.
> >
> > In the case where the VF doesn't support this, the PF will notice this and modify
> its length calculations for the handful of currently broken checks to include one
> extra member. In the case where the VF supports this but the PF does not, the VF
> must allocate extra memory and ensure it passes the larger message length. In
> the case where both PF and VF support the new "feature" we'll correctly switch
> to using 0 length flexible arrays.
> >
> > It's actually even slightly more convoluted because another 3-4 ops only mis-
> validate the size when the length of the flexible array is 0. In that case, they
> require the full size of the structure, but in the case where it's 1 or more, they
> require the size to match as you would expect with a 0-sized array.
> >
> > I'm not sure the union approach is suitable for that? We believe the use of a
> new capability bit is the best mechanism: we can fix the code to use flexible array
> definitions everywhere and simply ensure that when communicating with old PF
> or VF, we add additional padding as necessary to the message.
> 
> It seems like this can all be solved easily without versioning nor
> unions. Currently, VIRTCHNL_OP_CONFIG_VSI_QUEUES requires that "msglen"
> must be the header (struct virtchnl_vsi_queue_config_info) and at least
> 1 trailing qpair (struct virtchnl_queue_pair_info). There's no reason to
> change this requirement.
> 
> We can leave the "over allocation" that is present in
> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c too:
> 
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 0eab3c43bdc5..66c3d1442ced 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -256,7 +256,7 @@ void iavf_configure_queues(struct iavf_adapter
> *adapter)
>  		return;
>  	}
>  	adapter->current_op = VIRTCHNL_OP_CONFIG_VSI_QUEUES;
> -	len = struct_size(vqci, qpair, pairs);
> +	len = struct_size(vqci, qpair, pairs + 1);
>  	vqci = kzalloc(len, GFP_KERNEL);
>  	if (!vqci)
>  		return;
> diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
> index 8612f8fc86c1..d8d30dc98cd1 100644
> --- a/include/linux/avf/virtchnl.h
> +++ b/include/linux/avf/virtchnl.h
> @@ -338,10 +338,10 @@ struct virtchnl_vsi_queue_config_info {
>  	u16 vsi_id;
>  	u16 num_queue_pairs;
>  	u32 pad;
> -	struct virtchnl_queue_pair_info qpair[1];
> +	struct virtchnl_queue_pair_info qpair[0];
>  };
> 
> -VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
> +VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
> 
>  /* VIRTCHNL_OP_REQUEST_QUEUES
>   * VF sends this message to request the PF to allocate additional queues to
> @@ -993,18 +993,19 @@ virtchnl_vc_validate_vf_msg(struct
> virtchnl_version_info *ver, u32 v_opcode,
>  	case VIRTCHNL_OP_CONFIG_RX_QUEUE:
>  		valid_len = sizeof(struct virtchnl_rxq_info);
>  		break;
> -	case VIRTCHNL_OP_CONFIG_VSI_QUEUES:
> -		valid_len = sizeof(struct virtchnl_vsi_queue_config_info);
> +	case VIRTCHNL_OP_CONFIG_VSI_QUEUES: {
> +		struct virtchnl_vsi_queue_config_info *vqc =
> +		    (struct virtchnl_vsi_queue_config_info *)msg;
> +
> +		valid_len = struct_size(vqc, qpair, 1);
>  		if (msglen >= valid_len) {
> -			struct virtchnl_vsi_queue_config_info *vqc =
> -			    (struct virtchnl_vsi_queue_config_info *)msg;
> -			valid_len += (vqc->num_queue_pairs *
> -				      sizeof(struct
> -					     virtchnl_queue_pair_info));
> +			valid_len += flex_array_size(vqc, qpair,
> +						     vqc->num_queue_pairs);
>  			if (vqc->num_queue_pairs == 0)
>  				err_msg_format = true;
>  		}
>  		break;
> +	}
>  	case VIRTCHNL_OP_CONFIG_IRQ_MAP:
>  		valid_len = sizeof(struct virtchnl_irq_map_info);
>  		if (msglen >= valid_len) {
> 
> 
> 
> The above is a no-op change, and switches to flex arrays.
> 

I think there are three cases, but this approach should work for them all:

1) messages which require the extra allocation regardless of size of the flexible array
2) messages which only require the extra allocation if the size is 0
3) messages which don't have this issue because a size of 0 is invalid and rejected.

As long as we fix them all to correctly enforce the "send 1 extra size" in the right places, I think we are ok.

> Additionally, these should be fixed as well:
> 
> struct virtchnl_vf_resource
> struct virtchnl_irq_map_info
> struct virtchnl_ether_addr_list
> struct virtchnl_vlan_filter_list
> struct virtchnl_rss_key
> struct virtchnl_rss_lut
> struct virtchnl_tc_info
> struct virtchnl_iwarp_qvlist_info
> 
> 
> --
> Kees Cook