[iwl-net v3 0/5] ice: fix validation issues in virtchnl parameters

[iwl-net v3 0/5] ice: fix validation issues in virtchnl parameters

[Martyna Szapar-Mudlaw]

This patch series addresses validation issues in the virtchnl interface
of the ice driver. These fixes correct improper value checking,
ensuring that the driver can properly handle and reject invalid inputs
from potentially malicious VFs. By fixing validation mechanisms,
these patches strictly enforce existing constraints to prevent
out-of-bounds scenarios, making the system more robust against incorrect
or unexpected data.

---

v3 -> v2:
removed redundant check and fixed kfree being called on uninitialized var in 5. patch

v2 -> v1:
attached Mateusz's related patch
rephrase some commit messages to indicate that this are fixes and should target net

--- 

Jan Glaza (3):
  virtchnl: make proto and filter action count unsigned
  ice: stop truncating queue ids when checking
  ice: validate queue quanta parameters to prevent OOB access

Lukasz Czapnik (1):
  ice: fix input validation for virtchnl BW

Mateusz Polchlopek (1):
  ice: fix using untrusted value of pkt_len in ice_vc_fdir_parse_raw()

 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 39 +++++++++++++++----
 .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 24 +++++++-----
 include/linux/avf/virtchnl.h                  |  4 +-
 3 files changed, 48 insertions(+), 19 deletions(-)

-- 
2.47.0

[iwl-net v3 1/5] virtchnl: make proto and filter action count unsigned

[Martyna Szapar-Mudlaw]

From: Jan Glaza <jan.glaza@intel.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@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jan Glaza <jan.glaza@intel.com>
Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.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 */
 	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];
 };
 
-- 
2.47.0

[iwl-net v3 2/5] ice: stop truncating queue ids when checking

[Martyna Szapar-Mudlaw]

From: Jan Glaza <jan.glaza@intel.com>

Queue IDs can be up to 4096, fix invalid check to stop
truncating IDs to 8 bits.

Fixes: bf93bf791cec8 ("ice: introduce ice_virtchnl.c and ice_virtchnl.h")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jan Glaza <jan.glaza@intel.com>
Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index b6285433307c..343f2b4b0dc5 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -565,7 +565,7 @@ bool ice_vc_isvalid_vsi_id(struct ice_vf *vf, u16 vsi_id)
  *
  * check for the valid queue ID
  */
-static bool ice_vc_isvalid_q_id(struct ice_vsi *vsi, u8 qid)
+static bool ice_vc_isvalid_q_id(struct ice_vsi *vsi, u16 qid)
 {
 	/* allocated Tx and Rx queues should be always equal for VF VSI */
 	return qid < vsi->alloc_txq;
-- 
2.47.0

[iwl-net v3 3/5] ice: validate queue quanta parameters to prevent OOB access

[Martyna Szapar-Mudlaw]

From: Jan Glaza <jan.glaza@intel.com>

Add queue wraparound prevention in quanta configuration.
Ensure end_qid does not overflow by validating start_qid and num_queues.

Fixes: 015307754a19 ("ice: Support VF queue rate limit and quanta size configuration")
Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jan Glaza <jan.glaza@intel.com>
Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index 343f2b4b0dc5..adb1bf12542f 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -1903,13 +1903,21 @@ static int ice_vc_cfg_q_bw(struct ice_vf *vf, u8 *msg)
  */
 static int ice_vc_cfg_q_quanta(struct ice_vf *vf, u8 *msg)
 {
+	u16 quanta_prof_id, quanta_size, start_qid, num_queues, end_qid, i;
 	enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
-	u16 quanta_prof_id, quanta_size, start_qid, end_qid, i;
 	struct virtchnl_quanta_cfg *qquanta =
 		(struct virtchnl_quanta_cfg *)msg;
 	struct ice_vsi *vsi;
 	int ret;
 
+	start_qid = qquanta->queue_select.start_queue_id;
+	num_queues = qquanta->queue_select.num_queues;
+
+	if (check_add_overflow(start_qid, num_queues, &end_qid)) {
+		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+		goto err;
+	}
+
 	if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
 		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
 		goto err;
@@ -1921,8 +1929,6 @@ static int ice_vc_cfg_q_quanta(struct ice_vf *vf, u8 *msg)
 		goto err;
 	}
 
-	end_qid = qquanta->queue_select.start_queue_id +
-		  qquanta->queue_select.num_queues;
 	if (end_qid > ICE_MAX_RSS_QS_PER_VF ||
 	    end_qid > min_t(u16, vsi->alloc_txq, vsi->alloc_rxq)) {
 		dev_err(ice_pf_to_dev(vf->pf), "VF-%d trying to configure more than allocated number of queues: %d\n",
@@ -1951,7 +1957,6 @@ static int ice_vc_cfg_q_quanta(struct ice_vf *vf, u8 *msg)
 		goto err;
 	}
 
-	start_qid = qquanta->queue_select.start_queue_id;
 	for (i = start_qid; i < end_qid; i++)
 		vsi->tx_rings[i]->quanta_prof_id = quanta_prof_id;
 
-- 
2.47.0

[iwl-net v3 4/5] ice: fix input validation for virtchnl BW

[Martyna Szapar-Mudlaw]

From: Lukasz Czapnik <lukasz.czapnik@intel.com>

Add missing validation of tc and queue id values sent by a VF in
ice_vc_cfg_q_bw().
Additionally fixed logged value in the warning message,
where max_tx_rate was incorrectly referenced instead of min_tx_rate.
Also correct error handling in this function by properly exiting
when invalid configuration is detected.

Fixes: 015307754a19 ("ice: Support VF queue rate limit and quanta size configuration")
Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Lukasz Czapnik <lukasz.czapnik@intel.com>
Co-developed-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 24 ++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index adb1bf12542f..824ef849b0ea 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -1865,15 +1865,33 @@ static int ice_vc_cfg_q_bw(struct ice_vf *vf, u8 *msg)
 
 	for (i = 0; i < qbw->num_queues; i++) {
 		if (qbw->cfg[i].shaper.peak != 0 && vf->max_tx_rate != 0 &&
-		    qbw->cfg[i].shaper.peak > vf->max_tx_rate)
+		    qbw->cfg[i].shaper.peak > vf->max_tx_rate) {
 			dev_warn(ice_pf_to_dev(vf->pf), "The maximum queue %d rate limit configuration may not take effect because the maximum TX rate for VF-%d is %d\n",
 				 qbw->cfg[i].queue_id, vf->vf_id,
 				 vf->max_tx_rate);
+			v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+			goto err;
+		}
 		if (qbw->cfg[i].shaper.committed != 0 && vf->min_tx_rate != 0 &&
-		    qbw->cfg[i].shaper.committed < vf->min_tx_rate)
+		    qbw->cfg[i].shaper.committed < vf->min_tx_rate) {
 			dev_warn(ice_pf_to_dev(vf->pf), "The minimum queue %d rate limit configuration may not take effect because the minimum TX rate for VF-%d is %d\n",
 				 qbw->cfg[i].queue_id, vf->vf_id,
-				 vf->max_tx_rate);
+				 vf->min_tx_rate);
+			v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+			goto err;
+		}
+		if (qbw->cfg[i].queue_id > vf->num_vf_qs) {
+			dev_warn(ice_pf_to_dev(vf->pf), "VF-%d trying to configure invalid queue_id\n",
+				 vf->vf_id);
+			v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+			goto err;
+		}
+		if (qbw->cfg[i].tc >= ICE_MAX_TRAFFIC_CLASS) {
+			dev_warn(ice_pf_to_dev(vf->pf), "VF-%d trying to configure a traffic class higher than allowed\n",
+				 vf->vf_id);
+			v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+			goto err;
+		}
 	}
 
 	for (i = 0; i < qbw->num_queues; i++) {
-- 
2.47.0

[iwl-net v3 5/5] ice: fix using untrusted value of pkt_len in ice_vc_fdir_parse_raw()

[Martyna Szapar-Mudlaw]

From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>

Fix using the untrusted value of proto->raw.pkt_len in function
ice_vc_fdir_parse_raw() by verifying if it does not exceed the
VIRTCHNL_MAX_SIZE_RAW_PACKET value.

Fixes: 99f419df8a5c ("ice: enable FDIR filters from raw binary patterns for VFs")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
---
 .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 24 ++++++++++++-------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
index 14e3f0f89c78..9be4bd717512 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
@@ -832,21 +832,27 @@ ice_vc_fdir_parse_raw(struct ice_vf *vf,
 		      struct virtchnl_proto_hdrs *proto,
 		      struct virtchnl_fdir_fltr_conf *conf)
 {
-	u8 *pkt_buf, *msk_buf __free(kfree);
+	u8 *pkt_buf, *msk_buf __free(kfree) = NULL;
 	struct ice_parser_result rslt;
 	struct ice_pf *pf = vf->pf;
+	u16 pkt_len, udp_port = 0;
 	struct ice_parser *psr;
 	int status = -ENOMEM;
 	struct ice_hw *hw;
-	u16 udp_port = 0;
 
-	pkt_buf = kzalloc(proto->raw.pkt_len, GFP_KERNEL);
-	msk_buf = kzalloc(proto->raw.pkt_len, GFP_KERNEL);
+	pkt_len = proto->raw.pkt_len;
+
+	if (!pkt_len || pkt_len > VIRTCHNL_MAX_SIZE_RAW_PACKET)
+		return -EINVAL;
+
+	pkt_buf = kzalloc(pkt_len, GFP_KERNEL);
+	msk_buf = kzalloc(pkt_len, GFP_KERNEL);
+
 	if (!pkt_buf || !msk_buf)
 		goto err_mem_alloc;
 
-	memcpy(pkt_buf, proto->raw.spec, proto->raw.pkt_len);
-	memcpy(msk_buf, proto->raw.mask, proto->raw.pkt_len);
+	memcpy(pkt_buf, proto->raw.spec, pkt_len);
+	memcpy(msk_buf, proto->raw.mask, pkt_len);
 
 	hw = &pf->hw;
 
@@ -862,7 +868,7 @@ ice_vc_fdir_parse_raw(struct ice_vf *vf,
 	if (ice_get_open_tunnel_port(hw, &udp_port, TNL_VXLAN))
 		ice_parser_vxlan_tunnel_set(psr, udp_port, true);
 
-	status = ice_parser_run(psr, pkt_buf, proto->raw.pkt_len, &rslt);
+	status = ice_parser_run(psr, pkt_buf, pkt_len, &rslt);
 	if (status)
 		goto err_parser_destroy;
 
@@ -876,7 +882,7 @@ ice_vc_fdir_parse_raw(struct ice_vf *vf,
 	}
 
 	status = ice_parser_profile_init(&rslt, pkt_buf, msk_buf,
-					 proto->raw.pkt_len, ICE_BLK_FD,
+					 pkt_len, ICE_BLK_FD,
 					 conf->prof);
 	if (status)
 		goto err_parser_profile_init;
@@ -885,7 +891,7 @@ ice_vc_fdir_parse_raw(struct ice_vf *vf,
 		ice_parser_profile_dump(hw, conf->prof);
 
 	/* Store raw flow info into @conf */
-	conf->pkt_len = proto->raw.pkt_len;
+	conf->pkt_len = pkt_len;
 	conf->pkt_buf = pkt_buf;
 	conf->parser_ena = true;
 
-- 
2.47.0

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

[Paul Menzel]

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@intel.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@intel.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Jan Glaza <jan.glaza@intel.com>
> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.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

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

[Szapar-Mudlaw, Martyna]

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@intel.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@intel.com>
>> Reviewed-by: Simon Horman <horms@kernel.org>
>> Signed-off-by: Jan Glaza <jan.glaza@intel.com>
>> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar- 
>> mudlaw@linux.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
> 

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

[Paul Menzel]

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@intel.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@intel.com>
>>> Reviewed-by: Simon Horman <horms@kernel.org>
>>> Signed-off-by: Jan Glaza <jan.glaza@intel.com>
>>> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.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

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

[Szapar-Mudlaw, Martyna]

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@intel.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@intel.com>
>>>> Reviewed-by: Simon Horman <horms@kernel.org>
>>>> Signed-off-by: Jan Glaza <jan.glaza@intel.com>
>>>> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar- 
>>>> mudlaw@linux.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
> 

RE: [Intel-wired-lan] [iwl-net v3 5/5] ice: fix using untrusted value of pkt_len in ice_vc_fdir_parse_raw()

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Martyna Szapar-Mudlaw
> Sent: Tuesday, March 4, 2025 12:09 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Polchlopek, Mateusz
> <mateusz.polchlopek@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Martyna Szapar-Mudlaw <martyna.szapar-
> mudlaw@linux.intel.com>
> Subject: [Intel-wired-lan] [iwl-net v3 5/5] ice: fix using untrusted value of pkt_len
> in ice_vc_fdir_parse_raw()
> 
> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
> 
> Fix using the untrusted value of proto->raw.pkt_len in function
> ice_vc_fdir_parse_raw() by verifying if it does not exceed the
> VIRTCHNL_MAX_SIZE_RAW_PACKET value.
> 
> Fixes: 99f419df8a5c ("ice: enable FDIR filters from raw binary patterns for VFs")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-
> mudlaw@linux.intel.com>
> ---
>  .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 24 ++++++++++++-------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> index 14e3f0f89c78..9be4bd717512 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> @@ -832,21 +832,27 @@ ice_vc_fdir_parse_raw(struct ice_vf *vf,

Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>

RE: [Intel-wired-lan] [iwl-net v3 4/5] ice: fix input validation for virtchnl BW

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Martyna Szapar-Mudlaw
> Sent: Tuesday, March 4, 2025 12:09 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Czapnik, Lukasz <lukasz.czapnik@intel.com>;
> Jagielski, Jedrzej <jedrzej.jagielski@intel.com>; Simon Horman
> <horms@kernel.org>; Martyna Szapar-Mudlaw <martyna.szapar-
> mudlaw@linux.intel.com>
> Subject: [Intel-wired-lan] [iwl-net v3 4/5] ice: fix input validation for virtchnl BW
> 
> From: Lukasz Czapnik <lukasz.czapnik@intel.com>
> 
> Add missing validation of tc and queue id values sent by a VF in
> ice_vc_cfg_q_bw().
> Additionally fixed logged value in the warning message, where max_tx_rate was
> incorrectly referenced instead of min_tx_rate.
> Also correct error handling in this function by properly exiting when invalid
> configuration is detected.
> 
> Fixes: 015307754a19 ("ice: Support VF queue rate limit and quanta size
> configuration")
> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Lukasz Czapnik <lukasz.czapnik@intel.com>
> Co-developed-by: Martyna Szapar-Mudlaw <martyna.szapar-
> mudlaw@linux.intel.com>
> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-
> mudlaw@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_virtchnl.c | 24 ++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> index adb1bf12542f..824ef849b0ea 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> @@ -1865,15 +1865,33 @@ static int ice_vc_cfg_q_bw(struct ice_vf *vf, u8

Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>

RE: [Intel-wired-lan] [iwl-net v3 3/5] ice: validate queue quanta parameters to prevent OOB access

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Martyna Szapar-Mudlaw
> Sent: Tuesday, March 4, 2025 12:09 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Glaza, Jan <jan.glaza@intel.com>; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>; Simon Horman <horms@kernel.org>; Martyna
> Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
> Subject: [Intel-wired-lan] [iwl-net v3 3/5] ice: validate queue quanta parameters
> to prevent OOB access
> 
> From: Jan Glaza <jan.glaza@intel.com>
> 
> Add queue wraparound prevention in quanta configuration.
> Ensure end_qid does not overflow by validating start_qid and num_queues.
> 
> Fixes: 015307754a19 ("ice: Support VF queue rate limit and quanta size
> configuration")
> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Jan Glaza <jan.glaza@intel.com>
> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-
> mudlaw@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_virtchnl.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> index 343f2b4b0dc5..adb1bf12542f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> @@ -1903,13 +1903,21 @@ static int ice_vc_cfg_q_bw(struct ice_vf *vf, u8

Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>

RE: [Intel-wired-lan] [iwl-net v3 2/5] ice: stop truncating queue ids when checking

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Martyna Szapar-Mudlaw
> Sent: Tuesday, March 4, 2025 12:09 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Glaza, Jan <jan.glaza@intel.com>; Loktionov,
> Aleksandr <aleksandr.loktionov@intel.com>; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>; Simon Horman <horms@kernel.org>; Martyna
> Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
> Subject: [Intel-wired-lan] [iwl-net v3 2/5] ice: stop truncating queue ids when
> checking
> 
> From: Jan Glaza <jan.glaza@intel.com>
> 
> Queue IDs can be up to 4096, fix invalid check to stop truncating IDs to 8 bits.
> 
> Fixes: bf93bf791cec8 ("ice: introduce ice_virtchnl.c and ice_virtchnl.h")
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Jan Glaza <jan.glaza@intel.com>
> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-
> mudlaw@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_virtchnl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> index b6285433307c..343f2b4b0dc5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> @@ -565,7 +565,7 @@ bool ice_vc_isvalid_vsi_id(struct ice_vf *vf, u16 vsi_id)

Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>

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

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Szapar-Mudlaw, Martyna
> Sent: Tuesday, March 4, 2025 2:12 PM
> To: Paul Menzel <pmenzel@molgen.mpg.de>; Glaza, Jan <jan.glaza@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>; Simon Horman <horms@kernel.org>; Lobakin,
> Aleksander <aleksander.lobakin@intel.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@intel.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@intel.com>
> >>>> Reviewed-by: Simon Horman <horms@kernel.org>
> >>>> Signed-off-by: Jan Glaza <jan.glaza@intel.com>
> >>>> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-
> >>>> mudlaw@linux.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 {


Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>