Re: [PATCH net-next v4 07/10] enic: add MBOX PF handlers for VF register and capability

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
enic: add MBOX PF handlers for VF register and capability

Implement PF-side mailbox message processing for SR-IOV V2 admin channel
communication. It processes capability, register, and unregister requests
from VFs.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index d144c86d9ef84..f5784624ebbdf 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
[ ... ]
> @@ -124,10 +124,168 @@ int enic_mbox_send_msg(struct enic *enic, u8 msg_type, u16 dst_vnic_id,
[ ... ]
> +static int enic_mbox_pf_handle_unregister(struct enic *enic, void *msg,
> +					  u16 vf_id, u64 msg_num)
> +{
> +	struct enic_mbox_vf_register_reply_msg reply = {};
> +	int err;
> +
> +	if (!enic->vf_state || vf_id >= enic->num_vfs) {
> +		netdev_warn(enic->netdev,
> +			    "MBOX: unregister from invalid VF %u\n", vf_id);
> +		return -EINVAL;
> +	}
> +
> +	reply.reply.ret_major = cpu_to_le16(0);
> +	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_UNREGISTER_REPLY, vf_id,
> +				 &reply, sizeof(reply));
> +	if (err)
> +		return err;
> +
> +	enic->vf_state[vf_id].registered = false;

If enic_mbox_send_msg() fails (for example, due to a full admin WQ or channel
timeout), the function returns early and the PF misses setting the registered
state to false.

Since an unregister request indicates the VF driver is unloading or shutting
down, should the PF unconditionally clear the registration state regardless of
whether the acknowledgment successfully reaches the defunct VF?

If the local state is not cleared, could the PF assume the VF is still active
and continue attempting to send it unnecessary link state notifications?

[ ... ]
> +static void enic_mbox_pf_process_msg(struct enic *enic,
> +				     struct enic_mbox_hdr *hdr, void *payload)
> +{
> +	u16 vf_id = le16_to_cpu(hdr->src_vnic_id);
> +	u16 msg_len = le16_to_cpu(hdr->msg_len);
> +	int err = 0;
> +
> +	if (!enic->vf_state) {
> +		netdev_dbg(enic->netdev,
> +			   "MBOX: PF received msg but SRIOV not active\n");
> +		return;
> +	}
> +
> +	if (vf_id >= enic->num_vfs) {
> +		netdev_warn(enic->netdev,
> +			    "MBOX: PF received msg from invalid VF %u\n",
> +			    vf_id);
> +		return;
> +	}

Could a malicious or buggy guest VM use this untrusted VF input to cause a
host denial of service through log spamming?

Because this warning is not rate-limited, an untrusted VF could continuously
flood the PF's admin channel with malformed requests. This might exhaust host
CPU cycles and cause dmesg lock contention.

Should these logging calls in the message parsing path use
netdev_warn_ratelimited() instead?

[ ... ]
> @@ -142,10 +300,22 @@ static void enic_mbox_recv_handler(struct enic *enic, void *buf,
>  		return;
>  	}
>  
> +	msg_len = le16_to_cpu(hdr->msg_len);
> +	if (msg_len < sizeof(*hdr) || msg_len > len) {
> +		netdev_warn(enic->netdev,
> +			    "MBOX: invalid msg_len %u (buf len %u)\n",
> +			    msg_len, len);
> +		return;
> +	}

Similarly here, does this log also need rate limiting to protect the host
from being spammed by malformed headers sent from a VF?