Re: [PATCH rdma v8 7/7] RDMA/zrdma: Implement context, PD, and mmap operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] RDMA/zrdma: missing udata validation and response zeroing in ucontext and PD ops
- [Low] RDMA/zrdma: unratelimited pr_err() on user-triggerable paths in zxdh_alloc_ucontext() and zxdh_mmap()
--

commit 34250bc80391a0c4ba1beb61faa1ec946d1b1b77
Author: Yanze Zhang <zhang.yanze@zte.com.cn>

RDMA/zrdma: Implement context, PD, and mmap operations

This commit implements protection domain allocation and deallocation, user
context management, and doorbell memory mapping operations for the zrdma
driver.

> diff --git a/drivers/infiniband/hw/zrdma/zrdma_verbs.c b/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> index 821ccbc2054ea..239d607f7d182 100644
> --- a/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> +++ b/drivers/infiniband/hw/zrdma/zrdma_verbs.c
[ ... ]
> @@ -392,15 +393,343 @@ int zxdh_ib_register_device(struct zxdh_device *zdev)
>  	return ret;
>  }
>  
> +static void zxdh_sc_pd_init(struct zxdh_sc_dev *dev, struct zxdh_sc_pd *pd,
> +			    u32 pd_id, int abi_ver)
> +{
> +	pd->pd_id = pd_id;
> +	pd->abi_ver = abi_ver;
> +	pd->dev = dev;
> +}
> +
> +static int zxdh_alloc_pd(struct ib_pd *pd, struct ib_udata *udata)
> +{
> +	struct zxdh_device *zdev = to_zdev(pd->device);
> +	struct zxdh_sc_dev *dev = &zdev->rf->sc_dev;
> +	struct zxdh_alloc_pd_resp uresp = {};
> +	struct zxdh_pd *zpd = to_zpd(pd);
> +	struct zxdh_pci_f *rf = zdev->rf;
> +	struct zxdh_sc_pd *sc_pd;
> +	u32 pd_id;
> +	int err;
> +
> +	/* The user has to provide a buffer for the whole response. A short
> +	 * buffer is reported instead of being silently truncated, and no
> +	 * resource is taken for it.
> +	 */
> +	if (udata && udata->outlen < sizeof(uresp))
> +		return -ENOSPC;
> +
> +	err = zxdh_alloc_rsrc(rf, rf->allocated_pds, rf->max_pd, &pd_id,
> +			      &rf->next_pd, "PD");
> +	if (err) {
> +		pr_warn_ratelimited("zrdma: PD allocation failed err=%d\n",
> +				    err);
> +		return err;
> +	}
> +
> +	sc_pd = &zpd->sc_pd;
> +	if (udata) {
> +		struct zxdh_ucontext *ucontext;
> +
> +		ucontext = rdma_udata_to_drv_context(udata,
> +						     struct zxdh_ucontext,
> +						     ibucontext);
> +
> +		zxdh_sc_pd_init(dev, sc_pd, pd_id, ucontext->abi_ver);
> +		uresp.pd_id = pd_id;
> +		if (ib_copy_to_udata(udata, &uresp, sizeof(uresp))) {
> +			err = -EFAULT;
> +			goto error;
> +		}
[ ... ]
> +static int zxdh_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
> +{
> +	struct zxdh_ucontext *ucontext = to_ucontext(uctx);
> +	struct zxdh_alloc_ucontext_resp uresp = {};
> +	struct ib_device *ibdev = uctx->device;
> +	struct zxdh_device *zdev = to_zdev(ibdev);
> +	struct zxdh_alloc_ucontext_req req = {};
> +	u64 srq_db_mmap_size;
> +
> +	if (ib_copy_from_udata(&req, udata, min(sizeof(req), udata->inlen)))
> +		return -EINVAL;
> +
> +	if (req.userspace_ver != ZXDH_CONTEXT_VER_V1 &&
> +	    req.userspace_ver != ZXDH_CONTEXT_VER_V2 &&
> +	    req.userspace_ver != ZXDH_CONTEXT_VER_V3) {
> +		pr_err("zrdma: Invalid userspace_ver%d\n", req.userspace_ver);
> +		return -EINVAL;
> +	}
> +
> +	/* The response carries the keys of the doorbell mappings, so the
> +	 * user has to provide a buffer for the whole of it. A short buffer
> +	 * is reported instead of being silently truncated.
> +	 */
> +	if (udata->outlen < sizeof(uresp))
> +		return -ENOSPC;
[ ... ]
> +	if (ib_copy_to_udata(udata, &uresp, sizeof(uresp))) {
> +		rdma_user_mmap_entry_remove(ucontext->sq_db_mmap_entry);
> +		ucontext->sq_db_mmap_entry = NULL;
> +		rdma_user_mmap_entry_remove(ucontext->cq_db_mmap_entry);
> +		ucontext->cq_db_mmap_entry = NULL;
> +		rdma_user_mmap_entry_remove(ucontext->srq_db_mmap_entry);
> +		ucontext->srq_db_mmap_entry = NULL;
> +		return -EFAULT;
> +	}
[ ... ]
> +static int zxdh_dealloc_pd(struct ib_pd *ibpd, struct ib_udata *udata)
> +{
> +	struct zxdh_pd *zpd = to_zpd(ibpd);
> +	struct zxdh_device *zdev = to_zdev(ibpd->device);
> +
> +	zxdh_free_rsrc(zdev->rf, zdev->rf->allocated_pds, zpd->sc_pd.pd_id);
> +	return 0;
> +}

[Severity: Medium]
Should the ucontext and PD operations in drivers/infiniband/hw/zrdma/zrdma_verbs.c
validate udata input and zero-fill trailing response bytes to preserve UAPI
forward compatibility?

In zxdh_alloc_ucontext(), copying req via ib_copy_from_udata() with
min(sizeof(req), udata->inlen) and returning -EINVAL masks -EFAULT on bad user
pointers, accepts truncated input buffers smaller than sizeof(req), and
silently ignores non-zero trailing bytes when udata->inlen > sizeof(req).

Additionally, struct zxdh_alloc_ucontext_req in include/uapi/rdma/zrdma_abi.h
defines reserved fields:

struct zxdh_alloc_ucontext_req {
	__u32 rsvd32;
	/* Version of the user space, one of the ZXDH_CONTEXT_VER_*. */
	__u8 userspace_ver;
	__u8 rsvd8[3];
};

However, zxdh_alloc_ucontext() does not check that req.rsvd32 and req.rsvd8
are zeroed. Using ib_copy_validate_udata_in(udata, req, rsvd8) and checking
req.rsvd32 and req.rsvd8 for zero would ensure these fields remain usable for
future extensions.

Similarly, zxdh_alloc_pd() does not check ib_is_udata_in_empty(udata) when
udata is present, zxdh_dealloc_pd() does not check ib_no_udata_io(udata)
before freeing the PD, and both zxdh_alloc_pd() and zxdh_alloc_ucontext() use
ib_copy_to_udata(udata, &uresp, sizeof(uresp)) instead of
ib_respond_udata(udata, uresp), leaving trailing bytes in udata->outbuf
unzeroed when udata->outlen > sizeof(uresp).

[ ... ]
> +static int zxdh_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
> +{
> +	struct rdma_user_mmap_entry *rdma_entry;
> +	struct zxdh_user_mmap_entry *entry;
> +	struct zxdh_ucontext *ucontext;
> +	u64 pfn;
> +	int ret;
> +
> +	ucontext = to_ucontext(context);
> +
> +	rdma_entry = rdma_user_mmap_entry_get(&ucontext->ibucontext, vma);
> +	if (!rdma_entry) {
> +		pr_err("zrdma: pgoff[0x%lx] does not have valid entry\n",
> +		       vma->vm_pgoff);
> +		return -EINVAL;
> +	}

[Severity: Low]
Can unprivileged users flood the kernel log via these unratelimited pr_err()
calls in drivers/infiniband/hw/zrdma/zrdma_verbs.c?

In zxdh_alloc_ucontext(), passing an unsupported req.userspace_ver value
triggers an unratelimited pr_err(). In zxdh_mmap(), calling mmap() on the
uverbs file descriptor with an invalid page offset, a mismatched VMA length,
or without VM_SHARED causes rdma_user_mmap_entry_get() to return NULL and also
triggers an unratelimited pr_err().

Would ibdev_dbg() or ratelimited logging be more appropriate for these
user-triggerable error paths?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102082026.2645777-1-zhang.yanze@zte.com.cn?part=7