RE: [PATCH v3 net-next 03/15] net: enetc: add link status message support to PF driver

quoted

+static int enetc_pf_send_msg(struct enetc_pf *pf, u32 msg_code, u16

ms_mask)

quoted

+{
+	struct enetc_hw *hw = &pf->si->hw;
+	u16 old_ms_mask = ms_mask;
+	u16 ms_status;
+	u32 val;
+
+	/* The MS bit is set, indicating that the corresponding VF has not
+	 * read the last message, PF cannot send new message to the VF. To
+	 * avoid sending messages to such a VF, the bit corresponding to VF
+	 * is cleared from ms_mask. Because the MS bit can only be written
+	 * as 1, writing a 0 has no effect. Writing a 1 when the bit is
+	 * already set is undefined.
+	 */
+	ms_status = enetc_rd(hw, ENETC_PSIMSGSR) & 0xffff;
+	ms_mask &= ~ms_status;
+	if (!ms_mask)
+		return -EIO;

[Severity: High]
Does this logic introduce a time-of-check to time-of-use race condition?

If a VF acknowledges a previous message by clearing its MS bit immediately
after ms_status is read, the mask will remain zero and this early return
will prevent the link state from being updated.

Because the early return prevents the shared MC payload from being updated
in hardware, and there is no retry logic, will the VF receive the old link
state when it eventually reads the register and be left permanently out of
sync with the PF?

This issue stems from the hardware limitation, the software cannot guarantee
that the hardware won't clear the MS bit after the check is complete. However,
this problem is highly unlikely to occur, as mailbox messages are infrequent, and
VF has ample time to process them. If this situation does arise, it indicates that
VF itself is already malfunctioning, making retries pointless. Therefore, I believe
that adding a retry mechanism to address this theoretical issue is unnecessary,
as it would only complicate the code logic and introduce new problems.

quoted

+
+	if (ms_mask != old_ms_mask)
+		dev_warn_ratelimited(&pf->si->pdev->dev,
+				     "PF cannot send message to VF(s) 0x%x\n",
+				     ms_mask ^ old_ms_mask);
+
+	enetc_wr(hw, ENETC_PSIMSGSR,
+		 FIELD_PREP(PSIMSGSR_MC, msg_code) | ms_mask);
+
+	return read_poll_timeout(enetc_rd, val, !(val & ms_mask), 1000,
+				 200000, false, hw, ENETC_PSIMSGSR);

[Severity: High]
Can an untrusted VF use this synchronous polling to intentionally stall the
PF's execution thread?

This blocks for up to 200ms waiting for the VF to acknowledge the message.
Since this is reachable by untrusted VFs (e.g., running in a VM), can a
malicious VF ignore the message and tie up the PF thread?

The `enetc_pf_send_msg()` function is currently only used to notify link
status, and messages only occur when the link status changes, making its
frequency extremely low. Even if new messages are added in the future,
they will still be very infrequent. I don't believe that malicious VF
intentionally blocking for 200ms would have any impact, so there's nothing
to fix.

[ ... ]

quoted

+static void enetc_msg_register_link_status_notifier(struct enetc_pf *pf,
+						    int vf_id)
+{
+	u16 pf_msg = FIELD_PREP(ENETC_PF_MSG_CLASS_ID,
+				ENETC_MSG_CLASS_ID_CMD_SUCCESS);
+
+	mutex_lock(&pf->msg_lock);
+
+	enetc_pf_reply_msg(&pf->si->hw, vf_id, pf_msg);
+
+	/* SR-IOV is being disabled if pf->sriov_enabled is false, so no
+	 * need to set link_status_ms_mask and notify the link status.
+	 */
+	if (!pf->sriov_enabled)
+		goto msg_unlock;
+
+	pf->link_status_ms_mask |= PSIMSGSR_MS(vf_id);
+
+	/* Notify VF the current link status */
+	enetc_msg_notify_vf_link_status(pf, PSIMSGSR_MS(vf_id));

[Severity: High]
Does calling enetc_msg_notify_vf_link_status() here block other mailbox
operations?

Because enetc_msg_notify_vf_link_status() eventually calls
enetc_pf_send_msg() which can poll for 200ms, and we are holding
pf->msg_lock, will a slow or malicious VF block all other VF mailbox
operations and link state processing for this PF?

Mailbox messages are infrequent, and link status messages are only
generated when the link status changes. Therefore, this theoretical
problem is not worth fixing. Even if it does occur, it won't cause any
issues other VFs will simply experience timeout errors.