Adding a partially initialized struct tee_shm to teedev->idr makes that object reachable from userspace through TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE. We should delay the registration of the pointer in the idr until the object is fully initialized.
Fixes: 967c9cca2cc5 ("tee: generic TEE subsystem") Reported-by: Patrik Lantz patrik.lantz@axis.com Signed-off-by: Lars Persson larper@axis.com --- drivers/tee/tee_shm.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 8a9384a64f3e..93814ff7fde8 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -155,7 +155,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
mutex_lock(&teedev->mutex); - shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL); + shm->id = idr_alloc(&teedev->idr, NULL, 1, 0, GFP_KERNEL); mutex_unlock(&teedev->mutex); if (shm->id < 0) { ret = ERR_PTR(shm->id); @@ -172,6 +172,12 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) ret = ERR_CAST(shm->dmabuf); goto err_rem; } + + ret = idr_replace(&teedev->idr, shm, shm->id); + if (IS_ERR(ret)) { + dma_buf_put(shm->dmabuf); + return ret; + } }
teedev_ctx_get(ctx); @@ -288,7 +294,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, }
mutex_lock(&teedev->mutex); - shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL); + shm->id = idr_alloc(&teedev->idr, NULL, 1, 0, GFP_KERNEL); mutex_unlock(&teedev->mutex);
if (shm->id < 0) { @@ -319,6 +325,12 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, } }
+ ret = idr_replace(&teedev->idr, shm, shm->id); + if (IS_ERR(ret)) { + tee_shm_free(shm); + return ret; + } + return shm; err: if (shm) {
On Tue, 7 Dec 2021 at 15:28, Lars Persson larper@axis.com wrote:
Adding a partially initialized struct tee_shm to teedev->idr makes that object reachable from userspace through TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE.
Can you elaborate here how this partially initialized shared memory is reachable from user-space? AFAICT, TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE relies on shm->id to access underlying shared memory (via tee_shm_get_from_id()) and that is only known to user-space after TEE_IOC_SHM_ALLOC or TEE_IOC_SHM_REGISTER return successfully.
-Sumit
We should delay the registration of the pointer in the idr until the object is fully initialized.
Fixes: 967c9cca2cc5 ("tee: generic TEE subsystem") Reported-by: Patrik Lantz patrik.lantz@axis.com Signed-off-by: Lars Persson larper@axis.com
drivers/tee/tee_shm.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 8a9384a64f3e..93814ff7fde8 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -155,7 +155,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
mutex_lock(&teedev->mutex);
shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
shm->id = idr_alloc(&teedev->idr, NULL, 1, 0, GFP_KERNEL); mutex_unlock(&teedev->mutex); if (shm->id < 0) { ret = ERR_PTR(shm->id);@@ -172,6 +172,12 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) ret = ERR_CAST(shm->dmabuf); goto err_rem; }
ret = idr_replace(&teedev->idr, shm, shm->id);if (IS_ERR(ret)) {dma_buf_put(shm->dmabuf);return ret;} } teedev_ctx_get(ctx);@@ -288,7 +294,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, }
mutex_lock(&teedev->mutex);
shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
shm->id = idr_alloc(&teedev->idr, NULL, 1, 0, GFP_KERNEL); mutex_unlock(&teedev->mutex); if (shm->id < 0) {@@ -319,6 +325,12 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, } }
ret = idr_replace(&teedev->idr, shm, shm->id);if (IS_ERR(ret)) {tee_shm_free(shm);return ret;}return shm;err: if (shm) { -- 2.30.2
On 2021-12-09 13:00, Sumit Garg wrote:
On Tue, 7 Dec 2021 at 15:28, Lars Persson larper@axis.com wrote:
Adding a partially initialized struct tee_shm to teedev->idr makes that object reachable from userspace through TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE.
Can you elaborate here how this partially initialized shared memory is reachable from user-space? AFAICT, TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE relies on shm->id to access underlying shared memory (via tee_shm_get_from_id()) and that is only known to user-space after TEE_IOC_SHM_ALLOC or TEE_IOC_SHM_REGISTER return successfully.
Hi
The IDR does not hand out high-entropy identifiers that userspace is unable to guess. We can very easily provoke NULL pointer dereferences in the kernel when running syzkaller on the TEE device because shm->dmabuf has not yet been assigned.
It is also a general best practice that an object shall only be available through an API once it is fully initialized. Else undefined behavior will come back and bite us.
BR, Lars
On Fri, 10 Dec 2021 at 15:54, Lars Persson larper@axis.com wrote:
On 2021-12-09 13:00, Sumit Garg wrote:
On Tue, 7 Dec 2021 at 15:28, Lars Persson larper@axis.com wrote:
Adding a partially initialized struct tee_shm to teedev->idr makes that object reachable from userspace through TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE.
Can you elaborate here how this partially initialized shared memory is reachable from user-space? AFAICT, TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE relies on shm->id to access underlying shared memory (via tee_shm_get_from_id()) and that is only known to user-space after TEE_IOC_SHM_ALLOC or TEE_IOC_SHM_REGISTER return successfully.
Hi
The IDR does not hand out high-entropy identifiers that userspace is unable to guess. We can very easily provoke NULL pointer dereferences in the kernel when running syzkaller on the TEE device because shm->dmabuf has not yet been assigned.
It is also a general best practice that an object shall only be available through an API once it is fully initialized. Else undefined behavior will come back and bite us.
Okay, I see the fuzzing scenario where a kernel crash can be caused. So I take this fix as user-space shouldn't be able to crash the kernel.
Please do add all these details to the commit description along with the possible kernel crash trace.
-Sumit
BR, Lars
On 2021-12-10 11:50, Sumit Garg wrote:
On Fri, 10 Dec 2021 at 15:54, Lars Persson larper@axis.com wrote:
On 2021-12-09 13:00, Sumit Garg wrote:
On Tue, 7 Dec 2021 at 15:28, Lars Persson larper@axis.com wrote:
Adding a partially initialized struct tee_shm to teedev->idr makes that object reachable from userspace through TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE.
Can you elaborate here how this partially initialized shared memory is reachable from user-space? AFAICT, TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE relies on shm->id to access underlying shared memory (via tee_shm_get_from_id()) and that is only known to user-space after TEE_IOC_SHM_ALLOC or TEE_IOC_SHM_REGISTER return successfully.
Hi
The IDR does not hand out high-entropy identifiers that userspace is unable to guess. We can very easily provoke NULL pointer dereferences in the kernel when running syzkaller on the TEE device because shm->dmabuf has not yet been assigned.
It is also a general best practice that an object shall only be available through an API once it is fully initialized. Else undefined behavior will come back and bite us.
Okay, I see the fuzzing scenario where a kernel crash can be caused. So I take this fix as user-space shouldn't be able to crash the kernel.
Please do add all these details to the commit description along with the possible kernel crash trace.
After applying "[PATCH] tee: handle lookup of shm with reference count 0" my patch is no longer relevant. I will check again if there is any race condition left after the dmabuf was removed and submit a new version if needed.
On Thu, Dec 16, 2021 at 10:51 AM Lars Persson larper@axis.com wrote:
On 2021-12-10 11:50, Sumit Garg wrote:
On Fri, 10 Dec 2021 at 15:54, Lars Persson larper@axis.com wrote:
On 2021-12-09 13:00, Sumit Garg wrote:
On Tue, 7 Dec 2021 at 15:28, Lars Persson larper@axis.com wrote:
Adding a partially initialized struct tee_shm to teedev->idr makes that object reachable from userspace through TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE.
Can you elaborate here how this partially initialized shared memory is reachable from user-space? AFAICT, TEE_IOC_OPEN_SESSION and TEE_IOC_INVOKE relies on shm->id to access underlying shared memory (via tee_shm_get_from_id()) and that is only known to user-space after TEE_IOC_SHM_ALLOC or TEE_IOC_SHM_REGISTER return successfully.
Hi
The IDR does not hand out high-entropy identifiers that userspace is unable to guess. We can very easily provoke NULL pointer dereferences in the kernel when running syzkaller on the TEE device because shm->dmabuf has not yet been assigned.
It is also a general best practice that an object shall only be available through an API once it is fully initialized. Else undefined behavior will come back and bite us.
Okay, I see the fuzzing scenario where a kernel crash can be caused. So I take this fix as user-space shouldn't be able to crash the kernel.
Please do add all these details to the commit description along with the possible kernel crash trace.
After applying "[PATCH] tee: handle lookup of shm with reference count 0" my patch is no longer relevant. I will check again if there is any race condition left after the dmabuf was removed and submit a new version if needed.
Isn't it still relevant in tee_shm_register()?
Thanks, Jens
op-tee@lists.trustedfirmware.org
-
Jens Wiklander -
Lars Persson -
Sumit Garg