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

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

[Taimuraz Kaitmazov]

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);
+		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)) {
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Unhandled zero-size user buffer bypasses constraints and leads to a fatal kernel panic due to ZERO_SIZE_PTR dereference.
--

commit 0ce5a2a6bc3b9a95a5dd641e22f058018b729ead
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: Bound the page count of a user supplied buffer

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().
This patch rejects a total that does not fit in an int to prevent
truncation and unbound memory operations.

> diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
> index 0e0cd69cd1fbf..da8e32566ae0a 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);
> +		ret = -EINVAL;
> +		goto free_ent;
> +	}
> +

[Severity: Critical]
This isn't a bug introduced by this patch, but is it possible for
ubuf->nr_pages to be 0 here, leading to a kernel panic later?

If a user calls the AMDXDNA_DRM_CREATE_BO ioctl with a virtual address
table containing at least one entry but with lengths set to 0,
exp_info.size becomes 0 and ubuf->nr_pages becomes 0. This bypasses the
bounds check above.

Then kvmalloc_objs() allocates ubuf->pages with size 0, which returns
ZERO_SIZE_PTR (0x10) instead of NULL. Later, pin_user_pages_fast() handles
npages = 0 by returning 0, allowing the zero-length dma-buf to be
exported.

When the buffer is eventually mapped in amdxdna_ubuf_map() in
amdxdna_ubuf.c:

ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->nr_pages, 0,
                                ubuf->nr_pages << PAGE_SHIFT, GFP_KERNEL);

sg_alloc_table_from_pages() delegates to sg_alloc_append_table_from_pages(),
which computes chunks = 1 and evaluates pages[0] in its first iteration
since the nested loop bounds check is skipped when n_pages == 0.

Dereferencing ZERO_SIZE_PTR (0x10) will cause a fatal kernel page fault.

Could we add a check for ubuf->nr_pages == 0 here as well?

>  	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)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826195736.358579-1-taimuraz@kaitmazov.com?part=1

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)) {

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

[Taimuraz Kaitmazov]

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;
+	}
+
 	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)) {
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unrestricted userspace-controlled allocation size for `va_ent` can trigger the OOM killer.
--

commit 9b9a5c5cd1330d0aae0aea71c45dc018b59ee7e1
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: Bound the page count of a user supplied buffer

This patch fixes an issue where amdxdna_get_ubuf() could truncate a per-entry
page count derived from a 64-bit length into a 32-bit integer. It adds
validation to ensure the total page count fits within an integer, preventing
invalid memory accesses. It also explicitly rejects zero-length buffer
entries.

> diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
> index 0e0cd69cd1fbf..bf1e4dd7bbc30 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++) {

[Severity: High]
This is a pre-existing issue, but is it possible for num_entries to be a
massive value here?

Looking at amdxdna_gem_create_ubuf_object() in
drivers/accel/amdxdna/amdxdna_gem.c, it appears that va_tbl.num_entries
from userspace is passed directly to amdxdna_get_ubuf() without any bounds
checking.

Earlier in amdxdna_get_ubuf(), we allocate memory based on this value:

	va_ent = kvzalloc_objs(*va_ent, num_entries);

If an unprivileged user with a non-zero RLIMIT_MEMLOCK sets num_entries to a
huge value like U32_MAX, can this allocate an unrestricted amount of memory
and trigger the OOM killer before we even reach the bounds checks on nr_pages
added in this patch?

>  		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;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826212825.408846-1-taimuraz@kaitmazov.com?part=1

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)) {