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: <00a160e5-c9b2-4b91-9823-dee37fdc5d25@linux.intel.com>
Date: Tue, 4 Mar 2025 12:45:53 +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>
Subject: Re: [Intel-wired-lan] [iwl-net v3 1/5] virtchnl: make proto and
 filter action count unsigned



On 3/4/2025 12:15 PM, Paul Menzel wrote:
> Dear Jan, dear Martina,
> 
> 
> Thank you for the patch.
> 
> 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

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