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 for Android: free password hash cracker in your pocket

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

Message-ID: <e384e61a-4ccd-4ae7-8ddd-66259769f6dd@linux.intel.com>
Date: Tue, 4 Mar 2025 14:11:42 +0100
From: "Szapar-Mudlaw, Martyna" <martyna.szapar-mudlaw@...ux.intel.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>, 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



On 3/4/2025 12:51 PM, Paul Menzel wrote:
> 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].

Thanks for sharing the source and your perspective, you are right, as a 
general rule, using generic types is preferred - I actually learned 
something new from this.
That said, I still believe there are exceptions, and in this case, using 
u32 is the right choice. When dealing with protocols or data formats 
using a fixed-width type makes sense.
Additionally, throughout this file, we consistently use u32/u16 for 
similar cases, so also here we're keeping it aligned with the existing 
codebase.
Thank you for your review and appreciate the discussion on best practices.

Regards,
Martyna

> 
>>>>       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
>