From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
To: "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
"Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
"gustavoars@kernel.org" <gustavoars@kernel.org>
Cc: "intel-wired-lan@lists.osuosl.org"
<intel-wired-lan@lists.osuosl.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-hardening@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
Date: Fri, 28 May 2021 18:04:51 -0500 [thread overview]
Message-ID: <85e62583-5db9-ac5c-fdf9-8e3f6a50f3fa@embeddedor.com> (raw)
In-Reply-To: <f3674339c0390ced22b365101f2d3e3a2bf26845.camel@intel.com>
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
next prev parent reply other threads:[~2021-05-28 23:28 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-25 23:16 [PATCH][next] virtchnl: Replace one-element array in struct virtchnl_vsi_queue_config_info Gustavo A. R. Silva
2021-05-28 21:56 ` Nguyen, Anthony L
2021-05-28 23:04 ` Gustavo A. R. Silva [this message]
2021-05-29 0:19 ` [Intel-wired-lan] " Keller, Jacob E
2021-06-09 21:45 ` Kees Cook
2021-06-10 0:03 ` Keller, Jacob E
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=85e62583-5db9-ac5c-fdf9-8e3f6a50f3fa@embeddedor.com \
--to=gustavo@embeddedor.com \
--cc=anthony.l.nguyen@intel.com \
--cc=gustavoars@kernel.org \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=jesse.brandeburg@intel.com \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).