Re: [PATCH net-next v12 05/12] enic: add admin CQ service with MSI-X interrupt and workqueue polling

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Now I have the confirmation. Let me generate the report.

commit 91d5187517ef604e2669cd4fde3a2c606489afe1
Author: Satish Kharat [off-list ref]

enic: add admin CQ service with MSI-X interrupt and workqueue polling

This adds an MSI-X driven CQ service for the admin channel with a
workqueue-based poller, a bounded admin_msg backlog, and a deferred
dispatch work_struct for received admin messages.

[ ... ]

[ ... ]

+unsigned int enic_admin_rq_cq_service(struct enic *enic)
+{
+	struct vnic_cq *cq = &enic->admin_cq[1];
+	struct vnic_rq *rq = &enic->admin_rq;
+	struct cq_enet_rq_desc *rq_desc;
+	struct vnic_rq_buf *buf;
+	u16 bwf, bytes_written;
+	unsigned int work = 0;
+	void *desc;
+

[ ... ]

+		rq_desc = desc;
+		bwf = le16_to_cpu(rq_desc->bytes_written_flags);
+		bytes_written = bwf & CQ_ENET_RQ_DESC_BYTES_WRITTEN_MASK;
+		if (bytes_written > buf->len)
+			goto next_desc;
+
+		dma_sync_single_for_cpu(&enic->pdev->dev,
+					buf->dma_addr, buf->len,
+					DMA_FROM_DEVICE);

[ ... ]

+		enic_admin_msg_enqueue(enic, buf->os_buf, bytes_written);

[Medium]
Can this path leak uninitialised heap contents to downstream admin
message consumers?

The admin RQ buffers are allocated with plain kmalloc() in
enic_admin_rq_post_one():

    buf = kmalloc(ENIC_ADMIN_BUF_SIZE, gfp);

so the buffer contents are uninitialised heap.  The only bound applied
here is bytes_written > buf->len, but there is no check that hardware
actually DMA'd bytes_written bytes.  enic_admin_msg_enqueue() then does:

    memcpy(msg->data, buf, len);

A firmware or peer that DMAs a short payload but reports a larger
bytes_written value would cause the tail region to be copied verbatim
from stale heap into the queued admin_msg.

The commit comment above this block already flags a "buggy or hostile
VF" as part of the threat model.  Would switching enic_admin_rq_post_one()
to kzalloc(), or zeroing [bytes_written, buf->len) after
dma_sync_single_for_cpu() and before enqueue, be preferable?

The admin_rq_handler is NULL at this point in the series, but later
patches ("enic: add MBOX PF handlers for VF register and capability"
and "enic: add MBOX VF handlers for capability, register and link
state") process payload bytes, so any stale content would then reach
handler logic.

[ ... ]

+static void enic_admin_poll_work_handler(struct work_struct *work)
+{
+	struct enic *enic = container_of(work, struct enic, admin_poll_work);
+	unsigned int credits;
+	unsigned int rq_work;
+
+	/* Snapshot the pending credit count before draining so we acknowledge
+	 * exactly what the hardware reported for this interrupt.  Credits that
+	 * accrue while enic_admin_rq_cq_service() runs are left for the next
+	 * interrupt, which is harmless on this low-rate control path.
+	 */
+	credits = vnic_intr_credits(&enic->admin_intr);
+
+	rq_work = enic_admin_rq_cq_service(enic);
+
+	if (rq_work > 0)
+		schedule_work(&enic->admin_msg_work);
+
+	/* Acknowledge the snapshotted credits and unmask the vector.  Unlike
+	 * the NAPI data path, the admin channel is not re-polled, so the vector
+	 * must be re-armed here to receive the next completion.  The unmask is
+	 * applied through the interrupt mask register independently of the
+	 * credit count, so returning zero credits on a spurious wakeup still
+	 * re-arms the vector.
+	 */
+	vnic_intr_return_credits(&enic->admin_intr,
+				 credits,
+				 1 /* unmask */, 0);
+}

[Low]
The commit message states:

    At least one credit is returned so the write always
    carries a non-zero acknowledgment and re-arms the vector, since the
    admin channel is not re-polled like the NAPI data path.

Does the code actually enforce a floor of one credit?

credits comes directly from vnic_intr_credits() and is passed verbatim
to vnic_intr_return_credits() with no max(credits, 1U) clamp.  On a
spurious wakeup credits can be zero.  The in-function comment right
below correctly notes:

    returning zero credits on a spurious wakeup still
    re-arms the vector.

Should the commit message be reworded to match the actual behaviour
(the UNMASK bit re-arms the vector independently of the credit count),
or should a max(credits, 1U) be added to match the message?

[ ... ]
-- 
pw-bot: cr