Re: [PATCH net 13/13] iavf: validate num_vsis in VIRTCHNL_OP_GET_VF_RESOURCES response

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
iavf: validate num_vsis in VIRTCHNL_OP_GET_VF_RESOURCES response

This patch clamps the num_vsis field from the PF's
VIRTCHNL_OP_GET_VF_RESOURCES response so that the loop bound used when
iterating over the fixed-size vsi_res[] buffer cannot exceed what the
received message length can cover.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index ec234cc8bd9d..c4039d2b24a4 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c

[ ... ]

> @@ -300,7 +316,7 @@ int iavf_get_vf_config(struct iavf_adapter *adapter)
>  	 * we aren't getting too many queues
>  	 */
>  	if (!err)
> -		iavf_validate_num_queues(adapter);
> +		iavf_validate_num_queues(adapter, min(event.msg_len, len));
>  	iavf_vf_parse_hw_config(hw, adapter->vf_res);
>  
>  	kfree(event.msg_buf);

[High]

Does the error path in iavf_get_vf_config() still allow the
out-of-bounds access on vsi_res[] that the commit message describes?

Looking at the surrounding code:

    err = iavf_poll_virtchnl_msg(hw, &event, VIRTCHNL_OP_GET_VF_RESOURCES);
    memcpy(adapter->vf_res, event.msg_buf, min(event.msg_len, len));

    /* some PFs send more queues than we should have so validate that
     * we aren't getting too many queues
     */
    if (!err)
        iavf_validate_num_queues(adapter, min(event.msg_len, len));
    iavf_vf_parse_hw_config(hw, adapter->vf_res);

The memcpy of the PF-supplied payload into adapter->vf_res runs
unconditionally, but the new num_vsis clamp is only invoked when err is
zero.

iavf_poll_virtchnl_msg() returns virtchnl_status_to_errno(v_retval), so a
PF response carrying a non-success v_retval together with an
attacker-controlled msg_buf would still populate adapter->vf_res with the
raw num_vsis value and then skip the clamp.

iavf_vf_parse_hw_config() runs unconditionally after that and iterates
based on the unclamped u16 num_vsis:

    for (i = 0; i < msg->num_vsis; i++) {
        if (vsi_res->vsi_type == VIRTCHNL_VSI_SRIOV) {
            ether_addr_copy(hw->mac.perm_addr,
                    vsi_res->default_mac_addr);
            ether_addr_copy(hw->mac.addr,
                    vsi_res->default_mac_addr);
        }
        vsi_res++;
    }

vsi_res[] is a fixed-size array of IAVF_MAX_VF_VSI (3) entries in the
adapter->vf_res allocation, so num_vsis up to 65535 would walk far past
that buffer, and any OOB slot whose vsi_type happens to equal
VIRTCHNL_VSI_SRIOV would additionally copy six bytes of adjacent heap
memory into hw->mac.perm_addr and hw->mac.addr.

Would it make sense to either apply the num_vsis clamp unconditionally,
or gate the memcpy and iavf_vf_parse_hw_config() behind the same
if (!err) as the validation, so that the error path cannot reintroduce
the out-of-bounds access this patch is meant to close?