A race condition may occur if the user physically removes the smc_abi device while calling open().
This is a race condition between optee_smc_open() function and the optee_smc_remove() function, which may lead to Use-After-Free.
Therefore, add a refcount check to optee_smc_remove() function to free the "optee" structure after the device is close()d.
---------------CPU 0--------------------CPU 1----------------- optee_smc_open() | optee_smc_remove() -------------------------------------------------------------- struct optee *optee = tee_get_| drvdata(ctx->teedev); — (1) | | struct optee *optee = platform_ | get_drvdata(pdev); | ... | kfree(optee); — (2) u32 sec_caps = optee->smc.sec_| caps; — (3)
Signed-off-by: Yoochan Lee yoochan1026@gmail.com --- drivers/tee/optee/optee_private.h | 1 + drivers/tee/optee/smc_abi.c | 66 ++++++++++++++++++++++--------- 2 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index 04ae58892608..f52b1cf20eab 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -175,6 +175,7 @@ struct optee { bool scan_bus_done; struct workqueue_struct *scan_bus_wq; struct work_struct scan_bus_work; + struct kref refcnt; };
struct optee_session { diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index a1c1fa1a9c28..4fbec2acc255 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -1077,18 +1077,61 @@ static void optee_get_version(struct tee_device *teedev, *vers = v; }
+static void optee_smc_delete(struct kref *kref) +{ + struct optee *optee = container_of(kref, struct optee, refcnt); + + /* + * Ask OP-TEE to free all cached shared memory objects to decrease + * reference counters and also avoid wild pointers in secure world + * into the old shared memory range. + */ + if (!optee->rpc_param_count) + optee_disable_shm_cache(optee); + + optee_smc_notif_uninit_irq(optee); + + optee_remove_common(optee); + + if (optee->smc.memremaped_shm) + memunmap(optee->smc.memremaped_shm); + + kfree(optee); +} + +static void optee_smc_release_supp(struct tee_context *ctx) +{ + struct optee *optee = tee_get_drvdata(ctx->teedev); + + optee_release_helper(ctx, optee_close_session_helper); + if (optee->scan_bus_wq) { + destroy_workqueue(optee->scan_bus_wq); + optee->scan_bus_wq = NULL; + } + optee_supp_release(&optee->supp); + kref_put(&optee->refcnt, optee_smc_delete); +} + +static void optee_smc_release(struct tee_context *ctx) +{ + struct optee *optee = tee_get_drvdata(ctx->teedev); + + optee_release_helper(ctx, optee_close_session_helper); + kref_put(&optee->refcnt, optee_smc_delete); +} + static int optee_smc_open(struct tee_context *ctx) { struct optee *optee = tee_get_drvdata(ctx->teedev); u32 sec_caps = optee->smc.sec_caps; - + kref_get(&optee->refcnt); return optee_open(ctx, sec_caps & OPTEE_SMC_SEC_CAP_MEMREF_NULL); }
static const struct tee_driver_ops optee_clnt_ops = { .get_version = optee_get_version, .open = optee_smc_open, - .release = optee_release, + .release = optee_smc_release, .open_session = optee_open_session, .close_session = optee_close_session, .invoke_func = optee_invoke_func, @@ -1106,7 +1149,7 @@ static const struct tee_desc optee_clnt_desc = { static const struct tee_driver_ops optee_supp_ops = { .get_version = optee_get_version, .open = optee_smc_open, - .release = optee_release_supp, + .release = optee_smc_release_supp, .supp_recv = optee_supp_recv, .supp_send = optee_supp_send, .shm_register = optee_shm_register_supp, @@ -1319,22 +1362,7 @@ static int optee_smc_remove(struct platform_device *pdev) { struct optee *optee = platform_get_drvdata(pdev);
- /* - * Ask OP-TEE to free all cached shared memory objects to decrease - * reference counters and also avoid wild pointers in secure world - * into the old shared memory range. - */ - if (!optee->rpc_param_count) - optee_disable_shm_cache(optee); - - optee_smc_notif_uninit_irq(optee); - - optee_remove_common(optee); - - if (optee->smc.memremaped_shm) - memunmap(optee->smc.memremaped_shm); - - kfree(optee); + kref_put(&optee->refcnt, optee_smc_delete);
return 0; }
I missed the initialization of kref. So please add the following patch.
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index 4fbec2acc255..f17532f811e1 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -1478,6 +1478,7 @@ static int optee_probe(struct platform_device *pdev) optee->smc.invoke_fn = invoke_fn; optee->smc.sec_caps = sec_caps; optee->rpc_param_count = rpc_param_count; + kref_init(&optee->refcnt);
teedev = tee_device_alloc(&optee_clnt_desc, NULL, pool, optee); if (IS_ERR(teedev)) {
2022년 12월 31일 (토) 오후 2:19, Yoochan Lee yoochan1026@gmail.com님이 작성:
A race condition may occur if the user physically removes the smc_abi device while calling open().
This is a race condition between optee_smc_open() function and the optee_smc_remove() function, which may lead to Use-After-Free.
Therefore, add a refcount check to optee_smc_remove() function to free the "optee" structure after the device is close()d.
---------------CPU 0--------------------CPU 1----------------- optee_smc_open() | optee_smc_remove()
struct optee *optee = tee_get_| drvdata(ctx->teedev); — (1) | | struct optee *optee = platform_ | get_drvdata(pdev); | ... | kfree(optee); — (2) u32 sec_caps = optee->smc.sec_| caps; — (3)
Signed-off-by: Yoochan Lee yoochan1026@gmail.com
drivers/tee/optee/optee_private.h | 1 + drivers/tee/optee/smc_abi.c | 66 ++++++++++++++++++++++--------- 2 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index 04ae58892608..f52b1cf20eab 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -175,6 +175,7 @@ struct optee { bool scan_bus_done; struct workqueue_struct *scan_bus_wq; struct work_struct scan_bus_work;
struct kref refcnt;};
struct optee_session { diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index a1c1fa1a9c28..4fbec2acc255 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -1077,18 +1077,61 @@ static void optee_get_version(struct tee_device *teedev, *vers = v; }
+static void optee_smc_delete(struct kref *kref) +{
struct optee *optee = container_of(kref, struct optee, refcnt);/** Ask OP-TEE to free all cached shared memory objects to decrease* reference counters and also avoid wild pointers in secure world* into the old shared memory range.*/if (!optee->rpc_param_count)optee_disable_shm_cache(optee);optee_smc_notif_uninit_irq(optee);optee_remove_common(optee);if (optee->smc.memremaped_shm)memunmap(optee->smc.memremaped_shm);kfree(optee);+}
+static void optee_smc_release_supp(struct tee_context *ctx) +{
struct optee *optee = tee_get_drvdata(ctx->teedev);optee_release_helper(ctx, optee_close_session_helper);if (optee->scan_bus_wq) {destroy_workqueue(optee->scan_bus_wq);optee->scan_bus_wq = NULL;}optee_supp_release(&optee->supp);kref_put(&optee->refcnt, optee_smc_delete);+}
+static void optee_smc_release(struct tee_context *ctx) +{
struct optee *optee = tee_get_drvdata(ctx->teedev);optee_release_helper(ctx, optee_close_session_helper);kref_put(&optee->refcnt, optee_smc_delete);+}
static int optee_smc_open(struct tee_context *ctx) { struct optee *optee = tee_get_drvdata(ctx->teedev); u32 sec_caps = optee->smc.sec_caps;
kref_get(&optee->refcnt); return optee_open(ctx, sec_caps & OPTEE_SMC_SEC_CAP_MEMREF_NULL);}
static const struct tee_driver_ops optee_clnt_ops = { .get_version = optee_get_version, .open = optee_smc_open,
.release = optee_release,
.release = optee_smc_release, .open_session = optee_open_session, .close_session = optee_close_session, .invoke_func = optee_invoke_func,@@ -1106,7 +1149,7 @@ static const struct tee_desc optee_clnt_desc = { static const struct tee_driver_ops optee_supp_ops = { .get_version = optee_get_version, .open = optee_smc_open,
.release = optee_release_supp,
.release = optee_smc_release_supp, .supp_recv = optee_supp_recv, .supp_send = optee_supp_send, .shm_register = optee_shm_register_supp,@@ -1319,22 +1362,7 @@ static int optee_smc_remove(struct platform_device *pdev) { struct optee *optee = platform_get_drvdata(pdev);
/** Ask OP-TEE to free all cached shared memory objects to decrease* reference counters and also avoid wild pointers in secure world* into the old shared memory range.*/if (!optee->rpc_param_count)optee_disable_shm_cache(optee);optee_smc_notif_uninit_irq(optee);optee_remove_common(optee);if (optee->smc.memremaped_shm)memunmap(optee->smc.memremaped_shm);kfree(optee);
kref_put(&optee->refcnt, optee_smc_delete); return 0;}
2.39.0
Hi Yoochan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master] [also build test ERROR on v6.2-rc1 next-20221226] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/tee-optee-smc_abi... patch link: https://lore.kernel.org/r/20221231051954.2038772-1-yoochan1026%40gmail.com patch subject: [PATCH] tee: optee: smc_abi: Fix use-after-free in optee_smc_open config: arm-randconfig-c002-20221231 compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project f5700e7b69048de958172fb513b336564e7f8709) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm cross compiling tool for clang build # apt-get install binutils-arm-linux-gnueabi # https://github.com/intel-lab-lkp/linux/commit/6d3d5f78a2463a577c1a908cdedda6... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Yoochan-Lee/tee-optee-smc_abi-Fix-use-after-free-in-optee_smc_open/20221231-132046 git checkout 6d3d5f78a2463a577c1a908cdedda61affba9c01 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/tee/optee/
If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/tee/optee/smc_abi.c:1106:2: error: call to undeclared function 'optee_release_helper'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
optee_release_helper(ctx, optee_close_session_helper); ^ drivers/tee/optee/smc_abi.c:1106:2: note: did you mean 'optee_release_supp'? drivers/tee/optee/optee_private.h:258:6: note: 'optee_release_supp' declared here void optee_release_supp(struct tee_context *ctx); ^ drivers/tee/optee/smc_abi.c:1119:2: error: call to undeclared function 'optee_release_helper'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] optee_release_helper(ctx, optee_close_session_helper); ^ 2 errors generated.
vim +/optee_release_helper +1106 drivers/tee/optee/smc_abi.c
1101 1102 static void optee_smc_release_supp(struct tee_context *ctx) 1103 { 1104 struct optee *optee = tee_get_drvdata(ctx->teedev); 1105
1106 optee_release_helper(ctx, optee_close_session_helper);
1107 if (optee->scan_bus_wq) { 1108 destroy_workqueue(optee->scan_bus_wq); 1109 optee->scan_bus_wq = NULL; 1110 } 1111 optee_supp_release(&optee->supp); 1112 kref_put(&optee->refcnt, optee_smc_delete); 1113 } 1114
I fix the errors in the previous patch.
From 2046cc19aeaddb5f6fb5e9b1d9a380a116892657 Mon Sep 17 00:00:00 2001 From: Yoochan Lee yoochan1026@gmail.com Date: Mon, 2 Jan 2023 09:18:23 +0900 Subject: [PATCH 2/2] fix errors in previous patch
--- drivers/tee/optee/smc_abi.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index 4fbec2acc255..be662f263f29 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -1103,12 +1103,7 @@ static void optee_smc_release_supp(struct tee_context *ctx) { struct optee *optee = tee_get_drvdata(ctx->teedev);
- optee_release_helper(ctx, optee_close_session_helper); - if (optee->scan_bus_wq) { - destroy_workqueue(optee->scan_bus_wq); - optee->scan_bus_wq = NULL; - } - optee_supp_release(&optee->supp); + optee_release_supp(ctx) kref_put(&optee->refcnt, optee_smc_delete); }
@@ -1116,7 +1111,7 @@ static void optee_smc_release(struct tee_context *ctx) { struct optee *optee = tee_get_drvdata(ctx->teedev);
- optee_release_helper(ctx, optee_close_session_helper); + optee_release(ctx); kref_put(&optee->refcnt, optee_smc_delete); }
-- 2.39.0
2023년 1월 2일 (월) 오전 6:43, kernel test robot lkp@intel.com님이 작성:
Hi Yoochan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master] [also build test ERROR on v6.2-rc1 next-20221226] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/tee-optee-smc_abi... patch link: https://lore.kernel.org/r/20221231051954.2038772-1-yoochan1026%40gmail.com patch subject: [PATCH] tee: optee: smc_abi: Fix use-after-free in optee_smc_open config: arm-randconfig-c002-20221231 compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project f5700e7b69048de958172fb513b336564e7f8709) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm cross compiling tool for clang build # apt-get install binutils-arm-linux-gnueabi # https://github.com/intel-lab-lkp/linux/commit/6d3d5f78a2463a577c1a908cdedda6... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Yoochan-Lee/tee-optee-smc_abi-Fix-use-after-free-in-optee_smc_open/20221231-132046 git checkout 6d3d5f78a2463a577c1a908cdedda61affba9c01 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/tee/optee/
If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/tee/optee/smc_abi.c:1106:2: error: call to undeclared function 'optee_release_helper'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
optee_release_helper(ctx, optee_close_session_helper); ^drivers/tee/optee/smc_abi.c:1106:2: note: did you mean 'optee_release_supp'? drivers/tee/optee/optee_private.h:258:6: note: 'optee_release_supp' declared here void optee_release_supp(struct tee_context *ctx); ^ drivers/tee/optee/smc_abi.c:1119:2: error: call to undeclared function 'optee_release_helper'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] optee_release_helper(ctx, optee_close_session_helper); ^ 2 errors generated.
vim +/optee_release_helper +1106 drivers/tee/optee/smc_abi.c
1101 1102 static void optee_smc_release_supp(struct tee_context *ctx) 1103 { 1104 struct optee *optee = tee_get_drvdata(ctx->teedev); 1105
1106 optee_release_helper(ctx, optee_close_session_helper);
1107 if (optee->scan_bus_wq) { 1108 destroy_workqueue(optee->scan_bus_wq); 1109 optee->scan_bus_wq = NULL; 1110 } 1111 optee_supp_release(&optee->supp); 1112 kref_put(&optee->refcnt, optee_smc_delete); 1113 } 1114
-- 0-DAY CI Kernel Test Service https://01.org/lkp
Hi,
On Mon, Jan 2, 2023 at 1:18 AM Yoochan Lee yoochan1026@gmail.com wrote:
I fix the errors in the previous patch.
From 2046cc19aeaddb5f6fb5e9b1d9a380a116892657 Mon Sep 17 00:00:00 2001 From: Yoochan Lee yoochan1026@gmail.com Date: Mon, 2 Jan 2023 09:18:23 +0900 Subject: [PATCH 2/2] fix errors in previous patch
drivers/tee/optee/smc_abi.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
Please fix up the original patch and post it as a v2. By the way, have you tried to test this in some way?
Thanks, Jens
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index 4fbec2acc255..be662f263f29 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -1103,12 +1103,7 @@ static void optee_smc_release_supp(struct tee_context *ctx) { struct optee *optee = tee_get_drvdata(ctx->teedev);
- optee_release_helper(ctx, optee_close_session_helper);
- if (optee->scan_bus_wq) {
- destroy_workqueue(optee->scan_bus_wq);
- optee->scan_bus_wq = NULL;
- }
- optee_supp_release(&optee->supp);
- optee_release_supp(ctx) kref_put(&optee->refcnt, optee_smc_delete);
}
@@ -1116,7 +1111,7 @@ static void optee_smc_release(struct tee_context *ctx) { struct optee *optee = tee_get_drvdata(ctx->teedev);
- optee_release_helper(ctx, optee_close_session_helper);
- optee_release(ctx); kref_put(&optee->refcnt, optee_smc_delete);
}
-- 2.39.0
2023년 1월 2일 (월) 오전 6:43, kernel test robot lkp@intel.com님이 작성:
Hi Yoochan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master] [also build test ERROR on v6.2-rc1 next-20221226] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/tee-optee-smc_abi... patch link: https://lore.kernel.org/r/20221231051954.2038772-1-yoochan1026%40gmail.com patch subject: [PATCH] tee: optee: smc_abi: Fix use-after-free in optee_smc_open config: arm-randconfig-c002-20221231 compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project f5700e7b69048de958172fb513b336564e7f8709) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm cross compiling tool for clang build # apt-get install binutils-arm-linux-gnueabi # https://github.com/intel-lab-lkp/linux/commit/6d3d5f78a2463a577c1a908cdedda6... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Yoochan-Lee/tee-optee-smc_abi-Fix-use-after-free-in-optee_smc_open/20221231-132046 git checkout 6d3d5f78a2463a577c1a908cdedda61affba9c01 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/tee/optee/
If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/tee/optee/smc_abi.c:1106:2: error: call to undeclared function 'optee_release_helper'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
optee_release_helper(ctx, optee_close_session_helper); ^drivers/tee/optee/smc_abi.c:1106:2: note: did you mean 'optee_release_supp'? drivers/tee/optee/optee_private.h:258:6: note: 'optee_release_supp' declared here void optee_release_supp(struct tee_context *ctx); ^ drivers/tee/optee/smc_abi.c:1119:2: error: call to undeclared function 'optee_release_helper'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] optee_release_helper(ctx, optee_close_session_helper); ^ 2 errors generated.
vim +/optee_release_helper +1106 drivers/tee/optee/smc_abi.c
1101 1102 static void optee_smc_release_supp(struct tee_context *ctx) 1103 { 1104 struct optee *optee = tee_get_drvdata(ctx->teedev); 1105
1106 optee_release_helper(ctx, optee_close_session_helper);
1107 if (optee->scan_bus_wq) { 1108 destroy_workqueue(optee->scan_bus_wq); 1109 optee->scan_bus_wq = NULL; 1110 } 1111 optee_supp_release(&optee->supp); 1112 kref_put(&optee->refcnt, optee_smc_delete); 1113 } 1114
-- 0-DAY CI Kernel Test Service https://01.org/lkp
op-tee@lists.trustedfirmware.org
-
Jens Wiklander -
kernel test robot -
Yoochan Lee