[bug report] dma-buf: heaps: Rework heap allocation hooks to return struct dma_buf instead of fd

[bug report] dma-buf: heaps: Rework heap allocation hooks to return struct dma_buf instead of fd

[Dan Carpenter]

I'm not sure exactly who to report this bug too.  Probably the mm
devs?

	drivers/dma-buf/heaps/system_heap.c:499 system_heap_allocate()
	warn: passing positive error code 's32min-(-1),1' to 'ERR_PTR'

drivers/dma-buf/heaps/system_heap.c
    459         if (cc_shared) {
    460                 for_each_sgtable_sg(table, sg, i) {
    461                         ret = system_heap_set_page_decrypted(sg_page(sg));
    462                         if (ret)
    463                                 goto free_pages;

It kind of looks like system_heap_set_page_decrypted() can return 1.

    464                 }
    465         }
    466 
    467         /* create the dmabuf */
    468         exp_info.exp_name = dma_heap_get_name(heap);
    469         exp_info.ops = &system_heap_buf_ops;
    470         exp_info.size = buffer->len;
    471         exp_info.flags = fd_flags;
    472         exp_info.priv = buffer;
    473         dmabuf = dma_buf_export(&exp_info);
    474         if (IS_ERR(dmabuf)) {
    475                 ret = PTR_ERR(dmabuf);
    476                 goto free_pages;
    477         }
    478         return dmabuf;
    479 
    480 free_pages:
    481         for_each_sgtable_sg(table, sg, i) {
    482                 struct page *p = sg_page(sg);
    483 
    484                 /*
    485                  * Intentionally leak pages that cannot be re-encrypted
    486                  * to prevent shared memory from being reused.
    487                  */
    488                 if (buffer->cc_shared &&
    489                     system_heap_set_page_encrypted(p))
    490                         continue;
    491                 __free_pages(p, compound_order(p));
    492         }
    493         sg_free_table(table);
    494 free_buffer:
    495         list_for_each_entry_safe(page, tmp_page, &pages, lru)
    496                 __free_pages(page, compound_order(page));
    497         kfree(buffer);
    498 
--> 499         return ERR_PTR(ret);
    500 }

The problem is that add_to_pagemap() returns PM_END_OF_BUFFER (1)
which is used by pagemap_read() and nowhere else.  The call tree
is:

system_heap_allocate()
system_heap_set_page_decrypted()
set_memory_decrypted()
realm_set_memory_decrypted()
__set_memory_enc_dec()
__change_memory_common()
update_range_prot()
walk_kernel_page_table_range_lockless()
walk_pgd_range()
pagemap_pte_hole()
add_to_pagemap()

This code seems sort of old and I guess no one has reported the bug
so maybe it's a false positive, but it feels like it's asking for
problems to return the PM_END_OF_BUFFER.  There aren't any comments
on any of those functions above explaining what return values are
expected.

This email is a free service from the Smatch-CI project [smatch.sf.net].

regards,
dan carpenter

Re: [bug report] dma-buf: heaps: Rework heap allocation hooks to return struct dma_buf instead of fd

[John Stultz]

On Mon, May 18, 2026 at 8:06 AM Will Deacon <will@kernel.org> wrote:
>
> On Sat, May 02, 2026 at 12:40:10PM +0300, Dan Carpenter wrote:
> > I'm not sure exactly who to report this bug too.  Probably the mm
> > devs?
>
> [Adding John and TJ, in case they are interested / able to help]
>
> Will
>
> (original report follows)
>
> >       drivers/dma-buf/heaps/system_heap.c:499 system_heap_allocate()
> >       warn: passing positive error code 's32min-(-1),1' to 'ERR_PTR'
> >
> > drivers/dma-buf/heaps/system_heap.c
> >     459         if (cc_shared) {
> >     460                 for_each_sgtable_sg(table, sg, i) {
> >     461                         ret = system_heap_set_page_decrypted(sg_page(sg));
> >     462                         if (ret)
> >     463                                 goto free_pages;
> >
> > It kind of looks like system_heap_set_page_decrypted() can return 1.
> >
> >     464                 }
> >     465         }
> >     466
> >     467         /* create the dmabuf */
> >     468         exp_info.exp_name = dma_heap_get_name(heap);
> >     469         exp_info.ops = &system_heap_buf_ops;
> >     470         exp_info.size = buffer->len;
> >     471         exp_info.flags = fd_flags;
> >     472         exp_info.priv = buffer;
> >     473         dmabuf = dma_buf_export(&exp_info);
> >     474         if (IS_ERR(dmabuf)) {
> >     475                 ret = PTR_ERR(dmabuf);
> >     476                 goto free_pages;
> >     477         }
> >     478         return dmabuf;
> >     479
> >     480 free_pages:
> >     481         for_each_sgtable_sg(table, sg, i) {
> >     482                 struct page *p = sg_page(sg);
> >     483
> >     484                 /*
> >     485                  * Intentionally leak pages that cannot be re-encrypted
> >     486                  * to prevent shared memory from being reused.
> >     487                  */
> >     488                 if (buffer->cc_shared &&
> >     489                     system_heap_set_page_encrypted(p))
> >     490                         continue;
> >     491                 __free_pages(p, compound_order(p));
> >     492         }
> >     493         sg_free_table(table);
> >     494 free_buffer:
> >     495         list_for_each_entry_safe(page, tmp_page, &pages, lru)
> >     496                 __free_pages(page, compound_order(page));
> >     497         kfree(buffer);
> >     498
> > --> 499         return ERR_PTR(ret);
> >     500 }
> >
> > The problem is that add_to_pagemap() returns PM_END_OF_BUFFER (1)
> > which is used by pagemap_read() and nowhere else.  The call tree
> > is:
> >
> > system_heap_allocate()
> > system_heap_set_page_decrypted()
> > set_memory_decrypted()
> > realm_set_memory_decrypted()
> > __set_memory_enc_dec()
> > __change_memory_common()
> > update_range_prot()
> > walk_kernel_page_table_range_lockless()
> > walk_pgd_range()
> > pagemap_pte_hole()
> > add_to_pagemap()
> >
> > This code seems sort of old and I guess no one has reported the bug
> > so maybe it's a false positive, but it feels like it's asking for
> > problems to return the PM_END_OF_BUFFER.  There aren't any comments
> > on any of those functions above explaining what return values are
> > expected.

So, do we just need the system_heap_set_page_decrypted() to have a if
(ret > 0) special case on the return check from
set_memory_decrypted()?

Or should this get fixed at a deeper level?

thanks
-john

Re: [bug report] dma-buf: heaps: Rework heap allocation hooks to return struct dma_buf instead of fd

[T.J. Mercier]

On Mon, May 18, 2026 at 8:06 AM Will Deacon <will@kernel.org> wrote:
>
> On Sat, May 02, 2026 at 12:40:10PM +0300, Dan Carpenter wrote:
> > I'm not sure exactly who to report this bug too.  Probably the mm
> > devs?
>
> [Adding John and TJ, in case they are interested / able to help]
>
> Will
>
> (original report follows)
>
> >       drivers/dma-buf/heaps/system_heap.c:499 system_heap_allocate()
> >       warn: passing positive error code 's32min-(-1),1' to 'ERR_PTR'
> >
> > drivers/dma-buf/heaps/system_heap.c
> >     459         if (cc_shared) {
> >     460                 for_each_sgtable_sg(table, sg, i) {
> >     461                         ret = system_heap_set_page_decrypted(sg_page(sg));
> >     462                         if (ret)
> >     463                                 goto free_pages;
> >
> > It kind of looks like system_heap_set_page_decrypted() can return 1.
> >
> >     464                 }
> >     465         }
> >     466
> >     467         /* create the dmabuf */
> >     468         exp_info.exp_name = dma_heap_get_name(heap);
> >     469         exp_info.ops = &system_heap_buf_ops;
> >     470         exp_info.size = buffer->len;
> >     471         exp_info.flags = fd_flags;
> >     472         exp_info.priv = buffer;
> >     473         dmabuf = dma_buf_export(&exp_info);
> >     474         if (IS_ERR(dmabuf)) {
> >     475                 ret = PTR_ERR(dmabuf);
> >     476                 goto free_pages;
> >     477         }
> >     478         return dmabuf;
> >     479
> >     480 free_pages:
> >     481         for_each_sgtable_sg(table, sg, i) {
> >     482                 struct page *p = sg_page(sg);
> >     483
> >     484                 /*
> >     485                  * Intentionally leak pages that cannot be re-encrypted
> >     486                  * to prevent shared memory from being reused.
> >     487                  */
> >     488                 if (buffer->cc_shared &&
> >     489                     system_heap_set_page_encrypted(p))
> >     490                         continue;
> >     491                 __free_pages(p, compound_order(p));
> >     492         }
> >     493         sg_free_table(table);
> >     494 free_buffer:
> >     495         list_for_each_entry_safe(page, tmp_page, &pages, lru)
> >     496                 __free_pages(page, compound_order(page));
> >     497         kfree(buffer);
> >     498
> > --> 499         return ERR_PTR(ret);
> >     500 }
> >
> > The problem is that add_to_pagemap() returns PM_END_OF_BUFFER (1)
> > which is used by pagemap_read() and nowhere else.  The call tree
> > is:
> >
> > system_heap_allocate()
> > system_heap_set_page_decrypted()
> > set_memory_decrypted()
> > realm_set_memory_decrypted()
> > __set_memory_enc_dec()
> > __change_memory_common()
> > update_range_prot()
> > walk_kernel_page_table_range_lockless()
> > walk_pgd_range()
> > pagemap_pte_hole()

This doesn't look right to me. update_range_prot is in arm64 arch
code. It uses the arm64 pageattr_ops which does not provide a pte_hole
op. So walk_pgd_range should never call ops->pt_hole.

> > add_to_pagemap()
> >
> > This code seems sort of old and I guess no one has reported the bug
> > so maybe it's a false positive, but it feels like it's asking for
> > problems to return the PM_END_OF_BUFFER.  There aren't any comments
> > on any of those functions above explaining what return values are
> > expected.
> >
> > This email is a free service from the Smatch-CI project [smatch.sf.net].
> >
> > regards,
> > dan carpenter