Re: [PATCH net-next v7 08/10] enic: add MBOX VF handlers for capability, register and link state

[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 VF handlers for capability, register and link state

This commit implements VF-side mailbox message processing for SR-IOV V2
admin channel communication, adding receive handlers for capability, register,
unregister replies, and link state notifications, along with initiation
functions for the probe-time handshake.

> diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
> index 9b1fa3857df5a..483053c781df7 100644
> --- a/drivers/net/ethernet/cisco/enic/enic.h
> +++ b/drivers/net/ethernet/cisco/enic/enic.h
[ ... ]
> @@ -305,9 +307,22 @@ struct enic {
>  	void (*admin_rq_handler)(struct enic *enic, void *buf,
>  				 unsigned int len);
>  
> -	/* MBOX protocol state */
> +	/* MBOX protocol state -- single-flight: on the VF, all callers
> +	 * that wait on mbox_comp run under RTNL or during probe/remove,
> +	 * so only one completion is outstanding at a time. mbox_lock
> +	 * protects the shared admin WQ from concurrent senders.
> +	 */
>  	struct mutex mbox_lock;
>  	u64 mbox_msg_num;
> +	struct completion mbox_comp;
> +	/* Type of reply the current waiter on mbox_comp expects.  Set
> +	 * under mbox_lock before reinit_completion(); cleared after
> +	 * wait_reply returns.  Reply handlers compare against the
> +	 * incoming reply type and drop stale replies from previously
> +	 * timed-out requests instead of waking the unrelated current
> +	 * waiter.
> +	 */
> +	u8 mbox_expected_reply;

[ ... ]
> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index 0f7f9d2d091f7..b555362379b35 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
[ ... ]
> +static void enic_mbox_vf_handle_link_state(struct enic *enic, void *payload)
> +{
> +	struct enic_mbox_pf_link_state_notif_msg *notif = payload;
> +	struct enic_mbox_pf_link_state_ack_msg ack = {};
> +
> +	switch (le32_to_cpu(notif->link_state)) {
[ ... ]
> +	default:
> +		netdev_warn(enic->netdev, "MBOX: unknown link state %u\n",
> +			    le32_to_cpu(notif->link_state));
> +		ack.ack.ret_major = cpu_to_le16(ENIC_MBOX_ERR_GENERIC);
> +		break;
> +	}
> +
> +	enic_mbox_send_msg(enic, ENIC_MBOX_PF_LINK_STATE_ACK, ENIC_MBOX_DST_PF,
> +			   &ack, sizeof(ack));
> +}

Could sending this ENIC_MBOX_PF_LINK_STATE_ACK with a non-zero ret_major error
code allow an unprivileged guest VM to flood the host kernel log?

Looking at the existing PF message handler enic_mbox_pf_process_msg(), when it
receives an ENIC_MBOX_PF_LINK_STATE_ACK with an error, it logs it like this:

    if (le16_to_cpu(ack->ack.ret_major))
        netdev_warn(enic->netdev,
                    "MBOX: VF %u link state ACK error %u/%u\n",
                    vf_id, le16_to_cpu(ack->ack.ret_major),
                    le16_to_cpu(ack->ack.ret_minor));

Since there is no rate limiting (like net_ratelimit()) on this specific
warning, a malicious guest could potentially craft and continuously send
invalid ACK messages to the PF's admin channel, spamming the hypervisor's log
and causing a denial of service.

[ ... ]
> +int enic_mbox_vf_capability_check(struct enic *enic)
> +{
> +	struct enic_mbox_vf_capability_msg req = {};
> +	int err;
> +
> +	enic->pf_cap_version = 0;
> +	enic->mbox_expected_reply = ENIC_MBOX_VF_CAPABILITY_REPLY;
> +	reinit_completion(&enic->mbox_comp);
> +	req.version = cpu_to_le32(ENIC_MBOX_CAP_VERSION_1);

Does this code miss acquiring mbox_lock before modifying mbox_expected_reply
and calling reinit_completion()?

The comment in enic.h dictates that mbox_expected_reply must be set under
mbox_lock before reinit_completion(), but here it seems they are updated
without holding the lock.

When a request times out, the waiter clears mbox_expected_reply = 0 without
locks. A concurrent RX handler processing a delayed reply might read the
expected type before it is cleared and proceed to call
complete(&enic->mbox_comp).

If the probe path proceeds to a cleanup step and calls
enic_mbox_vf_unregister(), could this delayed complete() race with the new
reinit_completion(), leading to a TOCTOU race where the completion is
spuriously signaled?

> +
> +	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_CAPABILITY_REQUEST,
> +				 ENIC_MBOX_DST_PF, &req, sizeof(req));
> +	if (err) {
> +		enic->mbox_expected_reply = 0;
> +		return err;
> +	}
[ ... ]
> +int enic_mbox_vf_register(struct enic *enic)
> +{
> +	int err;
> +
> +	enic->vf_registered = false;
> +	enic->mbox_expected_reply = ENIC_MBOX_VF_REGISTER_REPLY;
> +	reinit_completion(&enic->mbox_comp);

Is the same mbox_lock missing here as well?

> +
> +	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_REGISTER_REQUEST,
> +				 ENIC_MBOX_DST_PF, NULL, 0);
[ ... ]
> +int enic_mbox_vf_unregister(struct enic *enic)
> +{
> +	int err;
> +
> +	if (!enic->vf_registered)
> +		return 0;
> +
> +	enic->mbox_expected_reply = ENIC_MBOX_VF_UNREGISTER_REPLY;
> +	reinit_completion(&enic->mbox_comp);

And is it also missing here?

> +
> +	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_UNREGISTER_REQUEST,
> +				 ENIC_MBOX_DST_PF, NULL, 0);