commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
With special lengths supplied by user space, tee_shm_register() has an integer overflow when calculating the number of pages covered by a supplied user space memory region.
This may cause pin_user_pages_fast() to do a NULL pointer dereference.
Fix this by adding an an explicit call to access_ok() in tee_ioctl_shm_register() to catch an invalid user space address early.
Fixes: 033ddf12bcf5 ("tee: add register user memory") Cc: stable@vger.kernel.org # 5.4 Cc: stable@vger.kernel.org # 5.10 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: Linus Torvalds torvalds@linux-foundation.org [JW: backport to stable 5.4 and 5.10 + update commit message] Signed-off-by: Jens Wiklander jens.wiklander@linaro.org --- drivers/tee/tee_core.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index a7ccd4d2bd10..2db144d2d26f 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx, if (data.flags) return -EINVAL;
+ if (!access_ok((void __user *)(unsigned long)data.addr, data.length)) + return -EFAULT; + shm = tee_shm_register(ctx, data.addr, data.length, TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED); if (IS_ERR(shm))
On Mon, Aug 22, 2022 at 03:12:27PM +0200, Jens Wiklander wrote:
commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
With special lengths supplied by user space, tee_shm_register() has an integer overflow when calculating the number of pages covered by a supplied user space memory region.
This may cause pin_user_pages_fast() to do a NULL pointer dereference.
Fix this by adding an an explicit call to access_ok() in tee_ioctl_shm_register() to catch an invalid user space address early.
Fixes: 033ddf12bcf5 ("tee: add register user memory") Cc: stable@vger.kernel.org # 5.4 Cc: stable@vger.kernel.org # 5.10 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: Linus Torvalds torvalds@linux-foundation.org [JW: backport to stable 5.4 and 5.10 + update commit message]
You already sent me a 5.4 version here: https://lore.kernel.org/r/20220822092621.3691771-1-jens.wiklander@linaro.org
And I applied that.
And for 5.10, it's already in the tree as commit 578c349570d2 ("tee: add overflow check in register_shm_helper()") and was in the 5.10.137 release.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
drivers/tee/tee_core.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index a7ccd4d2bd10..2db144d2d26f 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx, if (data.flags) return -EINVAL;
- if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
return -EFAULT;
What I took in 5.10.137 was:
+ if (!access_ok((void __user *)addr, length)) + return ERR_PTR(-EFAULT);
Should I fix it up to look like what you sent here instead?
confused,
greg k-h
On Mon, Aug 22, 2022 at 3:32 PM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Aug 22, 2022 at 03:12:27PM +0200, Jens Wiklander wrote:
commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
With special lengths supplied by user space, tee_shm_register() has an integer overflow when calculating the number of pages covered by a supplied user space memory region.
This may cause pin_user_pages_fast() to do a NULL pointer dereference.
Fix this by adding an an explicit call to access_ok() in tee_ioctl_shm_register() to catch an invalid user space address early.
Fixes: 033ddf12bcf5 ("tee: add register user memory") Cc: stable@vger.kernel.org # 5.4 Cc: stable@vger.kernel.org # 5.10 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: Linus Torvalds torvalds@linux-foundation.org [JW: backport to stable 5.4 and 5.10 + update commit message]
You already sent me a 5.4 version here: https://lore.kernel.org/r/20220822092621.3691771-1-jens.wiklander@linaro.org
And I applied that.
And for 5.10, it's already in the tree as commit 578c349570d2 ("tee: add overflow check in register_shm_helper()") and was in the 5.10.137 release.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
drivers/tee/tee_core.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index a7ccd4d2bd10..2db144d2d26f 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx, if (data.flags) return -EINVAL;
if (!access_ok((void __user *)(unsigned long)data.addr, data.length))return -EFAULT;What I took in 5.10.137 was:
if (!access_ok((void __user *)addr, length))return ERR_PTR(-EFAULT);Should I fix it up to look like what you sent here instead?
Yes, please.
confused,
I'm sorry for the confusion.
Thanks, Jens
greg k-h
On Mon, Aug 22, 2022 at 04:29:05PM +0200, Jens Wiklander wrote:
On Mon, Aug 22, 2022 at 3:32 PM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Aug 22, 2022 at 03:12:27PM +0200, Jens Wiklander wrote:
commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
With special lengths supplied by user space, tee_shm_register() has an integer overflow when calculating the number of pages covered by a supplied user space memory region.
This may cause pin_user_pages_fast() to do a NULL pointer dereference.
Fix this by adding an an explicit call to access_ok() in tee_ioctl_shm_register() to catch an invalid user space address early.
Fixes: 033ddf12bcf5 ("tee: add register user memory") Cc: stable@vger.kernel.org # 5.4 Cc: stable@vger.kernel.org # 5.10 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: Linus Torvalds torvalds@linux-foundation.org [JW: backport to stable 5.4 and 5.10 + update commit message]
You already sent me a 5.4 version here: https://lore.kernel.org/r/20220822092621.3691771-1-jens.wiklander@linaro.org
And I applied that.
And for 5.10, it's already in the tree as commit 578c349570d2 ("tee: add overflow check in register_shm_helper()") and was in the 5.10.137 release.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
drivers/tee/tee_core.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index a7ccd4d2bd10..2db144d2d26f 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx, if (data.flags) return -EINVAL;
if (!access_ok((void __user *)(unsigned long)data.addr, data.length))return -EFAULT;What I took in 5.10.137 was:
if (!access_ok((void __user *)addr, length))return ERR_PTR(-EFAULT);Should I fix it up to look like what you sent here instead?
Yes, please.
Ok, no, that does not work on 5.10.y at all, it blows up with the obvious issue that there is no data pointer in this function. It's also in a different file, drivers/tee/tee_shm.c
So I'm going to leave 5.10.y alone for now, I think it's fixed.
I've taken your 5.4 patch that you sent previously, and still need a 4.19.y change (and any older kernels that are also vulnerable.)
thanks,
greg k-h
On Mon, Aug 22, 2022 at 4:57 PM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Aug 22, 2022 at 04:29:05PM +0200, Jens Wiklander wrote:
On Mon, Aug 22, 2022 at 3:32 PM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Aug 22, 2022 at 03:12:27PM +0200, Jens Wiklander wrote:
commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
With special lengths supplied by user space, tee_shm_register() has an integer overflow when calculating the number of pages covered by a supplied user space memory region.
This may cause pin_user_pages_fast() to do a NULL pointer dereference.
Fix this by adding an an explicit call to access_ok() in tee_ioctl_shm_register() to catch an invalid user space address early.
Fixes: 033ddf12bcf5 ("tee: add register user memory") Cc: stable@vger.kernel.org # 5.4 Cc: stable@vger.kernel.org # 5.10 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: Linus Torvalds torvalds@linux-foundation.org [JW: backport to stable 5.4 and 5.10 + update commit message]
You already sent me a 5.4 version here: https://lore.kernel.org/r/20220822092621.3691771-1-jens.wiklander@linaro.org
And I applied that.
And for 5.10, it's already in the tree as commit 578c349570d2 ("tee: add overflow check in register_shm_helper()") and was in the 5.10.137 release.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
drivers/tee/tee_core.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index a7ccd4d2bd10..2db144d2d26f 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx, if (data.flags) return -EINVAL;
if (!access_ok((void __user *)(unsigned long)data.addr, data.length))return -EFAULT;What I took in 5.10.137 was:
if (!access_ok((void __user *)addr, length))return ERR_PTR(-EFAULT);Should I fix it up to look like what you sent here instead?
Yes, please.
Ok, no, that does not work on 5.10.y at all, it blows up with the obvious issue that there is no data pointer in this function. It's also in a different file, drivers/tee/tee_shm.c
So I'm going to leave 5.10.y alone for now, I think it's fixed.
It works somewhat, but there's the potential memory leak that Pavel Machek pointed out, https://lore.kernel.org/lkml/20220822111546.GA7795@duo.ucw.cz/ .
The 5.4 patch has a better approach since it verifies the supplied address range early before we do anything that must be undone on error. The 5.4 patch changes tee_ioctl_shm_register() instead, which is the function one step up in the call chain. This approach should be taken for all kernels before 53e16519c2ec ("tee: replace tee_shm_register()"), that is, before v5.18 if I'm reading the git log correctly.
access_ok() went from taking three arguments to two sometime after v4.19, why that patch is slightly different.
I've taken your 5.4 patch that you sent previously, and still need a 4.19.y change (and any older kernels that are also vulnerable.)
The oldest vulnerable kernel is v4.16.
Thanks, Jens
thanks,
greg k-h
On Tue, Aug 23, 2022 at 09:00:43AM +0200, Jens Wiklander wrote:
On Mon, Aug 22, 2022 at 4:57 PM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Aug 22, 2022 at 04:29:05PM +0200, Jens Wiklander wrote:
On Mon, Aug 22, 2022 at 3:32 PM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Aug 22, 2022 at 03:12:27PM +0200, Jens Wiklander wrote:
commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
With special lengths supplied by user space, tee_shm_register() has an integer overflow when calculating the number of pages covered by a supplied user space memory region.
This may cause pin_user_pages_fast() to do a NULL pointer dereference.
Fix this by adding an an explicit call to access_ok() in tee_ioctl_shm_register() to catch an invalid user space address early.
Fixes: 033ddf12bcf5 ("tee: add register user memory") Cc: stable@vger.kernel.org # 5.4 Cc: stable@vger.kernel.org # 5.10 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: Linus Torvalds torvalds@linux-foundation.org [JW: backport to stable 5.4 and 5.10 + update commit message]
You already sent me a 5.4 version here: https://lore.kernel.org/r/20220822092621.3691771-1-jens.wiklander@linaro.org
And I applied that.
And for 5.10, it's already in the tree as commit 578c349570d2 ("tee: add overflow check in register_shm_helper()") and was in the 5.10.137 release.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
drivers/tee/tee_core.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index a7ccd4d2bd10..2db144d2d26f 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx, if (data.flags) return -EINVAL;
if (!access_ok((void __user *)(unsigned long)data.addr, data.length))return -EFAULT;What I took in 5.10.137 was:
if (!access_ok((void __user *)addr, length))return ERR_PTR(-EFAULT);Should I fix it up to look like what you sent here instead?
Yes, please.
Ok, no, that does not work on 5.10.y at all, it blows up with the obvious issue that there is no data pointer in this function. It's also in a different file, drivers/tee/tee_shm.c
So I'm going to leave 5.10.y alone for now, I think it's fixed.
It works somewhat, but there's the potential memory leak that Pavel Machek pointed out, https://lore.kernel.org/lkml/20220822111546.GA7795@duo.ucw.cz/ .
The 5.4 patch has a better approach since it verifies the supplied address range early before we do anything that must be undone on error. The 5.4 patch changes tee_ioctl_shm_register() instead, which is the function one step up in the call chain. This approach should be taken for all kernels before 53e16519c2ec ("tee: replace tee_shm_register()"), that is, before v5.18 if I'm reading the git log correctly.
access_ok() went from taking three arguments to two sometime after v4.19, why that patch is slightly different.
Ok, can you send me a new fix-up patch, on top of the latest 5.10.y release, to resolve the 5.10 issue?
thanks,
greg k-h
Hi Greg,
On Mon, 22 Aug 2022 at 18:42, Jens Wiklander jens.wiklander@linaro.org wrote:
commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
With special lengths supplied by user space, tee_shm_register() has an integer overflow when calculating the number of pages covered by a supplied user space memory region.
This may cause pin_user_pages_fast() to do a NULL pointer dereference.
Fix this by adding an an explicit call to access_ok() in tee_ioctl_shm_register() to catch an invalid user space address early.
Fixes: 033ddf12bcf5 ("tee: add register user memory") Cc: stable@vger.kernel.org # 5.4 Cc: stable@vger.kernel.org # 5.10 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: Linus Torvalds torvalds@linux-foundation.org [JW: backport to stable 5.4 and 5.10 + update commit message] Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
drivers/tee/tee_core.c | 3 +++ 1 file changed, 3 insertions(+)
The v5.15 backport [1] for this fix has broken the kernel consumers for tee_shm_register(), the trusted keys driver is one of them reported here [2]. We need to fix that up with the following change [3]. Would you like to revert the backport and apply the correct one or should I prepare a fix patch for the following [3]?
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=l...
[2] https://github.com/OP-TEE/optee_os/issues/5624
[3]
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index 3fc426dad2df..d5c4fcd8733d 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -334,6 +334,9 @@ tee_ioctl_shm_register(struct tee_context *ctx, if (data.flags) return -EINVAL;
+ if (!access_ok((void __user *)data.addr, data.length)) + return ERR_PTR(-EFAULT); + shm = tee_shm_register(ctx, data.addr, data.length, TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED); if (IS_ERR(shm)) diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index bd96ebb82c8e..6fb4400333fb 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -223,9 +223,6 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, goto err; }
- if (!access_ok((void __user *)addr, length)) - return ERR_PTR(-EFAULT); - mutex_lock(&teedev->mutex); shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL); mutex_unlock(&teedev->mutex);
-Sumit
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index a7ccd4d2bd10..2db144d2d26f 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx, if (data.flags) return -EINVAL;
if (!access_ok((void __user *)(unsigned long)data.addr, data.length))return -EFAULT;shm = tee_shm_register(ctx, data.addr, data.length, TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED); if (IS_ERR(shm))-- 2.31.1
On Tue, Nov 08, 2022 at 11:42:01AM +0530, Sumit Garg wrote:
Hi Greg,
On Mon, 22 Aug 2022 at 18:42, Jens Wiklander jens.wiklander@linaro.org wrote:
commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
With special lengths supplied by user space, tee_shm_register() has an integer overflow when calculating the number of pages covered by a supplied user space memory region.
This may cause pin_user_pages_fast() to do a NULL pointer dereference.
Fix this by adding an an explicit call to access_ok() in tee_ioctl_shm_register() to catch an invalid user space address early.
Fixes: 033ddf12bcf5 ("tee: add register user memory") Cc: stable@vger.kernel.org # 5.4 Cc: stable@vger.kernel.org # 5.10 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: Linus Torvalds torvalds@linux-foundation.org [JW: backport to stable 5.4 and 5.10 + update commit message] Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
drivers/tee/tee_core.c | 3 +++ 1 file changed, 3 insertions(+)
The v5.15 backport [1] for this fix has broken the kernel consumers for tee_shm_register(), the trusted keys driver is one of them reported here [2]. We need to fix that up with the following change [3]. Would you like to revert the backport and apply the correct one or should I prepare a fix patch for the following [3]?
A fixup patch is fine if needed, along with the description of why the backport was broken. Note, this commit went much further back than 5.15, so be sure to check older kernels too.
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Greg Kroah-Hartman
-
Jens Wiklander -
Sumit Garg