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

[Nitin Shetty J <nshettyj@marvell.com>]

On 2026-09-15 at 18:47:14, Paolo Abeni (pabeni@redhat.com) wrote:
> 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?
> 
Since it is exisiting issue will submit separate patch.

> >  	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?
> 
will fix it in next patch version

> >  
> > +		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"?
> 
will fix it in next patch version

> >  		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?
will fix it in next patch version

> -- 
> This is an AI-generated review.
> 
>