Re: [PATCH net v2] octeontx2-af: Fix BPID leak in nix_bp_enable()

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
octeontx2-af: Fix BPID leak in nix_bp_enable()

For LBK interfaces rvu_nix_get_bpid() takes a BPID from the free pool on
every call, and nix_bp_enable() used to call it once before the loop and
once more after the last channel was programmed. The patch moves the
allocation into the loop body and rejects req->chan_cnt == 0 upfront.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> index 153eb57bad067..3a43432d29c15 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> @@ -764,19 +764,25 @@ static int nix_bp_enable(struct rvu *rvu,
>  	if (cpt_link && !rvu->hw->cpt_links)
>  		return 0;
>  
> +	if (!req->chan_cnt)
> +		return NIX_AF_ERR_INVALID_BPID_REQ;

[Severity: High]
This isn't a bug introduced by this patch, but since the change adds request
validation here, is the companion bound on req->chan_base / req->chan_cnt
also needed for LBK?

rvu_nix_get_bpid() range-checks the channel interval only for the CGX and
SDP cases:

	case NIX_INTF_TYPE_CGX:
		if ((req->chan_base + req->chan_cnt) > NIX_BPIDS_PER_LMAC)
			return NIX_AF_ERR_INVALID_BPID_REQ;
	...
	case NIX_INTF_TYPE_SDP:
		if ((req->chan_base + req->chan_cnt) > bp->sdp_bpid_cnt)
			return NIX_AF_ERR_INVALID_BPID_REQ;

The NIX_INTF_TYPE_LBK case has no such check, and nix_interface_init() gives
an LBK VF exactly one RX channel (pfvf->rx_chan_cnt = 1), while req->chan_base
is a u16 copied straight out of the mailbox region.

So chan_base = pfvf->rx_chan_base + req->chan_base can be any 16-bit value,
and each iteration does a read-modify-write of the AF-global register
NIX_AF_RX_CHANX_CFG(chan_v), which expands to 0x1A30 | (a) << 15. With
req->chan_base near 0xFFF0 the offset reaches roughly 0x7FFF8000, well past
the 1 << 28 per-block stride assumed by rvu_write64():

	writeq(val, rvu->afreg_base + ((block << 28) | offset));

Can an LBK VF therefore reprogram the backpressure configuration of channels
owned by other PF/VFs (arming them against a foreign BPID with BIT_ULL(16)
set), or push the access outside the intended NIX register window?

>  	pfvf = rvu_get_pfvf(rvu, pcifunc);
>  	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NIX, pcifunc);
>  
> -	bpid_base = rvu_nix_get_bpid(rvu, req, type, chan_id);
>  	chan_base = pfvf->rx_chan_base + req->chan_base;
> -	bpid = bpid_base;
> +	bpid_base = -1;
>  
>  	for (chan = chan_base; chan < (chan_base + req->chan_cnt); chan++) {
> +		bpid = rvu_nix_get_bpid(rvu, req, type, chan_id);
>  		if (bpid < 0) {
>  			dev_warn(rvu->dev, "Fail to enable backpressure\n");
>  			return -EINVAL;
>  		}

[Severity: High]
Now that the allocation happens per iteration, does this bare return leak the
BPIDs already taken for the earlier channels?

For LBK, each call consumes one entry of the finite free pool and records
ownership:

	case NIX_INTF_TYPE_LBK:
		/* Alloc bpid from the free pool */
		mutex_lock(&rvu->rsrc_lock);
		bpid = rvu_alloc_rsrc(&bp->bpids);
		if (bpid < 0) {
			mutex_unlock(&rvu->rsrc_lock);
			return NIX_AF_ERR_INVALID_BPID;
		}
		bp->fn_map[bpid] = req->hdr.pcifunc;
		bp->ref_cnt[bpid]++;

A request with req->chan_cnt larger than the number of free pool entries
fails deterministically once rvu_alloc_rsrc() returns -ENOSPC. At that point
iterations 0..i-1 have already allocated BPIDs and written them into
NIX_AF_RX_CHANX_CFG with the enable bit set, and this path returns without
freeing them or clearing the enable bit.

The response is also left zeroed (otx2_mbox_alloc_msg_rsp() memsets the msg
region and rsp->chan_cnt is only set at the end of the function), so the
requester learns no channel/BPID mapping and will not send a matching
NIX_BP_DISABLE. Does that leave the BPIDs marked used with fn_map/ref_cnt
set until FLR, and the affected RX channels armed against BPIDs that no aura
maps?

Would an unwind loop that frees the BPIDs and clears BIT_ULL(16) for the
channels programmed so far be appropriate here?

>  
> +		if (bpid_base < 0)
> +			bpid_base = bpid;
> +

[Severity: Medium]
Only the first allocation is remembered here, but the response is built from
it arithmetically:

	for (chan = 0; chan < req->chan_cnt; chan++) {
		/* Map channel and bpid assign to it */
		rsp->chan_bpid[chan] = ((req->chan_base + chan) & 0x7F) << 10 |
					(bpid_base & 0x3FF);
		if (req->bpid_per_chan)
			bpid_base++;
	}

Does this diverge from what was actually programmed for LBK?

The mailbox contract in mbox.h says:

	u8	bpid_per_chan;
	/* bpid_per_chan = 0 assigns single bp id for range of channels */
	/* bpid_per_chan = 1 assigns separate bp id for each channel */

The LBK branch of rvu_nix_get_bpid() ignores req->bpid_per_chan and always
allocates a fresh pool entry. With bpid_per_chan == 0 and chan_cnt > 1, each
NIX_AF_RX_CHANX_CFG gets a different BPID while rsp->chan_bpid[] reports
bpid_base for every channel. The requester then wires up only bpid_base:

drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c:mbox_handler_nix_bp_enable() {
	for (chan = 0; chan < rsp->chan_cnt; chan++) {
		chan_id = ((rsp->chan_bpid[chan] >> 10) & 0x7F);
		pfvf->bpid[chan_id] = rsp->chan_bpid[chan] & 0x3FF;
	}
}

With bpid_per_chan == 1 the response assumes bpid_base, bpid_base + 1, ...,
but rvu_alloc_rsrc() returns the first free bit of a shared bitmap:

	id = find_first_zero_bit(rsrc->bmap, rsrc->max);
	if (id >= rsrc->max)
		return -ENOSPC;
	__set_bit(id, rsrc->bmap);

and rvu_nix_get_bpid() takes and drops rvu->rsrc_lock once per channel, so a
fragmented pool or an interleaved allocation by another LBK function gives
non-contiguous BPIDs. Can the reported map then name a BPID owned by a
different pcifunc?

For bpid_per_chan == 0, would allocating once per request (and reporting the
BPIDs actually written) match the documented behaviour better than "exactly
once per channel actually programmed"?

>  		chan_v = nix_get_channel(chan, cpt_link);
>  
>  		cfg = rvu_read64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v));
> @@ -784,7 +790,6 @@ static int nix_bp_enable(struct rvu *rvu,
>  		rvu_write64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v),
>  			    cfg | (bpid & GENMASK_ULL(8, 0)) | BIT_ULL(16));
>  		chan_id++;
> -		bpid = rvu_nix_get_bpid(rvu, req, type, chan_id);
>  	}
>  
>  	for (chan = 0; chan < req->chan_cnt; chan++) {

[Severity: Critical]
This is a pre-existing issue, but it interacts with the new mid-loop error
return above, so it seems worth raising here.

nix_bp_disable() recovers the BPID to release from the channel register:

	if (type == NIX_INTF_TYPE_LBK) {
		bpid = cfg & GENMASK(8, 0);
		mutex_lock(&rvu->rsrc_lock);
		rvu_free_rsrc(&bp->bpids, bpid - bp->free_pool_base);

nix_setup_bpids() makes free_pool_base strictly positive:

	bp->free_pool_base = bp->cgx_bpid_cnt + bp->sdp_bpid_cnt +
			     NIX_BPIDS_PER_CPT;

So for any channel whose CFG BPID field is below free_pool_base - the reset
value 0 on a channel that was never enabled, the tail channels left
unprogrammed by the new error return in nix_bp_enable(), or a CGX-range BPID
belonging to another function - the second argument is negative.

rvu_free_rsrc() does not check it:

	void rvu_free_rsrc(struct rsrc_bmap *rsrc, int id)
	{
		if (!rsrc->bmap)
			return;

		__clear_bit(id, rsrc->bmap);
	}

and generic___clear_bit() takes an unsigned long:

	unsigned long mask = BIT_MASK(nr);
	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);

	*p &= ~mask;

Can this write outside bp->bpids.bmap? Since nothing bounds chan_base /
chan_cnt for LBK, a VF sending NIX_BP_DISABLE over channels it never enabled
appears to reach this with bpid == 0. Should nix_bp_disable() validate that
the register-derived BPID falls inside the free pool range before converting
it to a pool index?
-- 
This is an AI-generated review.