netdev - Re: [Intel-wired-lan] [iwl-net v3 1/5] virtchnl: make proto and filter action count unsigned

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <832cc2a5-0c15-42d1-924b-a14674db6391@molgen.mpg.de>
Date: Tue, 4 Mar 2025 12:51:51 +0100
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@...ux.intel.com>,
 Jan Glaza <jan.glaza@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
 Jedrzej Jagielski <jedrzej.jagielski@...el.com>,
 Simon Horman <horms@...nel.org>,
 Alexander Lobakin <aleksander.lobakin@...el.com>
Subject: Re: [Intel-wired-lan] [iwl-net v3 1/5] virtchnl: make proto and
 filter action count unsigned

Dear Martyna,


Thank you for your quick reply.

Am 04.03.25 um 12:45 schrieb Szapar-Mudlaw, Martyna:

> On 3/4/2025 12:15 PM, Paul Menzel wrote:

>> Am 04.03.25 um 12:08 schrieb Martyna Szapar-Mudlaw:
>>> From: Jan Glaza <jan.glaza@...el.com>
>>>
>>> The count field in virtchnl_proto_hdrs and virtchnl_filter_action_set
>>> should never be negative while still being valid. Changing it from
>>> int to u32 ensures proper handling of values in virtchnl messages in
>>> driverrs and prevents unintended behavior.
>>> In its current signed form, a negative count does not trigger
>>> an error in ice driver but instead results in it being treated as 0.
>>> This can lead to unexpected outcomes when processing messages.
>>> By using u32, any invalid values will correctly trigger -EINVAL,
>>> making error detection more robust.
>>>
>>> Fixes: 1f7ea1cd6a374 ("ice: Enable FDIR Configure for AVF")
>>> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>>> Reviewed-by: Simon Horman <horms@...nel.org>
>>> Signed-off-by: Jan Glaza <jan.glaza@...el.com>
>>> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@...ux.intel.com>
>>> ---
>>>   include/linux/avf/virtchnl.h | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
>>> index 4811b9a14604..cf0afa60e4a7 100644
>>> --- a/include/linux/avf/virtchnl.h
>>> +++ b/include/linux/avf/virtchnl.h
>>> @@ -1343,7 +1343,7 @@ struct virtchnl_proto_hdrs {
>>>        * 2 - from the second inner layer
>>>        * ....
>>>        **/
>>> -    int count; /* the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS */
>>> +    u32 count; /* the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS */
>>
>> Why limit the length, and not use unsigned int?
> 
> u32 range is completely sufficient for number of proto hdrs (as said: 
> "the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS") and I believe it 
> is recommended to use fixed sized variables where possible

Do you have a pointer to the recommendation? I heard the opposite, that 
fixed length is only useful for register writes. Otherwise, you should 
use the “generic” types [1].

>>>       union {
>>>           struct virtchnl_proto_hdr
>>>               proto_hdr[VIRTCHNL_MAX_NUM_PROTO_HDRS];
>>> @@ -1395,7 +1395,7 @@ VIRTCHNL_CHECK_STRUCT_LEN(36, virtchnl_filter_action);
>>>   struct virtchnl_filter_action_set {
>>>       /* action number must be less then VIRTCHNL_MAX_NUM_ACTIONS */
>>> -    int count;
>>> +    u32 count;
>>>       struct virtchnl_filter_action actions[VIRTCHNL_MAX_NUM_ACTIONS];
>>>   };

Kind regards,

Paul


[1]: https://notabs.org/coding/smallIntsBigPenalty.htm