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: <9f6b830f-d2ee-4fde-a131-a956a6e84df7@molgen.mpg.de>
Date: Tue, 4 Mar 2025 12:15:57 +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>
Subject: Re: [Intel-wired-lan] [iwl-net v3 1/5] virtchnl: make proto and
 filter action count unsigned

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?

>   	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