Re: [PATCH rdma-next v4 01/10] RDMA/mana_ib: Allocate and map fast-registration MRs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rdma: missing bounds check on max_num_sg leading to integer overflow and OOB write
- [Low] rdma: hardware structure defined using compiler-dependent C bitfields
--

commit 7ef4d39c8200798999dc9e55ae57d45e1ec08621
Author: Konstantin Taranov <kotaranov@microsoft.com>

RDMA/mana_ib: Allocate and map fast-registration MRs

Implement alloc_mr and map_mr_sg for memory-registration MRs.

> diff --git a/drivers/infiniband/hw/mana/mana_ib.h b/drivers/infiniband/hw/mana/mana_ib.h
> index 9173ca64d2e1e..da9b48b422a1c 100644
> --- a/drivers/infiniband/hw/mana/mana_ib.h
> +++ b/drivers/infiniband/hw/mana/mana_ib.h

[ ... ]

> +#define MANA_FMR_MST_ALIGNMENT 64
> +
> +struct mana_fmr_mst_entry {
> +	u32 reserved1;
> +	u32 reserved2		: 8;
> +	u32 page_size		: 5;
> +	u32 reserved3		: 3;
> +	u32 access_right	: 8;
> +	u32 addr_type		: 2;
> +	u32 reserved4		: 6;
> +	u32 reserved5[2];
> +	u64 start_va;
> +	u64 length;
> +	u32 reserved6[8];
> +}; /* HW DATA */

[Severity: Low]
This structure defines a hardware interface but uses C bitfields. Are there
risks of compiler-dependent memory layout causing portability issues when used
for hardware ABI?

> +
> +static inline u32 mana_ib_fmr_header_offset(u32 num_ptes)
> +{
> +	/* The header follows the PTEs with the required MST alignment. */
> +	return ALIGN(sizeof(u64) * num_ptes, MANA_FMR_MST_ALIGNMENT);
> +}

[Severity: High]
Could sizeof(u64) * num_ptes produce a 64-bit value that is silently truncated
by the u32 return type when num_ptes is very large?

[ ... ]

> diff --git a/drivers/infiniband/hw/mana/mr.c b/drivers/infiniband/hw/mana/mr.c
> index 1233685b4b890..34dcc50d0be96 100644
> --- a/drivers/infiniband/hw/mana/mr.c
> +++ b/drivers/infiniband/hw/mana/mr.c

[ ... ]

> +struct ib_mr *mana_ib_alloc_mr(struct ib_pd *ibpd, enum ib_mr_type mr_type,
> +			       u32 max_num_sg)
> +{
> +	struct mana_ib_pd *pd = container_of(ibpd, struct mana_ib_pd, ibpd);
> +	struct gdma_create_mr_params mr_params = {};
> +	struct gdma_context *gc;
> +	struct mana_ib_dev *dev;
> +	struct mana_ib_mr *mr;
> +	int err;
> +
> +	dev = container_of(ibpd->device, struct mana_ib_dev, ib_dev);
> +	gc = mdev_to_gc(dev);
> +
> +	if (mr_type != IB_MR_TYPE_MEM_REG)
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	mr = kzalloc_obj(*mr);
> +	if (!mr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mr->fmr.max_ptes = max_num_sg;
> +	mr->fmr.size = mana_ib_fmr_buf_sz(max_num_sg);
> +	mr->fmr.ptes = dma_alloc_coherent(gc->dev, mr->fmr.size, &mr->fmr.dma_handle,
> +					  GFP_KERNEL);

[Severity: High]
Since max_num_sg is used without validation, is it possible for an excessively
large value to result in an undersized allocation for mr->fmr.ptes, given the
potential truncation in mana_ib_fmr_header_offset() called by
mana_ib_fmr_buf_sz()?

[ ... ]

> +static int mana_ib_set_page(struct ib_mr *ibmr, u64 addr)
> +{
> +	struct mana_ib_mr *mr = container_of(ibmr, struct mana_ib_mr, ibmr);
> +
> +	if (mr->fmr.pte_count >= mr->fmr.max_ptes)
> +		return -ENOMEM;
> +
> +	mr->fmr.ptes[mr->fmr.pte_count] = addr | MANA_PTE_VALID_MASK;
> +	mr->fmr.pte_count++;
> +
> +	return 0;
> +}

[Severity: High]
If mr->fmr.size was truncated to a small value, but mr->fmr.max_ptes retains
the original large max_num_sg value, would this bounds check be insufficient to
prevent out-of-bounds writes into memory past the allocated DMA buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010065443.3554193-1-kotaranov@linux.microsoft.com?part=1