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: stable@vger.kernel.org Reported-by: Nimish Mishra neelam.nimish@gmail.com Reported-by: Anirban Chakraborty ch.anirban00727@gmail.com Reported-by: Debdeep Mukhopadhyay debdeep.mukhopadhyay@gmail.com Suggested-by: Jerome Forissier jerome.forissier@linaro.org Signed-off-by: Jens Wiklander jens.wiklander@linaro.org --- 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);
Hi Jens,
On Thu, 18 Aug 2022 at 16:39, Jens Wiklander jens.wiklander@linaro.org 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: stable@vger.kernel.org Reported-by: Nimish Mishra neelam.nimish@gmail.com Reported-by: Anirban Chakraborty ch.anirban00727@gmail.com Reported-by: Debdeep Mukhopadhyay debdeep.mukhopadhyay@gmail.com Suggested-by: Jerome Forissier jerome.forissier@linaro.org Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
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 sumit.garg@linaro.org wrote:
Hi Jens,
On Thu, 18 Aug 2022 at 16:39, Jens Wiklander jens.wiklander@linaro.org 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: stable@vger.kernel.org Reported-by: Nimish Mishra neelam.nimish@gmail.com Reported-by: Anirban Chakraborty ch.anirban00727@gmail.com Reported-by: Debdeep Mukhopadhyay debdeep.mukhopadhyay@gmail.com Suggested-by: Jerome Forissier jerome.forissier@linaro.org Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
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 jens.wiklander@linaro.org wrote:
Hi Sumit,
On Thu, Aug 18, 2022 at 2:41 PM Sumit Garg sumit.garg@linaro.org wrote:
Hi Jens,
On Thu, 18 Aug 2022 at 16:39, Jens Wiklander jens.wiklander@linaro.org 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: stable@vger.kernel.org Reported-by: Nimish Mishra neelam.nimish@gmail.com Reported-by: Anirban Chakraborty ch.anirban00727@gmail.com Reported-by: Debdeep Mukhopadhyay debdeep.mukhopadhyay@gmail.com Suggested-by: Jerome Forissier jerome.forissier@linaro.org Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
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 jens.wiklander@linaro.org 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 torvalds@linuxfoundation.org wrote:
On Thu, Aug 18, 2022 at 4:09 AM Jens Wiklander jens.wiklander@linaro.org 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
op-tee@lists.trustedfirmware.org
-
Jens Wiklander -
Linus Torvalds -
Sumit Garg