With special lengths supplied by user space, register_shm_helper() has
an integer overflow when calculating the number of pages covered by a
supplied user space memory region. This causes
internal_get_user_pages_fast() a helper function of
pin_user_pages_fast() to do a NULL pointer dereference.
[ 14.141620] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
[ 14.142556] Mem abort info:
[ 14.142829] ESR = 0x0000000096000044
[ 14.143237] EC = 0x25: DABT (current EL), IL = 32 bits
[ 14.143742] SET = 0, FnV = 0
[ 14.144052] EA = 0, S1PTW = 0
[ 14.144348] FSC = 0x04: level 0 translation fault
[ 14.144767] Data abort info:
[ 14.145053] ISV = 0, ISS = 0x00000044
[ 14.145394] CM = 0, WnR = 1
[ 14.145766] user pgtable: 4k pages, 48-bit VAs, pgdp=000000004278e000
[ 14.146279] [0000000000000010] pgd=0000000000000000, p4d=0000000000000000
[ 14.147435] Internal error: Oops: 96000044 [#1] PREEMPT SMP
[ 14.148026] Modules linked in:
[ 14.148595] CPU: 1 PID: 173 Comm: optee_example_a Not tainted 5.19.0 #11
[ 14.149204] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
[ 14.149832] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 14.150481] pc : internal_get_user_pages_fast+0x474/0xa80
[ 14.151640] lr : internal_get_user_pages_fast+0x404/0xa80
[ 14.152408] sp : ffff80000a88bb30
[ 14.152711] x29: ffff80000a88bb30 x28: 0000fffff836d000 x27: 0000fffff836e000
[ 14.153580] x26: fffffc0000000000 x25: fffffc0000f4a1c0 x24: ffff00000289fb70
[ 14.154634] x23: ffff000002702e08 x22: 0000000000040001 x21: ffff8000097eec60
[ 14.155378] x20: 0000000000f4a1c0 x19: 00e800007d287f43 x18: 0000000000000000
[ 14.156215] x17: 0000000000000000 x16: 0000000000000000 x15: 0000fffff836cfb0
[ 14.157068] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
[ 14.157747] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
[ 14.158576] x8 : ffff00000276ec80 x7 : 0000000000000000 x6 : 000000000000003f
[ 14.159243] x5 : 0000000000000000 x4 : ffff000041ec4eac x3 : ffff000002774cb8
[ 14.159977] x2 : 0000000000000004 x1 : 0000000000000010 x0 : 0000000000000000
[ 14.160883] Call trace:
[ 14.161166] internal_get_user_pages_fast+0x474/0xa80
[ 14.161763] pin_user_pages_fast+0x24/0x4c
[ 14.162227] register_shm_helper+0x194/0x330
[ 14.162734] tee_shm_register_user_buf+0x78/0x120
[ 14.163290] tee_ioctl+0xd0/0x11a0
[ 14.163739] __arm64_sys_ioctl+0xa8/0xec
[ 14.164227] invoke_syscall+0x48/0x114
[ 14.164653] el0_svc_common.constprop.0+0x44/0xec
[ 14.165130] do_el0_svc+0x2c/0xc0
[ 14.165498] el0_svc+0x2c/0x84
[ 14.165847] el0t_64_sync_handler+0x1ac/0x1b0
[ 14.166258] el0t_64_sync+0x18c/0x190
[ 14.166878] Code: 91002318 11000401 b900f7e1 f9403be1 (f820d839)
[ 14.167666] ---[ end trace 0000000000000000 ]---
Fix this by adding an overflow check when calculating the end of the
memory range. Also add an explicit call to access_ok() in
tee_shm_register_user_buf() to catch an invalid user space address
early.
Fixes: 033ddf12bcf5 ("tee: add register user memory")
Cc: [email protected]
Reported-by: Nimish Mishra <[email protected]>
Reported-by: Anirban Chakraborty <[email protected]>
Reported-by: Debdeep Mukhopadhyay <[email protected]>
Suggested-by: Jerome Forissier <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
---
drivers/tee/tee_shm.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index f2b1bcefcadd..f71651021c8d 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -231,15 +231,30 @@ EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
static struct tee_shm *
register_shm_helper(struct tee_context *ctx, unsigned long addr,
- size_t length, u32 flags, int id)
+ unsigned long length, u32 flags, int id)
{
struct tee_device *teedev = ctx->teedev;
+ unsigned long end_addr;
struct tee_shm *shm;
unsigned long start;
size_t num_pages;
void *ret;
int rc;
+ /* Check for overflows, this may be input from user space */
+ addr = untagged_addr(addr);
+ start = rounddown(addr, PAGE_SIZE);
+ if (check_add_overflow(addr, length, &end_addr))
+ return ERR_PTR(-EINVAL);
+ end_addr = roundup(end_addr, PAGE_SIZE);
+ if (end_addr < start)
+ return ERR_PTR(-EINVAL);
+ num_pages = (end_addr - start) / PAGE_SIZE;
+
+ /* Error out early if no pages are to be registered */
+ if (!num_pages)
+ return ERR_PTR(-EINVAL);
+
if (!tee_device_get(teedev))
return ERR_PTR(-EINVAL);
@@ -261,11 +276,8 @@ register_shm_helper(struct tee_context *ctx, unsigned long addr,
shm->flags = flags;
shm->ctx = ctx;
shm->id = id;
- addr = untagged_addr(addr);
- start = rounddown(addr, PAGE_SIZE);
shm->offset = addr - start;
shm->size = length;
- num_pages = (roundup(addr + length, PAGE_SIZE) - start) / PAGE_SIZE;
shm->pages = kcalloc(num_pages, sizeof(*shm->pages), GFP_KERNEL);
if (!shm->pages) {
ret = ERR_PTR(-ENOMEM);
@@ -326,6 +338,9 @@ struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx,
void *ret;
int id;
+ if (!access_ok((void __user *)addr, length))
+ return ERR_PTR(-EFAULT);
+
mutex_lock(&teedev->mutex);
id = idr_alloc(&teedev->idr, NULL, 1, 0, GFP_KERNEL);
mutex_unlock(&teedev->mutex);
--
2.31.1
Hi Jens,
On Thu, 18 Aug 2022 at 16:39, Jens Wiklander <[email protected]> wrote:
>
> With special lengths supplied by user space, register_shm_helper() has
> an integer overflow when calculating the number of pages covered by a
> supplied user space memory region. This causes
> internal_get_user_pages_fast() a helper function of
> pin_user_pages_fast() to do a NULL pointer dereference.
>
> [ 14.141620] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> [ 14.142556] Mem abort info:
> [ 14.142829] ESR = 0x0000000096000044
> [ 14.143237] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 14.143742] SET = 0, FnV = 0
> [ 14.144052] EA = 0, S1PTW = 0
> [ 14.144348] FSC = 0x04: level 0 translation fault
> [ 14.144767] Data abort info:
> [ 14.145053] ISV = 0, ISS = 0x00000044
> [ 14.145394] CM = 0, WnR = 1
> [ 14.145766] user pgtable: 4k pages, 48-bit VAs, pgdp=000000004278e000
> [ 14.146279] [0000000000000010] pgd=0000000000000000, p4d=0000000000000000
> [ 14.147435] Internal error: Oops: 96000044 [#1] PREEMPT SMP
> [ 14.148026] Modules linked in:
> [ 14.148595] CPU: 1 PID: 173 Comm: optee_example_a Not tainted 5.19.0 #11
> [ 14.149204] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
> [ 14.149832] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 14.150481] pc : internal_get_user_pages_fast+0x474/0xa80
> [ 14.151640] lr : internal_get_user_pages_fast+0x404/0xa80
> [ 14.152408] sp : ffff80000a88bb30
> [ 14.152711] x29: ffff80000a88bb30 x28: 0000fffff836d000 x27: 0000fffff836e000
> [ 14.153580] x26: fffffc0000000000 x25: fffffc0000f4a1c0 x24: ffff00000289fb70
> [ 14.154634] x23: ffff000002702e08 x22: 0000000000040001 x21: ffff8000097eec60
> [ 14.155378] x20: 0000000000f4a1c0 x19: 00e800007d287f43 x18: 0000000000000000
> [ 14.156215] x17: 0000000000000000 x16: 0000000000000000 x15: 0000fffff836cfb0
> [ 14.157068] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
> [ 14.157747] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
> [ 14.158576] x8 : ffff00000276ec80 x7 : 0000000000000000 x6 : 000000000000003f
> [ 14.159243] x5 : 0000000000000000 x4 : ffff000041ec4eac x3 : ffff000002774cb8
> [ 14.159977] x2 : 0000000000000004 x1 : 0000000000000010 x0 : 0000000000000000
> [ 14.160883] Call trace:
> [ 14.161166] internal_get_user_pages_fast+0x474/0xa80
> [ 14.161763] pin_user_pages_fast+0x24/0x4c
> [ 14.162227] register_shm_helper+0x194/0x330
> [ 14.162734] tee_shm_register_user_buf+0x78/0x120
> [ 14.163290] tee_ioctl+0xd0/0x11a0
> [ 14.163739] __arm64_sys_ioctl+0xa8/0xec
> [ 14.164227] invoke_syscall+0x48/0x114
> [ 14.164653] el0_svc_common.constprop.0+0x44/0xec
> [ 14.165130] do_el0_svc+0x2c/0xc0
> [ 14.165498] el0_svc+0x2c/0x84
> [ 14.165847] el0t_64_sync_handler+0x1ac/0x1b0
> [ 14.166258] el0t_64_sync+0x18c/0x190
> [ 14.166878] Code: 91002318 11000401 b900f7e1 f9403be1 (f820d839)
> [ 14.167666] ---[ end trace 0000000000000000 ]---
>
> Fix this by adding an overflow check when calculating the end of the
> memory range. Also add an explicit call to access_ok() in
> tee_shm_register_user_buf() to catch an invalid user space address
> early.
>
> Fixes: 033ddf12bcf5 ("tee: add register user memory")
> Cc: [email protected]
> Reported-by: Nimish Mishra <[email protected]>
> Reported-by: Anirban Chakraborty <[email protected]>
> Reported-by: Debdeep Mukhopadhyay <[email protected]>
> Suggested-by: Jerome Forissier <[email protected]>
> Signed-off-by: Jens Wiklander <[email protected]>
> ---
> drivers/tee/tee_shm.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
I can't see the v1 and neither a changelog for v2, so my comments
below may be duplicate.
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index f2b1bcefcadd..f71651021c8d 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -231,15 +231,30 @@ EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
>
> static struct tee_shm *
> register_shm_helper(struct tee_context *ctx, unsigned long addr,
> - size_t length, u32 flags, int id)
> + unsigned long length, u32 flags, int id)
> {
> struct tee_device *teedev = ctx->teedev;
> + unsigned long end_addr;
> struct tee_shm *shm;
> unsigned long start;
> size_t num_pages;
> void *ret;
> int rc;
>
> + /* Check for overflows, this may be input from user space */
IMO, this bound checking should be part of the parent function (like
tee_shm_register_user_buf() in this case).
> + addr = untagged_addr(addr);
> + start = rounddown(addr, PAGE_SIZE);
> + if (check_add_overflow(addr, length, &end_addr))
> + return ERR_PTR(-EINVAL);
Isn't this check redundant after access_ok()? AFAICS, access_ok()
should limit the upper bound to TASK_SIZE_MAX which should detect any
overflows.
> + end_addr = roundup(end_addr, PAGE_SIZE);
> + if (end_addr < start)
> + return ERR_PTR(-EINVAL);
Ditto?
-Sumit
> + num_pages = (end_addr - start) / PAGE_SIZE;
> +
> + /* Error out early if no pages are to be registered */
> + if (!num_pages)
> + return ERR_PTR(-EINVAL);
> +
> if (!tee_device_get(teedev))
> return ERR_PTR(-EINVAL);
>
> @@ -261,11 +276,8 @@ register_shm_helper(struct tee_context *ctx, unsigned long addr,
> shm->flags = flags;
> shm->ctx = ctx;
> shm->id = id;
> - addr = untagged_addr(addr);
> - start = rounddown(addr, PAGE_SIZE);
> shm->offset = addr - start;
> shm->size = length;
> - num_pages = (roundup(addr + length, PAGE_SIZE) - start) / PAGE_SIZE;
> shm->pages = kcalloc(num_pages, sizeof(*shm->pages), GFP_KERNEL);
> if (!shm->pages) {
> ret = ERR_PTR(-ENOMEM);
> @@ -326,6 +338,9 @@ struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx,
> void *ret;
> int id;
>
> + if (!access_ok((void __user *)addr, length))
> + return ERR_PTR(-EFAULT);
> +
> mutex_lock(&teedev->mutex);
> id = idr_alloc(&teedev->idr, NULL, 1, 0, GFP_KERNEL);
> mutex_unlock(&teedev->mutex);
> --
> 2.31.1
>
Hi Sumit,
On Thu, Aug 18, 2022 at 2:41 PM Sumit Garg <[email protected]> wrote:
>
> Hi Jens,
>
> On Thu, 18 Aug 2022 at 16:39, Jens Wiklander <[email protected]> wrote:
> >
> > With special lengths supplied by user space, register_shm_helper() has
> > an integer overflow when calculating the number of pages covered by a
> > supplied user space memory region. This causes
> > internal_get_user_pages_fast() a helper function of
> > pin_user_pages_fast() to do a NULL pointer dereference.
> >
> > [ 14.141620] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> > [ 14.142556] Mem abort info:
> > [ 14.142829] ESR = 0x0000000096000044
> > [ 14.143237] EC = 0x25: DABT (current EL), IL = 32 bits
> > [ 14.143742] SET = 0, FnV = 0
> > [ 14.144052] EA = 0, S1PTW = 0
> > [ 14.144348] FSC = 0x04: level 0 translation fault
> > [ 14.144767] Data abort info:
> > [ 14.145053] ISV = 0, ISS = 0x00000044
> > [ 14.145394] CM = 0, WnR = 1
> > [ 14.145766] user pgtable: 4k pages, 48-bit VAs, pgdp=000000004278e000
> > [ 14.146279] [0000000000000010] pgd=0000000000000000, p4d=0000000000000000
> > [ 14.147435] Internal error: Oops: 96000044 [#1] PREEMPT SMP
> > [ 14.148026] Modules linked in:
> > [ 14.148595] CPU: 1 PID: 173 Comm: optee_example_a Not tainted 5.19.0 #11
> > [ 14.149204] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
> > [ 14.149832] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [ 14.150481] pc : internal_get_user_pages_fast+0x474/0xa80
> > [ 14.151640] lr : internal_get_user_pages_fast+0x404/0xa80
> > [ 14.152408] sp : ffff80000a88bb30
> > [ 14.152711] x29: ffff80000a88bb30 x28: 0000fffff836d000 x27: 0000fffff836e000
> > [ 14.153580] x26: fffffc0000000000 x25: fffffc0000f4a1c0 x24: ffff00000289fb70
> > [ 14.154634] x23: ffff000002702e08 x22: 0000000000040001 x21: ffff8000097eec60
> > [ 14.155378] x20: 0000000000f4a1c0 x19: 00e800007d287f43 x18: 0000000000000000
> > [ 14.156215] x17: 0000000000000000 x16: 0000000000000000 x15: 0000fffff836cfb0
> > [ 14.157068] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
> > [ 14.157747] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
> > [ 14.158576] x8 : ffff00000276ec80 x7 : 0000000000000000 x6 : 000000000000003f
> > [ 14.159243] x5 : 0000000000000000 x4 : ffff000041ec4eac x3 : ffff000002774cb8
> > [ 14.159977] x2 : 0000000000000004 x1 : 0000000000000010 x0 : 0000000000000000
> > [ 14.160883] Call trace:
> > [ 14.161166] internal_get_user_pages_fast+0x474/0xa80
> > [ 14.161763] pin_user_pages_fast+0x24/0x4c
> > [ 14.162227] register_shm_helper+0x194/0x330
> > [ 14.162734] tee_shm_register_user_buf+0x78/0x120
> > [ 14.163290] tee_ioctl+0xd0/0x11a0
> > [ 14.163739] __arm64_sys_ioctl+0xa8/0xec
> > [ 14.164227] invoke_syscall+0x48/0x114
> > [ 14.164653] el0_svc_common.constprop.0+0x44/0xec
> > [ 14.165130] do_el0_svc+0x2c/0xc0
> > [ 14.165498] el0_svc+0x2c/0x84
> > [ 14.165847] el0t_64_sync_handler+0x1ac/0x1b0
> > [ 14.166258] el0t_64_sync+0x18c/0x190
> > [ 14.166878] Code: 91002318 11000401 b900f7e1 f9403be1 (f820d839)
> > [ 14.167666] ---[ end trace 0000000000000000 ]---
> >
> > Fix this by adding an overflow check when calculating the end of the
> > memory range. Also add an explicit call to access_ok() in
> > tee_shm_register_user_buf() to catch an invalid user space address
> > early.
> >
> > Fixes: 033ddf12bcf5 ("tee: add register user memory")
> > Cc: [email protected]
> > Reported-by: Nimish Mishra <[email protected]>
> > Reported-by: Anirban Chakraborty <[email protected]>
> > Reported-by: Debdeep Mukhopadhyay <[email protected]>
> > Suggested-by: Jerome Forissier <[email protected]>
> > Signed-off-by: Jens Wiklander <[email protected]>
> > ---
> > drivers/tee/tee_shm.c | 23 +++++++++++++++++++----
> > 1 file changed, 19 insertions(+), 4 deletions(-)
> >
>
> I can't see the v1 and neither a changelog for v2, so my comments
> below may be duplicate.
Fair point. The original patch wasn't posted publicly, but in order to
avoid confusion with that patch I chose to publish this as V2.
>
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index f2b1bcefcadd..f71651021c8d 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -231,15 +231,30 @@ EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
> >
> > static struct tee_shm *
> > register_shm_helper(struct tee_context *ctx, unsigned long addr,
> > - size_t length, u32 flags, int id)
> > + unsigned long length, u32 flags, int id)
> > {
> > struct tee_device *teedev = ctx->teedev;
> > + unsigned long end_addr;
> > struct tee_shm *shm;
> > unsigned long start;
> > size_t num_pages;
> > void *ret;
> > int rc;
> >
> > + /* Check for overflows, this may be input from user space */
>
> IMO, this bound checking should be part of the parent function (like
> tee_shm_register_user_buf() in this case).
I don't see any harm in checking it here even if it will then check
input from tee_shm_register_kernel_buf() too. Then I'm also reusing
the result in the roundup() and that should be done in this function.
>
> > + addr = untagged_addr(addr);
> > + start = rounddown(addr, PAGE_SIZE);
> > + if (check_add_overflow(addr, length, &end_addr))
> > + return ERR_PTR(-EINVAL);
>
> Isn't this check redundant after access_ok()? AFAICS, access_ok()
> should limit the upper bound to TASK_SIZE_MAX which should detect any
> overflows.
It may be redundant, depending on the configuration. It's likely
redundant on all platforms we care about at the moment, but who knows
where this will be used in the future.
>
> > + end_addr = roundup(end_addr, PAGE_SIZE);
> > + if (end_addr < start)
> > + return ERR_PTR(-EINVAL);
>
> Ditto?
Yeah, same argument.
Thanks,
Jens
>
> -Sumit
>
> > + num_pages = (end_addr - start) / PAGE_SIZE;
> > +
> > + /* Error out early if no pages are to be registered */
> > + if (!num_pages)
> > + return ERR_PTR(-EINVAL);
> > +
> > if (!tee_device_get(teedev))
> > return ERR_PTR(-EINVAL);
> >
> > @@ -261,11 +276,8 @@ register_shm_helper(struct tee_context *ctx, unsigned long addr,
> > shm->flags = flags;
> > shm->ctx = ctx;
> > shm->id = id;
> > - addr = untagged_addr(addr);
> > - start = rounddown(addr, PAGE_SIZE);
> > shm->offset = addr - start;
> > shm->size = length;
> > - num_pages = (roundup(addr + length, PAGE_SIZE) - start) / PAGE_SIZE;
> > shm->pages = kcalloc(num_pages, sizeof(*shm->pages), GFP_KERNEL);
> > if (!shm->pages) {
> > ret = ERR_PTR(-ENOMEM);
> > @@ -326,6 +338,9 @@ struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx,
> > void *ret;
> > int id;
> >
> > + if (!access_ok((void __user *)addr, length))
> > + return ERR_PTR(-EFAULT);
> > +
> > mutex_lock(&teedev->mutex);
> > id = idr_alloc(&teedev->idr, NULL, 1, 0, GFP_KERNEL);
> > mutex_unlock(&teedev->mutex);
> > --
> > 2.31.1
> >
On Thu, 18 Aug 2022 at 18:32, Jens Wiklander <[email protected]> wrote:
>
> Hi Sumit,
>
> On Thu, Aug 18, 2022 at 2:41 PM Sumit Garg <[email protected]> wrote:
> >
> > Hi Jens,
> >
> > On Thu, 18 Aug 2022 at 16:39, Jens Wiklander <[email protected]> wrote:
> > >
> > > With special lengths supplied by user space, register_shm_helper() has
> > > an integer overflow when calculating the number of pages covered by a
> > > supplied user space memory region. This causes
> > > internal_get_user_pages_fast() a helper function of
> > > pin_user_pages_fast() to do a NULL pointer dereference.
> > >
> > > [ 14.141620] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> > > [ 14.142556] Mem abort info:
> > > [ 14.142829] ESR = 0x0000000096000044
> > > [ 14.143237] EC = 0x25: DABT (current EL), IL = 32 bits
> > > [ 14.143742] SET = 0, FnV = 0
> > > [ 14.144052] EA = 0, S1PTW = 0
> > > [ 14.144348] FSC = 0x04: level 0 translation fault
> > > [ 14.144767] Data abort info:
> > > [ 14.145053] ISV = 0, ISS = 0x00000044
> > > [ 14.145394] CM = 0, WnR = 1
> > > [ 14.145766] user pgtable: 4k pages, 48-bit VAs, pgdp=000000004278e000
> > > [ 14.146279] [0000000000000010] pgd=0000000000000000, p4d=0000000000000000
> > > [ 14.147435] Internal error: Oops: 96000044 [#1] PREEMPT SMP
> > > [ 14.148026] Modules linked in:
> > > [ 14.148595] CPU: 1 PID: 173 Comm: optee_example_a Not tainted 5.19.0 #11
> > > [ 14.149204] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
> > > [ 14.149832] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > [ 14.150481] pc : internal_get_user_pages_fast+0x474/0xa80
> > > [ 14.151640] lr : internal_get_user_pages_fast+0x404/0xa80
> > > [ 14.152408] sp : ffff80000a88bb30
> > > [ 14.152711] x29: ffff80000a88bb30 x28: 0000fffff836d000 x27: 0000fffff836e000
> > > [ 14.153580] x26: fffffc0000000000 x25: fffffc0000f4a1c0 x24: ffff00000289fb70
> > > [ 14.154634] x23: ffff000002702e08 x22: 0000000000040001 x21: ffff8000097eec60
> > > [ 14.155378] x20: 0000000000f4a1c0 x19: 00e800007d287f43 x18: 0000000000000000
> > > [ 14.156215] x17: 0000000000000000 x16: 0000000000000000 x15: 0000fffff836cfb0
> > > [ 14.157068] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
> > > [ 14.157747] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
> > > [ 14.158576] x8 : ffff00000276ec80 x7 : 0000000000000000 x6 : 000000000000003f
> > > [ 14.159243] x5 : 0000000000000000 x4 : ffff000041ec4eac x3 : ffff000002774cb8
> > > [ 14.159977] x2 : 0000000000000004 x1 : 0000000000000010 x0 : 0000000000000000
> > > [ 14.160883] Call trace:
> > > [ 14.161166] internal_get_user_pages_fast+0x474/0xa80
> > > [ 14.161763] pin_user_pages_fast+0x24/0x4c
> > > [ 14.162227] register_shm_helper+0x194/0x330
> > > [ 14.162734] tee_shm_register_user_buf+0x78/0x120
> > > [ 14.163290] tee_ioctl+0xd0/0x11a0
> > > [ 14.163739] __arm64_sys_ioctl+0xa8/0xec
> > > [ 14.164227] invoke_syscall+0x48/0x114
> > > [ 14.164653] el0_svc_common.constprop.0+0x44/0xec
> > > [ 14.165130] do_el0_svc+0x2c/0xc0
> > > [ 14.165498] el0_svc+0x2c/0x84
> > > [ 14.165847] el0t_64_sync_handler+0x1ac/0x1b0
> > > [ 14.166258] el0t_64_sync+0x18c/0x190
> > > [ 14.166878] Code: 91002318 11000401 b900f7e1 f9403be1 (f820d839)
> > > [ 14.167666] ---[ end trace 0000000000000000 ]---
> > >
> > > Fix this by adding an overflow check when calculating the end of the
> > > memory range. Also add an explicit call to access_ok() in
> > > tee_shm_register_user_buf() to catch an invalid user space address
> > > early.
> > >
> > > Fixes: 033ddf12bcf5 ("tee: add register user memory")
> > > Cc: [email protected]
> > > Reported-by: Nimish Mishra <[email protected]>
> > > Reported-by: Anirban Chakraborty <[email protected]>
> > > Reported-by: Debdeep Mukhopadhyay <[email protected]>
> > > Suggested-by: Jerome Forissier <[email protected]>
> > > Signed-off-by: Jens Wiklander <[email protected]>
> > > ---
> > > drivers/tee/tee_shm.c | 23 +++++++++++++++++++----
> > > 1 file changed, 19 insertions(+), 4 deletions(-)
> > >
> >
> > I can't see the v1 and neither a changelog for v2, so my comments
> > below may be duplicate.
>
> Fair point. The original patch wasn't posted publicly, but in order to
> avoid confusion with that patch I chose to publish this as V2.
>
> >
> > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > > index f2b1bcefcadd..f71651021c8d 100644
> > > --- a/drivers/tee/tee_shm.c
> > > +++ b/drivers/tee/tee_shm.c
> > > @@ -231,15 +231,30 @@ EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
> > >
> > > static struct tee_shm *
> > > register_shm_helper(struct tee_context *ctx, unsigned long addr,
> > > - size_t length, u32 flags, int id)
> > > + unsigned long length, u32 flags, int id)
> > > {
> > > struct tee_device *teedev = ctx->teedev;
> > > + unsigned long end_addr;
> > > struct tee_shm *shm;
> > > unsigned long start;
> > > size_t num_pages;
> > > void *ret;
> > > int rc;
> > >
> > > + /* Check for overflows, this may be input from user space */
> >
> > IMO, this bound checking should be part of the parent function (like
> > tee_shm_register_user_buf() in this case).
>
> I don't see any harm in checking it here even if it will then check
> input from tee_shm_register_kernel_buf() too. Then I'm also reusing
> the result in the roundup() and that should be done in this function.
>
> >
> > > + addr = untagged_addr(addr);
> > > + start = rounddown(addr, PAGE_SIZE);
> > > + if (check_add_overflow(addr, length, &end_addr))
> > > + return ERR_PTR(-EINVAL);
> >
> > Isn't this check redundant after access_ok()? AFAICS, access_ok()
> > should limit the upper bound to TASK_SIZE_MAX which should detect any
> > overflows.
>
> It may be redundant, depending on the configuration. It's likely
> redundant on all platforms we care about at the moment, but who knows
> where this will be used in the future.
>
Firstly, access_ok() is the common kernel way to check for valid
user-space access as per quote below from
include/asm-generic/access_ok.h:
/*
* 'size' is a compile-time constant for most callers, so optimize for
* this case to turn the check into a single comparison against a constant
* limit and catch all possible overflows.
* On architectures with separate user address space (m68k, s390, parisc,
* sparc64) or those without an MMU, this should always return true.
*
* This version was originally contributed by Jonas Bonn for the
* OpenRISC architecture, and was found to be the most efficient
* for constant 'size' and 'limit' values.
*/
So we shouldn't invent a redundant method to check if there is a buggy
arch override for access_ok(). Also, results from check_add_overflow()
are still inaccurate as it can allow addresses greater than
TASK_SIZE_MAX.
Secondly, a redundant check which is anticipated to fix a future arch
bug doesn't qualify for a fix patch.
-Sumit
> >
> > > + end_addr = roundup(end_addr, PAGE_SIZE);
> > > + if (end_addr < start)
> > > + return ERR_PTR(-EINVAL);
> >
> > Ditto?
>
> Yeah, same argument.
>
> Thanks,
> Jens
>
> >
> > -Sumit
> >
> > > + num_pages = (end_addr - start) / PAGE_SIZE;
> > > +
> > > + /* Error out early if no pages are to be registered */
> > > + if (!num_pages)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > if (!tee_device_get(teedev))
> > > return ERR_PTR(-EINVAL);
> > >
> > > @@ -261,11 +276,8 @@ register_shm_helper(struct tee_context *ctx, unsigned long addr,
> > > shm->flags = flags;
> > > shm->ctx = ctx;
> > > shm->id = id;
> > > - addr = untagged_addr(addr);
> > > - start = rounddown(addr, PAGE_SIZE);
> > > shm->offset = addr - start;
> > > shm->size = length;
> > > - num_pages = (roundup(addr + length, PAGE_SIZE) - start) / PAGE_SIZE;
> > > shm->pages = kcalloc(num_pages, sizeof(*shm->pages), GFP_KERNEL);
> > > if (!shm->pages) {
> > > ret = ERR_PTR(-ENOMEM);
> > > @@ -326,6 +338,9 @@ struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx,
> > > void *ret;
> > > int id;
> > >
> > > + if (!access_ok((void __user *)addr, length))
> > > + return ERR_PTR(-EFAULT);
> > > +
> > > mutex_lock(&teedev->mutex);
> > > id = idr_alloc(&teedev->idr, NULL, 1, 0, GFP_KERNEL);
> > > mutex_unlock(&teedev->mutex);
> > > --
> > > 2.31.1
> > >
On Thu, Aug 18, 2022 at 4:09 AM Jens Wiklander
<[email protected]> wrote:
>
> Fix this by adding an overflow check when calculating the end of the
> memory range. Also add an explicit call to access_ok() in
> tee_shm_register_user_buf() to catch an invalid user space address
> early.
I applied the access_ok() part of this which was clearly missing.
The check_add_overflow() should be pointless with that.
And the "roundup() overflows" check should just check for a zero
result - if it is actually needed. Which I don't think it is on any
relevant platform (the TEE subsystem only works on arm and x86).
I do think it might be worth discussing whether
ALTERNATE_USER_ADDRESS_SPACE (and no-MMU) architectures should still
have access_ok() check that it doesn't actually wrap around in the
address space, so I've added linux-arch here.
That's m68k, PA-RISC, S390 and sparc.
In fact, I wonder if some or all of those might want to have the
TASK_SIZE limit anyway - they may have a separate user address space,
but several ones have some limits even then, and probably should have
access_ok() check them rather than depend on the hardware then giving
page fault.
For example, sparc32 has a user address space, but defines TASK_SIZE
to 0xF0000000. m68k has several different case. parisc also has an
actual limit.
And s390 uses
#define TASK_SIZE_MAX (-PAGE_SIZE)
which is a good value and leaves a guard page at the top.
So I think the "roundup overflows" would probably be best fixed by
just admitting that every architecture in practice has a TASK_SIZE_MAX
anyway, and we should just make access_ok() check it.
Linus
On Thu, Aug 18, 2022 at 6:38 PM Linus Torvalds
<[email protected]> wrote:
>
> On Thu, Aug 18, 2022 at 4:09 AM Jens Wiklander
> <[email protected]> wrote:
> >
> > Fix this by adding an overflow check when calculating the end of the
> > memory range. Also add an explicit call to access_ok() in
> > tee_shm_register_user_buf() to catch an invalid user space address
> > early.
>
> I applied the access_ok() part of this which was clearly missing.
>
> The check_add_overflow() should be pointless with that.
>
> And the "roundup() overflows" check should just check for a zero
> result - if it is actually needed. Which I don't think it is on any
> relevant platform (the TEE subsystem only works on arm and x86).
>
> I do think it might be worth discussing whether
> ALTERNATE_USER_ADDRESS_SPACE (and no-MMU) architectures should still
> have access_ok() check that it doesn't actually wrap around in the
> address space, so I've added linux-arch here.
>
> That's m68k, PA-RISC, S390 and sparc.
>
> In fact, I wonder if some or all of those might want to have the
> TASK_SIZE limit anyway - they may have a separate user address space,
> but several ones have some limits even then, and probably should have
> access_ok() check them rather than depend on the hardware then giving
> page fault.
>
> For example, sparc32 has a user address space, but defines TASK_SIZE
> to 0xF0000000. m68k has several different case. parisc also has an
> actual limit.
>
> And s390 uses
>
> #define TASK_SIZE_MAX (-PAGE_SIZE)
>
> which is a good value and leaves a guard page at the top.
>
> So I think the "roundup overflows" would probably be best fixed by
> just admitting that every architecture in practice has a TASK_SIZE_MAX
> anyway, and we should just make access_ok() check it.
Thanks for the detailed clarifications. I'll remove the redundant
overflow checks.
Cheers,
Jens
>
> Linus