Re: [PATCH] accel/amdxdna: Bound the page count of a user supplied buffer

Re: [PATCH] accel/amdxdna: Bound the page count of a user supplied buffer

[Lizhi Hou]

On 8/26/26 12:57, Taimuraz Kaitmazov wrote:
> amdxdna_get_ubuf() puts a per-entry page count derived from a __u64
> va_ent[i].len into a u32, then passes it to pin_user_pages_fast(), whose
> nr_pages is an int. An entry of 2^44 bytes truncates npages to zero, so
> nothing is pinned, the ret != npages test still passes, and ubuf->pages
> keeps whatever kvmalloc_objs() returned while ubuf->nr_pages describes the
> untruncated count. amdxdna_ubuf_release() then walks all of it. Reaching
> that needs CAP_IPC_LOCK and a multi-gigabyte kvmalloc() to succeed.
>
> Reject a total that does not fit in an int. The lengths are page aligned
> and summed with check_add_overflow(), so the total is at least as large as
> any one entry and bounds the pin call, the offset accumulator and
> sg_alloc_table_from_pages().
>
> Fixes: bd72d4acda10 ("accel/amdxdna: Support user space allocated buffer")
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
> ---
>   drivers/accel/amdxdna/amdxdna_ubuf.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
> index 0e0cd69cd1fb..da8e32566ae0 100644
> --- a/drivers/accel/amdxdna/amdxdna_ubuf.c
> +++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
> @@ -125,6 +125,12 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
>   	}
>   
>   	ubuf->nr_pages = exp_info.size >> PAGE_SHIFT;
> +	if (ubuf->nr_pages > INT_MAX) {
> +		XDNA_ERR(xdna, "Too many pages %lld", ubuf->nr_pages);

XDNA_DBG(.... %llu", )

Could you also add the boundary check sashiko suggested? Maybe something 
like:

         for (i = 0, exp_info.size = 0; i < num_entries; i++) {
                 if (!IS_ALIGNED(va_ent[i].vaddr, PAGE_SIZE) ||
-                   !IS_ALIGNED(va_ent[i].len, PAGE_SIZE)) {
-                       XDNA_ERR(xdna, "Invalid address or len %llx, %llx",
+                   !IS_ALIGNED(va_ent[i].len, PAGE_SIZE) ||
+                   !va_ent[i].len) {
+                       XDNA_DBG(xdna, "Invalid address or len %llx, %llx",
                                  va_ent[i].vaddr, va_ent[i].len);

Thanks,

Lizhi


> +		ret = -EINVAL;
> +		goto free_ent;
> +	}
> +
>   	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>   	new_pinned = atomic64_add_return(ubuf->nr_pages, &ubuf->mm->pinned_vm);
>   	if (new_pinned > lock_limit && !capable(CAP_IPC_LOCK)) {

Re: [PATCH v2] accel/amdxdna: Bound the page count of a user supplied buffer

[Lizhi Hou]

On 8/26/26 14:28, Taimuraz Kaitmazov wrote:
> amdxdna_get_ubuf() puts a per-entry page count derived from a __u64
> va_ent[i].len into a u32, then passes it to pin_user_pages_fast(), whose
> nr_pages is an int. An entry of 2^44 bytes truncates npages to zero, so
> nothing is pinned, the ret != npages test still passes, and ubuf->pages
> keeps whatever kvmalloc_objs() returned while ubuf->nr_pages describes the
> untruncated count. amdxdna_ubuf_release() then walks all of it. Reaching
> that needs CAP_IPC_LOCK and a multi-gigabyte kvmalloc() to succeed.
>
> Reject a total that does not fit in an int. The lengths are page aligned
> and summed with check_add_overflow(), so the total is at least as large as
> any one entry and bounds the pin call, the offset accumulator and
> sg_alloc_table_from_pages().
>
> Reject a zero length entry as well: it contributes nothing to the mapping
> and a table of them leaves nr_pages at zero.
>
> Fixes: bd72d4acda10 ("accel/amdxdna: Support user space allocated buffer")
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
> ---
> v2:
> - XDNA_DBG and %llu, per your comment.
> - Reject a zero length entry in the validation loop, and lower that log to
>    XDNA_DBG too, as you suggested.
>
>   drivers/accel/amdxdna/amdxdna_ubuf.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
> index 0e0cd69cd1fb..bf1e4dd7bbc3 100644
> --- a/drivers/accel/amdxdna/amdxdna_ubuf.c
> +++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
> @@ -111,8 +111,9 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
>   
>   	for (i = 0, exp_info.size = 0; i < num_entries; i++) {
>   		if (!IS_ALIGNED(va_ent[i].vaddr, PAGE_SIZE) ||
> -		    !IS_ALIGNED(va_ent[i].len, PAGE_SIZE)) {
> -			XDNA_ERR(xdna, "Invalid address or len %llx, %llx",
> +		    !IS_ALIGNED(va_ent[i].len, PAGE_SIZE) ||
> +		    !va_ent[i].len) {
> +			XDNA_DBG(xdna, "Invalid address or len %llx, %llx",
>   				 va_ent[i].vaddr, va_ent[i].len);
>   			ret = -EINVAL;
>   			goto free_ent;
> @@ -125,6 +126,12 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
>   	}
>   
>   	ubuf->nr_pages = exp_info.size >> PAGE_SHIFT;
> +	if (ubuf->nr_pages > INT_MAX) {
> +		XDNA_DBG(xdna, "Too many pages %llu", ubuf->nr_pages);
> +		ret = -EINVAL;
> +		goto free_ent;
> +	}
> +
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>   	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>   	new_pinned = atomic64_add_return(ubuf->nr_pages, &ubuf->mm->pinned_vm);
>   	if (new_pinned > lock_limit && !capable(CAP_IPC_LOCK)) {