Re: [PATCH net v2] octeontx2-af: cn10k: restrict VF LMTLINE sharing to its own PF

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
octeontx2-af: cn10k: restrict VF LMTLINE sharing to its own PF

This patch tries to plug a privilege boundary in the LMTLINE setup
mailbox by rejecting -EPERM when a VF caller's parent PF differs from
the parent PF of req->base_pcifunc. It carries Fixes: 893ae97214c3
and Cc: stable@vger.kernel.org.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cn10k.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cn10k.c
> index d2163da28d18..33f25e2fc262 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cn10k.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cn10k.c
> @@ -178,6 +178,14 @@ int rvu_mbox_handler_lmtst_tbl_setup(struct rvu *rvu,
>  	 * pcifunc (will be the one who is calling this mailbox).
>  	 */
>  	if (req->base_pcifunc) {
> +		/* A VF is untrusted and must not redirect its LMTLINE to
> +		 * another PF's region, so confine VF callers to their own PF.
> +		 */
> +		if (is_vf(req->hdr.pcifunc) &&
> +		    rvu_get_pf(rvu->pdev, req->hdr.pcifunc) !=
> +		    rvu_get_pf(rvu->pdev, req->base_pcifunc))
> +			return -EPERM;
> +

[Critical]
Can this check still be bypassed via the FUNC bits of base_pcifunc?

rvu_get_pf() only inspects the upper PF bits of the pcifunc (shift 10
on OTX2, shift 9 on CN20K), but RVU_PFVF_FUNC_MASK is 0x3FF, so the
FUNC field a VF can set in req->base_pcifunc spans 0..1023:

  static inline int rvu_get_pf(struct pci_dev *pdev, u16 pcifunc)
  {
          if (is_cn20k(pdev))
                  return (pcifunc >> RVU_CN20K_PFVF_PF_SHIFT) &
                          RVU_CN20K_PFVF_PF_MASK;
          else
                  return (pcifunc >> RVU_OTX2_PFVF_PF_SHIFT) &
                          RVU_OTX2_PFVF_PF_MASK;
  }

After the new check passes, base_pcifunc is fed straight into
rvu_get_lmtst_tbl_index(), where the index is computed from the full
FUNC field and LMT_MAX_VFS = 256:

  #define LMT_MAX_VFS             256

  static u32 rvu_get_lmtst_tbl_index(struct rvu *rvu, u16 pcifunc)
  {
          return ((rvu_get_pf(rvu->pdev, pcifunc) * LMT_MAX_VFS) +
                  (pcifunc & RVU_PFVF_FUNC_MASK)) * LMT_MAPTBL_ENTRY_SIZE;
  }

So a FUNC value >= 256 wraps into the next PF's slot range. Worked
example on OTX2: a VF on PF=0 (req->hdr.pcifunc = 0x0001) sets
req->base_pcifunc = 0x012C (300). is_vf() is true, and
rvu_get_pf(0x012C) = (300 >> 10) & PF_MASK = 0, equal to the caller's
PF, so the new -EPERM path is not taken.

rvu_get_lmtst_tbl_index() then yields (0 * 256 + 300) * 16 = 4800,
which is the slot for PF=1 FUNC=44 (1 * 256 + 44 = 300).

  pri_tbl_idx = rvu_get_lmtst_tbl_index(rvu, req->base_pcifunc);
  err = lmtst_map_table_ops(rvu, pri_tbl_idx, &val, LMT_TBL_OP_READ);
  ...
  err = rvu_update_lmtaddr(rvu, req->hdr.pcifunc, val);

That reads PF=1's physical LMT base address and copies it into the
attacker VF's own LMT map entry, which is the cross-PF redirection the
patch is intended to prevent.

The same wrap is reachable on CN20K, e.g. base_pcifunc = 511 lands in
PF=1 FUNC=255's slot since PF_SHIFT is 9 there.

Would it make sense to also reject base_pcifunc whose FUNC field is
out of range, for example by additionally requiring
(req->base_pcifunc & RVU_PFVF_FUNC_MASK) < LMT_MAX_VFS, or by looking
base_pcifunc up via rvu_get_pfvf() / is_pffunc_map_valid() and
confirming it is a real attached function under the same parent PF?