Re: [PATCH 10/15] yunsilicon: add RDMA PD/MR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] rdma/xsc: massive heap buffer overflow due to 32-bit truncation of npages
- [Critical] rdma/xsc: arbitrary physical memory exposure via unvalidated mmap size
- [High] rdma/xsc: MPT resource leak on DMA MR deregistration due to npages check
- [High] rdma/xsc: xsc_ib_get_dma_mr ignores ib_pd argument and hardcodes pdn to 0
- [High] drivers/infiniband: hw/xsc: unhandled xsc_reg_local_dma_mr failure
- [Medium] rdma/xsc: undefined behavior and zero-division on bitwise shifts exceeding 31 bits
--

commit 858863cb9a838a1ac3899ebcbed7d2f60b3b8241
Author: Xin Tian <tianx@yunsilicon.com>

yunsilicon: add RDMA PD/MR

This commit adds protection domains and memory regions for the yunsilicon
RDMA driver. It introduces PD allocation, kernel and user MR registration
paths, and DMA address translation helpers.

> diff --git a/drivers/infiniband/hw/xsc/main.c b/drivers/infiniband/hw/xsc/main.c
> --- a/drivers/infiniband/hw/xsc/main.c
> +++ b/drivers/infiniband/hw/xsc/main.c
> @@ -343,45 +343,45 @@ static int xsc_ib_mmap(struct ib_ucontext *ibcontext, struct vm_area_struct *vma
>  {
>  	struct xsc_ib_dev *dev = to_xdev(ibcontext->device);
>  	struct xsc_core_device *xdev = dev->xdev;
>  	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
>  	resource_size_t reg_base;
>  	resource_size_t reg_size = vma->vm_end - vma->vm_start;

[ ... ]

>  	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>  	return remap_pfn_range(vma, vma->vm_start, reg_base >> PAGE_SHIFT,
>  			       reg_size, vma->vm_page_prot);
>  }

[Severity: Critical]
Can this mapping size be safely used without validation?

In xsc_ib_mmap(), the requested mapping size is calculated entirely from
userspace input (reg_size = vma->vm_end - vma->vm_start). The function
checks the offset parameter to select a hardware base address, but it
doesn't validate whether reg_size exceeds the actual bounds of that
hardware region.

Passing this unvalidated reg_size directly to remap_pfn_range() could allow
userspace to map arbitrary adjacent physical memory or PCI MMIO regions.

> diff --git a/drivers/infiniband/hw/xsc/mem.c b/drivers/infiniband/hw/xsc/mem.c
> --- /dev/null
> +++ b/drivers/infiniband/hw/xsc/mem.c
> @@ -0,0 +1,161 @@

[ ... ]

> +static void __xsc_ib_cont_pages(struct ib_umem *umem, u64 addr,
> +				unsigned long max_page_shift,
> +				int *count, int *shift, int *ncont)
> +{

[ ... ]

> +		if (base + p != pfn) {
> +			/* If either the offset or the new
> +			 * base are unaligned update m
> +			 */
> +			tmp = (unsigned long)(pfn | p);
> +			if (!IS_ALIGNED(tmp, 1 << m))
> +				m = find_first_bit(&tmp, BITS_PER_LONG);
> +
> +			base = pfn;
> +			p = 0;
> +		}

[Severity: Medium]
Could this result in undefined behavior and a zero division?

If the DMA address is 0 (which is a valid IOMMU DMA address), tmp will be
0, causing find_first_bit() to return BITS_PER_LONG (64). The subsequent
shift of a 32-bit signed literal (1 << m) will be shifted by 64 bits.

This undefined value is later used as a divisor:
*ncont = DIV_ROUND_UP(i, (1 << m));

Similarly, in xsc_ib_get_buf_offset() lower in this file,
page_size = 1 << page_shift will overflow for 2GB hugepages
(page_shift == 31), incorrectly sign-extending the 64-bit page_mask.

> diff --git a/drivers/infiniband/hw/xsc/mr.c b/drivers/infiniband/hw/xsc/mr.c
> --- /dev/null
> +++ b/drivers/infiniband/hw/xsc/mr.c
> @@ -0,0 +1,313 @@

[ ... ]

> +struct ib_mr *xsc_ib_get_dma_mr(struct ib_pd *pd, int acc)
> +{

[ ... ]

> +	req = &in->req;
> +	req->acc = convert_access(acc);
> +	req->va_base = 0;
> +	req->map_en = !(XSC_MPT_MAP_EN);
> +
> +	err = xsc_core_create_mkey(xdev, &mr->xmr);

[Severity: High]
Does this code assign the protection domain to the DMA MR?

The req structure is zero-initialized via kzalloc_obj(), so req->pdn
defaults to 0. The ib_pd argument provided by the caller doesn't appear
to be assigned to req->pdn before calling xsc_core_register_mr().

This could cause the hardware to register the MR under PD 0, which might
break kernel components that allocate QPs in their own PDs and rely on
this DMA MR.

[ ... ]

> +struct ib_mr *xsc_ib_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
> +				 u64 virt_addr, int access_flags,
> +				 struct ib_dmah *dmah,
> +				 struct ib_udata *udata)
> +{

[ ... ]

> +	pgsz = ib_umem_find_best_pgsz(umem, XSC_MR_PAGE_CAP_MASK, virt_addr);
> +	if (!pgsz) {
> +		ibdev_warn(&dev->ib_dev, "find best page size failed\n");
> +		err = -EINVAL;
> +		goto error;
> +	}
> +
> +	page_shift = order_base_2(pgsz);
> +	npages = ib_umem_num_dma_blocks(umem, pgsz);

[Severity: Critical]
Does this code truncate the block count?

ib_umem_num_dma_blocks() returns a 64-bit size_t block count, which is
being assigned to a 32-bit signed int npages.

For large memory regions, truncating this value causes the subsequent
buffer allocation in reg_create() to be undersized:

inlen = sizeof(*in) + sizeof(*in->req.pas) * npages;
in = xsc_vzalloc(inlen);

Then rdma_umem_for_each_dma_block() will execute based on the actual umem
blocks, writing 64-bit physical addresses far out of the allocated bounds.
Could this lead to a heap buffer overflow?

[ ... ]

> +int xsc_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
> +{

[ ... ]

> +	ibdev_dbg(&dev->ib_dev, "dereg mkey = 0x%x\n", mr->xmr.key);
> +
> +	if (mr->npages) {
> +		err = xsc_core_dereg_mr(dev->xdev, &mr->xmr);

[Severity: High]
Does this skip firmware deregistration for local DMA MRs?

xsc_ib_get_dma_mr() initializes a local DMA MR with req->pa_num = 0, which
results in mr->npages = 0, but it successfully allocates and configures an
MPT entry in hardware.

When deregistering, checking if (mr->npages) bypasses the
XSC_CMD_OP_DEREG_MR teardown command for these MRs. Will this leak MPT
resources for every DMA MR created and destroyed?

[ ... ]

> +void xsc_reg_local_dma_mr(struct xsc_core_device *dev)
> +{

[ ... ]

> +	err = xsc_core_register_mr(dev, NULL, &in, sizeof(in));
> +	if (err)
> +		dev_err(&dev->pdev->dev, "\n");
> +}

[Severity: High]
Is it safe to ignore this MR registration failure?

If xsc_core_register_mr() fails, the error is caught and printed, but the
function returns void without propagating the failure. The caller (init_one)
will then proceed to register the IB device and advertise
local_dma_lkey = 0xFF.

If components attempt to use this unregistered MKEY for DMA, won't the
hardware generate memory protection faults?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012114352.2471317-1-tianx@yunsilicon.com?part=10