Re: [PATCH v4] tee: Use iov_iter to better support shared buffer registration

[Sumit Garg]

+ Jens A., Al Viro

<snip>

> >>>> +        */
> >>>> +       if (iov_iter_is_kvec(iter))
> >>>> +               shm_get_kernel_pages(shm->pages, num_pages);
> >>>> +
> >>>> +       shm->offset = off;
> >>>> +       shm->size = len;
> >>>> +       shm->num_pages = num_pages;
> >>>> +
> >>>>         rc = teedev->desc->ops->shm_register(ctx, shm, shm->pages,
> >>>>                                              shm->num_pages, start);
> >>>>         if (rc) {
> >>>> @@ -279,10 +276,11 @@ register_shm_helper(struct tee_context *ctx, unsigned long addr,
> >>>>
> >>>>         return shm;
> >>>>  err_put_shm_pages:
> >>>> -       if (flags & TEE_SHM_USER_MAPPED)
> >>>> +       if (!iov_iter_is_kvec(iter))
> >>>>                 unpin_user_pages(shm->pages, shm->num_pages);
> >>>>         else
> >>>>                 shm_put_kernel_pages(shm->pages, shm->num_pages);
> >>>> +err_free_shm_pages:
> >>>>         kfree(shm->pages);
> >>>>  err_free_shm:
> >>>>         kfree(shm);
> >>>> @@ -307,8 +305,9 @@ struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx,
> >>>>         u32 flags = TEE_SHM_USER_MAPPED | TEE_SHM_DYNAMIC;
> >>>>         struct tee_device *teedev = ctx->teedev;
> >>>>         struct tee_shm *shm;
> >>>> +       struct iov_iter iter;
> >>>>         void *ret;
> >>>> -       int id;
> >>>> +       int id, err;
> >>>>
> >>>>         if (!access_ok((void __user *)addr, length))
> >>>>                 return ERR_PTR(-EFAULT);
> >>>> @@ -319,7 +318,11 @@ struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx,
> >>>>         if (id < 0)
> >>>>                 return ERR_PTR(id);
> >>>>
> >>>> -       shm = register_shm_helper(ctx, addr, length, flags, id);
> >>>> +       err = import_ubuf(ITER_DEST, (void __user *)addr, length, &iter);
> >>>
> >>> As I mentioned in a previous review, import_ubuf() already does the
> >>> access_ok() check, so we don't need the extra access_ok() check above.
> >>> Also, you should move import_ubuf() to be the first invocation within
> >>> this API.
> >>
> >> My apologies, I re-added import_ubuf() during testing to debug an issue and
> >
> > I suppose you intended to mention access_ok() here, BTW, no worries :).
>
> Re-running xtest after removing the access_ok() I have a crash in
> regression_5006.3 Allocate_out_of_memory
>
> [   89.258100] ------------[ cut here ]------------
> [   89.258377] WARNING: CPU: 1 PID: 134 at mm/page_alloc.c:4402
> __alloc_pages+0x674/0xd14
> [   89.258988] Modules linked in:
> [   89.259554] CPU: 1 PID: 134 Comm: xtest Not tainted 6.6.0-g1ebcc18a80d7-dirty #69
> [   89.259763] Hardware name: linux,dummy-virt (DT)
> [   89.259977] pstate: 21400005 (nzCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> [   89.260143] pc : __alloc_pages+0x674/0xd14
> [   89.260252] lr : alloc_pages+0xac/0x160
> [   89.260364] sp : ffff8000803f3a30
> [   89.260437] x29: ffff8000803f3a30 x28: ffff8000803f3d70 x27: 0000000000000000
> [   89.260705] x26: 000000000000000b x25: 0000000000000000 x24: ffff000000178000
> [   89.260847] x23: ffff00003f52b720 x22: ffff000001442720 x21: 0000000000000000
> [   89.261022] x20: 000000000000000b x19: 0000000000000000 x18: ffff8000803f3b78
> [   89.261162] x17: 0000000000000000 x16: ffffa7a428b67660 x15: 0000ffff63ffffff
> [   89.261348] x14: 0000000000000008 x13: 1fffe00000a4c421 x12: ffff8000803f3b78
> [   89.261528] x11: ffff000005262100 x10: ffff000005262108 x9 : ffff00000526210c
> [   89.261739] x8 : 0000aaab0a97c000 x7 : 0000aaab0a90e000 x6 : ffff000005262150
> [   89.261920] x5 : 0000000000000000 x4 : ffff000000976740 x3 : 0000000000000000
> [   89.262098] x2 : 0000000000000000 x1 : 0000000000000001 x0 : ffffa7a429daf000
> [   89.262340] Call trace:
> [   89.262921]  __alloc_pages+0x674/0xd14
> [   89.263262]  alloc_pages+0xac/0x160
> [   89.263373]  alloc_pages_exact+0x48/0x94
> [   89.263464]  optee_shm_register+0xa8/0x1f4
> [   89.263591]  register_shm_helper+0x1bc/0x28c
> [   89.263687]  tee_shm_register_user_buf+0xb8/0x128
> [   89.263816]  tee_ioctl+0xbc/0xfa0
> [   89.263915]  __arm64_sys_ioctl+0xa8/0xec
> [   89.264053]  invoke_syscall+0x48/0x114
> [   89.264173]  el0_svc_common.constprop.0+0x40/0xe8
> [   89.264321]  do_el0_svc+0x20/0x2c
> [   89.264488]  el0_svc+0x40/0xf4
> [   89.264578]  el0t_64_sync_handler+0x13c/0x158
> [   89.264714]  el0t_64_sync+0x190/0x194
> [   89.265003] ---[ end trace 0000000000000000 ]---
>
>
> The issue is that, in import_ubuf(), it updates the length [1], making the
>
> access_ok() succeed and leading to an issue later in the page allocation process.
>
> To fix, I propose to add a test in tee_shm_register_user_buf() after calling
>
> import_ubuf()
>
>         if (length != iter_iov_len(&iter))
>                 return ERR_PTR(-EFAULT);
>
>
> Would it be ok for you ? I'm afraid of side effects if I update import_ubuf()...

IMO, access_ok() should be the first thing that import_ubuf() or
import_single_range() should do, something as follows:

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 8ff6824a1005..4aee0371824c 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1384,10 +1384,10 @@ EXPORT_SYMBOL(import_single_range);

 int import_ubuf(int rw, void __user *buf, size_t len, struct iov_iter *i)
 {
-       if (len > MAX_RW_COUNT)
-               len = MAX_RW_COUNT;
        if (unlikely(!access_ok(buf, len)))
                return -EFAULT;
+       if (len > MAX_RW_COUNT)
+               len = MAX_RW_COUNT;

        iov_iter_ubuf(i, rw, buf, len);
        return 0;

Jens A., Al Viro,

Was there any particular reason which I am unaware of to perform
access_ok() check on modified input length?

-Sumit

>
> [1] https://elixir.bootlin.com/linux/latest/source/lib/iov_iter.c#L1553
>