- OP-TEE - lists.trustedfirmware.org

Issue with optee fixing supplicant wait loop

by Amirreza Zarrabi

Hi Jens, Sumit, The following change has recently been merged: tee: optee: Fix supplicant wait loop (https://github.com/torvalds/linux/commit/70b0d6b0a199c5a3ee6c72f5e61681ed6f…). This fix addresses the issue where the supplicant may terminate before the client due to the shutdown sequence or a crash. However, it introduces a potential new issue - now that the client is killable, what happens if it terminates before the supplicant? During cleanup, the client would remove/free the request, while the request would still be available in the request idr for the supplicant. Although I have not been able to reproduce it, this could theoretically result in a kernel page fault. To address this, there are a few options: (1) Use rcu and access request under a read lock in optee_supp_recv and optee_supp_send. (2) Reuse supp->mutex, ensuring that most of these functions run within the lock, even though it may not always be necessary. Could you confirm whether this behavior was intentional? If not, I am happy to prepare a patch. Additionally, please let me know whether you prefer solution (1) or (2), or if you have any other suggestions. Thank you for your time. Regards, Amir

7 months, 1 week

2
2
0 0

[PATCH AUTOSEL 5.4 4/5] tee: Prevent size calculation wraparound on 32-bit kernels

by Sasha Levin

From: Jann Horn <jannh(a)google.com> [ Upstream commit 39bb67edcc582b3b386a9ec983da67fa8a10ec03 ] The current code around TEE_IOCTL_PARAM_SIZE() is a bit wrong on 32-bit kernels: Multiplying a user-provided 32-bit value with the size of a structure can wrap around on such platforms. Fix it by using saturating arithmetic for the size calculation. This has no security consequences because, in all users of TEE_IOCTL_PARAM_SIZE(), the subsequent kcalloc() implicitly checks for wrapping. Signed-off-by: Jann Horn <jannh(a)google.com> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> Tested-by: Rouven Czerwinski <rouven.czerwinski(a)linaro.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- **YES** This commit should be backported to stable kernel trees. Here's my extensive analysis: ## Vulnerability Analysis The commit fixes a real integer overflow vulnerability in the TEE (Trusted Execution Environment) subsystem on 32-bit kernels. The issue occurs in the `TEE_IOCTL_PARAM_SIZE()` macro defined as: ```c #define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) ``` Where `struct tee_ioctl_param` is 32 bytes (4 × 8-byte fields). On 32-bit systems, when a user provides a large `num_params` value, the multiplication `32 * num_params` can wrap around, potentially bypassing buffer length validation checks. ## Specific Vulnerable Code Locations The vulnerable pattern appears in 4 locations in `drivers/tee/tee_core.c`: 1. **Line 490**: `tee_ioctl_open_session()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 2. **Line 568**: `tee_ioctl_invoke()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 3. **Line 702**: `tee_ioctl_supp_recv()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len` 4. **Line 801**: `tee_ioctl_supp_send()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len` ## Attack Scenario On 32-bit systems, if `num_params = 0x08000000` (134,217,728), then: - `TEE_IOCTL_PARAM_SIZE(0x08000000) = 32 * 0x08000000 = 0x100000000` - This wraps to `0x00000000` on 32-bit systems - The size validation `sizeof(arg) + 0 != buf.buf_len` could be bypassed - Subsequent `kcalloc()` would allocate a huge buffer, but the size calculations would be wrong ## Why This Should Be Backported **Matches "YES" backport criteria from similar commits:** 1. **Security Fix**: Like Similar Commit #2 (rtl8188eu security hole) and #4 (gup_benchmark integer overflow), this prevents user-controlled integer overflow that could lead to memory corruption. 2. **Small, Contained Change**: The fix uses the kernel's standard `size_add()` and `size_mul()` helpers from `linux/overflow.h` - exactly like Similar Commit #3 which was noted as using `size_add()` for similar protection. 3. **Critical Subsystem**: TEE handles sensitive cryptographic operations and trusted applications, making security fixes here particularly important. 4. **Low Regression Risk**: The fix only adds overflow protection without changing functionality. The `size_mul()` and `size_add()` functions are well- tested kernel overflow helpers. 5. **User-Controllable Input**: The `num_params` value comes directly from user space through ioctl calls, making this a user-triggerable condition. 6. **Clear Security Impact**: While the commit states "no security consequences because kcalloc() implicitly checks for wrapping," this is defense-in-depth - the size validation should not rely on downstream allocation checks. The fix pattern exactly matches Similar Commit #4 (mm/gup_benchmark.c) which was backported for preventing integer overflow in size calculations with user-provided values. This commit follows the same security principle and uses the same overflow-safe arithmetic helpers that the kernel has standardized on for these scenarios. drivers/tee/tee_core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index 2db144d2d26f3..357944bc73b19 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -9,6 +9,7 @@ #include <linux/fs.h> #include <linux/idr.h> #include <linux/module.h> +#include <linux/overflow.h> #include <linux/slab.h> #include <linux/tee_drv.h> #include <linux/uaccess.h> @@ -16,7 +17,7 @@ #define TEE_NUM_DEVICES 32 -#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) +#define TEE_IOCTL_PARAM_SIZE(x) (size_mul(sizeof(struct tee_param), (x))) /* * Unprivileged devices in the lower half range and privileged devices in @@ -327,7 +328,7 @@ static int tee_ioctl_open_session(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -398,7 +399,7 @@ static int tee_ioctl_invoke(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -532,7 +533,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx, if (get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) != buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); @@ -631,7 +632,7 @@ static int tee_ioctl_supp_send(struct tee_context *ctx, get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) > buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); -- 2.39.5

7 months, 2 weeks

1
0
0 0

[PATCH AUTOSEL 5.10 4/5] tee: Prevent size calculation wraparound on 32-bit kernels

by Sasha Levin

From: Jann Horn <jannh(a)google.com> [ Upstream commit 39bb67edcc582b3b386a9ec983da67fa8a10ec03 ] The current code around TEE_IOCTL_PARAM_SIZE() is a bit wrong on 32-bit kernels: Multiplying a user-provided 32-bit value with the size of a structure can wrap around on such platforms. Fix it by using saturating arithmetic for the size calculation. This has no security consequences because, in all users of TEE_IOCTL_PARAM_SIZE(), the subsequent kcalloc() implicitly checks for wrapping. Signed-off-by: Jann Horn <jannh(a)google.com> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> Tested-by: Rouven Czerwinski <rouven.czerwinski(a)linaro.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- **YES** This commit should be backported to stable kernel trees. Here's my extensive analysis: ## Vulnerability Analysis The commit fixes a real integer overflow vulnerability in the TEE (Trusted Execution Environment) subsystem on 32-bit kernels. The issue occurs in the `TEE_IOCTL_PARAM_SIZE()` macro defined as: ```c #define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) ``` Where `struct tee_ioctl_param` is 32 bytes (4 × 8-byte fields). On 32-bit systems, when a user provides a large `num_params` value, the multiplication `32 * num_params` can wrap around, potentially bypassing buffer length validation checks. ## Specific Vulnerable Code Locations The vulnerable pattern appears in 4 locations in `drivers/tee/tee_core.c`: 1. **Line 490**: `tee_ioctl_open_session()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 2. **Line 568**: `tee_ioctl_invoke()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 3. **Line 702**: `tee_ioctl_supp_recv()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len` 4. **Line 801**: `tee_ioctl_supp_send()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len` ## Attack Scenario On 32-bit systems, if `num_params = 0x08000000` (134,217,728), then: - `TEE_IOCTL_PARAM_SIZE(0x08000000) = 32 * 0x08000000 = 0x100000000` - This wraps to `0x00000000` on 32-bit systems - The size validation `sizeof(arg) + 0 != buf.buf_len` could be bypassed - Subsequent `kcalloc()` would allocate a huge buffer, but the size calculations would be wrong ## Why This Should Be Backported **Matches "YES" backport criteria from similar commits:** 1. **Security Fix**: Like Similar Commit #2 (rtl8188eu security hole) and #4 (gup_benchmark integer overflow), this prevents user-controlled integer overflow that could lead to memory corruption. 2. **Small, Contained Change**: The fix uses the kernel's standard `size_add()` and `size_mul()` helpers from `linux/overflow.h` - exactly like Similar Commit #3 which was noted as using `size_add()` for similar protection. 3. **Critical Subsystem**: TEE handles sensitive cryptographic operations and trusted applications, making security fixes here particularly important. 4. **Low Regression Risk**: The fix only adds overflow protection without changing functionality. The `size_mul()` and `size_add()` functions are well- tested kernel overflow helpers. 5. **User-Controllable Input**: The `num_params` value comes directly from user space through ioctl calls, making this a user-triggerable condition. 6. **Clear Security Impact**: While the commit states "no security consequences because kcalloc() implicitly checks for wrapping," this is defense-in-depth - the size validation should not rely on downstream allocation checks. The fix pattern exactly matches Similar Commit #4 (mm/gup_benchmark.c) which was backported for preventing integer overflow in size calculations with user-provided values. This commit follows the same security principle and uses the same overflow-safe arithmetic helpers that the kernel has standardized on for these scenarios. drivers/tee/tee_core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index 9cc4a7b63b0d6..e6de0e80b793e 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -10,6 +10,7 @@ #include <linux/fs.h> #include <linux/idr.h> #include <linux/module.h> +#include <linux/overflow.h> #include <linux/slab.h> #include <linux/tee_drv.h> #include <linux/uaccess.h> @@ -19,7 +20,7 @@ #define TEE_NUM_DEVICES 32 -#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) +#define TEE_IOCTL_PARAM_SIZE(x) (size_mul(sizeof(struct tee_param), (x))) #define TEE_UUID_NS_NAME_SIZE 128 @@ -492,7 +493,7 @@ static int tee_ioctl_open_session(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -570,7 +571,7 @@ static int tee_ioctl_invoke(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -704,7 +705,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx, if (get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) != buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); @@ -803,7 +804,7 @@ static int tee_ioctl_supp_send(struct tee_context *ctx, get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) > buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); -- 2.39.5

7 months, 2 weeks

1
0
0 0

[PATCH AUTOSEL 5.15 4/5] tee: Prevent size calculation wraparound on 32-bit kernels

by Sasha Levin

From: Jann Horn <jannh(a)google.com> [ Upstream commit 39bb67edcc582b3b386a9ec983da67fa8a10ec03 ] The current code around TEE_IOCTL_PARAM_SIZE() is a bit wrong on 32-bit kernels: Multiplying a user-provided 32-bit value with the size of a structure can wrap around on such platforms. Fix it by using saturating arithmetic for the size calculation. This has no security consequences because, in all users of TEE_IOCTL_PARAM_SIZE(), the subsequent kcalloc() implicitly checks for wrapping. Signed-off-by: Jann Horn <jannh(a)google.com> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> Tested-by: Rouven Czerwinski <rouven.czerwinski(a)linaro.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- **YES** This commit should be backported to stable kernel trees. Here's my extensive analysis: ## Vulnerability Analysis The commit fixes a real integer overflow vulnerability in the TEE (Trusted Execution Environment) subsystem on 32-bit kernels. The issue occurs in the `TEE_IOCTL_PARAM_SIZE()` macro defined as: ```c #define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) ``` Where `struct tee_ioctl_param` is 32 bytes (4 × 8-byte fields). On 32-bit systems, when a user provides a large `num_params` value, the multiplication `32 * num_params` can wrap around, potentially bypassing buffer length validation checks. ## Specific Vulnerable Code Locations The vulnerable pattern appears in 4 locations in `drivers/tee/tee_core.c`: 1. **Line 490**: `tee_ioctl_open_session()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 2. **Line 568**: `tee_ioctl_invoke()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 3. **Line 702**: `tee_ioctl_supp_recv()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len` 4. **Line 801**: `tee_ioctl_supp_send()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len` ## Attack Scenario On 32-bit systems, if `num_params = 0x08000000` (134,217,728), then: - `TEE_IOCTL_PARAM_SIZE(0x08000000) = 32 * 0x08000000 = 0x100000000` - This wraps to `0x00000000` on 32-bit systems - The size validation `sizeof(arg) + 0 != buf.buf_len` could be bypassed - Subsequent `kcalloc()` would allocate a huge buffer, but the size calculations would be wrong ## Why This Should Be Backported **Matches "YES" backport criteria from similar commits:** 1. **Security Fix**: Like Similar Commit #2 (rtl8188eu security hole) and #4 (gup_benchmark integer overflow), this prevents user-controlled integer overflow that could lead to memory corruption. 2. **Small, Contained Change**: The fix uses the kernel's standard `size_add()` and `size_mul()` helpers from `linux/overflow.h` - exactly like Similar Commit #3 which was noted as using `size_add()` for similar protection. 3. **Critical Subsystem**: TEE handles sensitive cryptographic operations and trusted applications, making security fixes here particularly important. 4. **Low Regression Risk**: The fix only adds overflow protection without changing functionality. The `size_mul()` and `size_add()` functions are well- tested kernel overflow helpers. 5. **User-Controllable Input**: The `num_params` value comes directly from user space through ioctl calls, making this a user-triggerable condition. 6. **Clear Security Impact**: While the commit states "no security consequences because kcalloc() implicitly checks for wrapping," this is defense-in-depth - the size validation should not rely on downstream allocation checks. The fix pattern exactly matches Similar Commit #4 (mm/gup_benchmark.c) which was backported for preventing integer overflow in size calculations with user-provided values. This commit follows the same security principle and uses the same overflow-safe arithmetic helpers that the kernel has standardized on for these scenarios. drivers/tee/tee_core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index a44e5b53e7a91..a7e89c229fc51 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -10,6 +10,7 @@ #include <linux/fs.h> #include <linux/idr.h> #include <linux/module.h> +#include <linux/overflow.h> #include <linux/slab.h> #include <linux/tee_drv.h> #include <linux/uaccess.h> @@ -19,7 +20,7 @@ #define TEE_NUM_DEVICES 32 -#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) +#define TEE_IOCTL_PARAM_SIZE(x) (size_mul(sizeof(struct tee_param), (x))) #define TEE_UUID_NS_NAME_SIZE 128 @@ -493,7 +494,7 @@ static int tee_ioctl_open_session(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -571,7 +572,7 @@ static int tee_ioctl_invoke(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -705,7 +706,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx, if (get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) != buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); @@ -804,7 +805,7 @@ static int tee_ioctl_supp_send(struct tee_context *ctx, get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) > buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); -- 2.39.5

7 months, 2 weeks

1
0
0 0

[PATCH AUTOSEL 6.1 5/6] tee: Prevent size calculation wraparound on 32-bit kernels

by Sasha Levin

From: Jann Horn <jannh(a)google.com> [ Upstream commit 39bb67edcc582b3b386a9ec983da67fa8a10ec03 ] The current code around TEE_IOCTL_PARAM_SIZE() is a bit wrong on 32-bit kernels: Multiplying a user-provided 32-bit value with the size of a structure can wrap around on such platforms. Fix it by using saturating arithmetic for the size calculation. This has no security consequences because, in all users of TEE_IOCTL_PARAM_SIZE(), the subsequent kcalloc() implicitly checks for wrapping. Signed-off-by: Jann Horn <jannh(a)google.com> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> Tested-by: Rouven Czerwinski <rouven.czerwinski(a)linaro.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- **YES** This commit should be backported to stable kernel trees. Here's my extensive analysis: ## Vulnerability Analysis The commit fixes a real integer overflow vulnerability in the TEE (Trusted Execution Environment) subsystem on 32-bit kernels. The issue occurs in the `TEE_IOCTL_PARAM_SIZE()` macro defined as: ```c #define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) ``` Where `struct tee_ioctl_param` is 32 bytes (4 × 8-byte fields). On 32-bit systems, when a user provides a large `num_params` value, the multiplication `32 * num_params` can wrap around, potentially bypassing buffer length validation checks. ## Specific Vulnerable Code Locations The vulnerable pattern appears in 4 locations in `drivers/tee/tee_core.c`: 1. **Line 490**: `tee_ioctl_open_session()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 2. **Line 568**: `tee_ioctl_invoke()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 3. **Line 702**: `tee_ioctl_supp_recv()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len` 4. **Line 801**: `tee_ioctl_supp_send()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len` ## Attack Scenario On 32-bit systems, if `num_params = 0x08000000` (134,217,728), then: - `TEE_IOCTL_PARAM_SIZE(0x08000000) = 32 * 0x08000000 = 0x100000000` - This wraps to `0x00000000` on 32-bit systems - The size validation `sizeof(arg) + 0 != buf.buf_len` could be bypassed - Subsequent `kcalloc()` would allocate a huge buffer, but the size calculations would be wrong ## Why This Should Be Backported **Matches "YES" backport criteria from similar commits:** 1. **Security Fix**: Like Similar Commit #2 (rtl8188eu security hole) and #4 (gup_benchmark integer overflow), this prevents user-controlled integer overflow that could lead to memory corruption. 2. **Small, Contained Change**: The fix uses the kernel's standard `size_add()` and `size_mul()` helpers from `linux/overflow.h` - exactly like Similar Commit #3 which was noted as using `size_add()` for similar protection. 3. **Critical Subsystem**: TEE handles sensitive cryptographic operations and trusted applications, making security fixes here particularly important. 4. **Low Regression Risk**: The fix only adds overflow protection without changing functionality. The `size_mul()` and `size_add()` functions are well- tested kernel overflow helpers. 5. **User-Controllable Input**: The `num_params` value comes directly from user space through ioctl calls, making this a user-triggerable condition. 6. **Clear Security Impact**: While the commit states "no security consequences because kcalloc() implicitly checks for wrapping," this is defense-in-depth - the size validation should not rely on downstream allocation checks. The fix pattern exactly matches Similar Commit #4 (mm/gup_benchmark.c) which was backported for preventing integer overflow in size calculations with user-provided values. This commit follows the same security principle and uses the same overflow-safe arithmetic helpers that the kernel has standardized on for these scenarios. drivers/tee/tee_core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index 98da206cd7615..a9a893bc19fa4 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -10,6 +10,7 @@ #include <linux/fs.h> #include <linux/idr.h> #include <linux/module.h> +#include <linux/overflow.h> #include <linux/slab.h> #include <linux/tee_drv.h> #include <linux/uaccess.h> @@ -19,7 +20,7 @@ #define TEE_NUM_DEVICES 32 -#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) +#define TEE_IOCTL_PARAM_SIZE(x) (size_mul(sizeof(struct tee_param), (x))) #define TEE_UUID_NS_NAME_SIZE 128 @@ -487,7 +488,7 @@ static int tee_ioctl_open_session(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -565,7 +566,7 @@ static int tee_ioctl_invoke(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -699,7 +700,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx, if (get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) != buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); @@ -798,7 +799,7 @@ static int tee_ioctl_supp_send(struct tee_context *ctx, get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) > buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); -- 2.39.5

7 months, 2 weeks

1
0
0 0

[PATCH AUTOSEL 6.6 5/6] tee: Prevent size calculation wraparound on 32-bit kernels

by Sasha Levin

From: Jann Horn <jannh(a)google.com> [ Upstream commit 39bb67edcc582b3b386a9ec983da67fa8a10ec03 ] The current code around TEE_IOCTL_PARAM_SIZE() is a bit wrong on 32-bit kernels: Multiplying a user-provided 32-bit value with the size of a structure can wrap around on such platforms. Fix it by using saturating arithmetic for the size calculation. This has no security consequences because, in all users of TEE_IOCTL_PARAM_SIZE(), the subsequent kcalloc() implicitly checks for wrapping. Signed-off-by: Jann Horn <jannh(a)google.com> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> Tested-by: Rouven Czerwinski <rouven.czerwinski(a)linaro.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- **YES** This commit should be backported to stable kernel trees. Here's my extensive analysis: ## Vulnerability Analysis The commit fixes a real integer overflow vulnerability in the TEE (Trusted Execution Environment) subsystem on 32-bit kernels. The issue occurs in the `TEE_IOCTL_PARAM_SIZE()` macro defined as: ```c #define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) ``` Where `struct tee_ioctl_param` is 32 bytes (4 × 8-byte fields). On 32-bit systems, when a user provides a large `num_params` value, the multiplication `32 * num_params` can wrap around, potentially bypassing buffer length validation checks. ## Specific Vulnerable Code Locations The vulnerable pattern appears in 4 locations in `drivers/tee/tee_core.c`: 1. **Line 490**: `tee_ioctl_open_session()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 2. **Line 568**: `tee_ioctl_invoke()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 3. **Line 702**: `tee_ioctl_supp_recv()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len` 4. **Line 801**: `tee_ioctl_supp_send()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len` ## Attack Scenario On 32-bit systems, if `num_params = 0x08000000` (134,217,728), then: - `TEE_IOCTL_PARAM_SIZE(0x08000000) = 32 * 0x08000000 = 0x100000000` - This wraps to `0x00000000` on 32-bit systems - The size validation `sizeof(arg) + 0 != buf.buf_len` could be bypassed - Subsequent `kcalloc()` would allocate a huge buffer, but the size calculations would be wrong ## Why This Should Be Backported **Matches "YES" backport criteria from similar commits:** 1. **Security Fix**: Like Similar Commit #2 (rtl8188eu security hole) and #4 (gup_benchmark integer overflow), this prevents user-controlled integer overflow that could lead to memory corruption. 2. **Small, Contained Change**: The fix uses the kernel's standard `size_add()` and `size_mul()` helpers from `linux/overflow.h` - exactly like Similar Commit #3 which was noted as using `size_add()` for similar protection. 3. **Critical Subsystem**: TEE handles sensitive cryptographic operations and trusted applications, making security fixes here particularly important. 4. **Low Regression Risk**: The fix only adds overflow protection without changing functionality. The `size_mul()` and `size_add()` functions are well- tested kernel overflow helpers. 5. **User-Controllable Input**: The `num_params` value comes directly from user space through ioctl calls, making this a user-triggerable condition. 6. **Clear Security Impact**: While the commit states "no security consequences because kcalloc() implicitly checks for wrapping," this is defense-in-depth - the size validation should not rely on downstream allocation checks. The fix pattern exactly matches Similar Commit #4 (mm/gup_benchmark.c) which was backported for preventing integer overflow in size calculations with user-provided values. This commit follows the same security principle and uses the same overflow-safe arithmetic helpers that the kernel has standardized on for these scenarios. drivers/tee/tee_core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index 0eb342de0b001..d7ad16f262b2e 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -10,6 +10,7 @@ #include <linux/fs.h> #include <linux/idr.h> #include <linux/module.h> +#include <linux/overflow.h> #include <linux/slab.h> #include <linux/tee_drv.h> #include <linux/uaccess.h> @@ -19,7 +20,7 @@ #define TEE_NUM_DEVICES 32 -#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) +#define TEE_IOCTL_PARAM_SIZE(x) (size_mul(sizeof(struct tee_param), (x))) #define TEE_UUID_NS_NAME_SIZE 128 @@ -487,7 +488,7 @@ static int tee_ioctl_open_session(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -565,7 +566,7 @@ static int tee_ioctl_invoke(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -699,7 +700,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx, if (get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) != buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); @@ -798,7 +799,7 @@ static int tee_ioctl_supp_send(struct tee_context *ctx, get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) > buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); -- 2.39.5

7 months, 2 weeks

1
0
0 0

[PATCH AUTOSEL 6.12 5/6] tee: Prevent size calculation wraparound on 32-bit kernels

by Sasha Levin

From: Jann Horn <jannh(a)google.com> [ Upstream commit 39bb67edcc582b3b386a9ec983da67fa8a10ec03 ] The current code around TEE_IOCTL_PARAM_SIZE() is a bit wrong on 32-bit kernels: Multiplying a user-provided 32-bit value with the size of a structure can wrap around on such platforms. Fix it by using saturating arithmetic for the size calculation. This has no security consequences because, in all users of TEE_IOCTL_PARAM_SIZE(), the subsequent kcalloc() implicitly checks for wrapping. Signed-off-by: Jann Horn <jannh(a)google.com> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> Tested-by: Rouven Czerwinski <rouven.czerwinski(a)linaro.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- **YES** This commit should be backported to stable kernel trees. Here's my extensive analysis: ## Vulnerability Analysis The commit fixes a real integer overflow vulnerability in the TEE (Trusted Execution Environment) subsystem on 32-bit kernels. The issue occurs in the `TEE_IOCTL_PARAM_SIZE()` macro defined as: ```c #define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) ``` Where `struct tee_ioctl_param` is 32 bytes (4 × 8-byte fields). On 32-bit systems, when a user provides a large `num_params` value, the multiplication `32 * num_params` can wrap around, potentially bypassing buffer length validation checks. ## Specific Vulnerable Code Locations The vulnerable pattern appears in 4 locations in `drivers/tee/tee_core.c`: 1. **Line 490**: `tee_ioctl_open_session()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 2. **Line 568**: `tee_ioctl_invoke()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 3. **Line 702**: `tee_ioctl_supp_recv()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len` 4. **Line 801**: `tee_ioctl_supp_send()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len` ## Attack Scenario On 32-bit systems, if `num_params = 0x08000000` (134,217,728), then: - `TEE_IOCTL_PARAM_SIZE(0x08000000) = 32 * 0x08000000 = 0x100000000` - This wraps to `0x00000000` on 32-bit systems - The size validation `sizeof(arg) + 0 != buf.buf_len` could be bypassed - Subsequent `kcalloc()` would allocate a huge buffer, but the size calculations would be wrong ## Why This Should Be Backported **Matches "YES" backport criteria from similar commits:** 1. **Security Fix**: Like Similar Commit #2 (rtl8188eu security hole) and #4 (gup_benchmark integer overflow), this prevents user-controlled integer overflow that could lead to memory corruption. 2. **Small, Contained Change**: The fix uses the kernel's standard `size_add()` and `size_mul()` helpers from `linux/overflow.h` - exactly like Similar Commit #3 which was noted as using `size_add()` for similar protection. 3. **Critical Subsystem**: TEE handles sensitive cryptographic operations and trusted applications, making security fixes here particularly important. 4. **Low Regression Risk**: The fix only adds overflow protection without changing functionality. The `size_mul()` and `size_add()` functions are well- tested kernel overflow helpers. 5. **User-Controllable Input**: The `num_params` value comes directly from user space through ioctl calls, making this a user-triggerable condition. 6. **Clear Security Impact**: While the commit states "no security consequences because kcalloc() implicitly checks for wrapping," this is defense-in-depth - the size validation should not rely on downstream allocation checks. The fix pattern exactly matches Similar Commit #4 (mm/gup_benchmark.c) which was backported for preventing integer overflow in size calculations with user-provided values. This commit follows the same security principle and uses the same overflow-safe arithmetic helpers that the kernel has standardized on for these scenarios. drivers/tee/tee_core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index d113679b1e2d7..acc7998758ad8 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -10,6 +10,7 @@ #include <linux/fs.h> #include <linux/idr.h> #include <linux/module.h> +#include <linux/overflow.h> #include <linux/slab.h> #include <linux/tee_core.h> #include <linux/uaccess.h> @@ -19,7 +20,7 @@ #define TEE_NUM_DEVICES 32 -#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) +#define TEE_IOCTL_PARAM_SIZE(x) (size_mul(sizeof(struct tee_param), (x))) #define TEE_UUID_NS_NAME_SIZE 128 @@ -487,7 +488,7 @@ static int tee_ioctl_open_session(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -565,7 +566,7 @@ static int tee_ioctl_invoke(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -699,7 +700,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx, if (get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) != buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); @@ -798,7 +799,7 @@ static int tee_ioctl_supp_send(struct tee_context *ctx, get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) > buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); -- 2.39.5

7 months, 2 weeks

1
0
0 0

[PATCH AUTOSEL 6.14 7/8] tee: Prevent size calculation wraparound on 32-bit kernels

by Sasha Levin

From: Jann Horn <jannh(a)google.com> [ Upstream commit 39bb67edcc582b3b386a9ec983da67fa8a10ec03 ] The current code around TEE_IOCTL_PARAM_SIZE() is a bit wrong on 32-bit kernels: Multiplying a user-provided 32-bit value with the size of a structure can wrap around on such platforms. Fix it by using saturating arithmetic for the size calculation. This has no security consequences because, in all users of TEE_IOCTL_PARAM_SIZE(), the subsequent kcalloc() implicitly checks for wrapping. Signed-off-by: Jann Horn <jannh(a)google.com> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> Tested-by: Rouven Czerwinski <rouven.czerwinski(a)linaro.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- **YES** This commit should be backported to stable kernel trees. Here's my extensive analysis: ## Vulnerability Analysis The commit fixes a real integer overflow vulnerability in the TEE (Trusted Execution Environment) subsystem on 32-bit kernels. The issue occurs in the `TEE_IOCTL_PARAM_SIZE()` macro defined as: ```c #define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) ``` Where `struct tee_ioctl_param` is 32 bytes (4 × 8-byte fields). On 32-bit systems, when a user provides a large `num_params` value, the multiplication `32 * num_params` can wrap around, potentially bypassing buffer length validation checks. ## Specific Vulnerable Code Locations The vulnerable pattern appears in 4 locations in `drivers/tee/tee_core.c`: 1. **Line 490**: `tee_ioctl_open_session()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 2. **Line 568**: `tee_ioctl_invoke()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 3. **Line 702**: `tee_ioctl_supp_recv()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len` 4. **Line 801**: `tee_ioctl_supp_send()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len` ## Attack Scenario On 32-bit systems, if `num_params = 0x08000000` (134,217,728), then: - `TEE_IOCTL_PARAM_SIZE(0x08000000) = 32 * 0x08000000 = 0x100000000` - This wraps to `0x00000000` on 32-bit systems - The size validation `sizeof(arg) + 0 != buf.buf_len` could be bypassed - Subsequent `kcalloc()` would allocate a huge buffer, but the size calculations would be wrong ## Why This Should Be Backported **Matches "YES" backport criteria from similar commits:** 1. **Security Fix**: Like Similar Commit #2 (rtl8188eu security hole) and #4 (gup_benchmark integer overflow), this prevents user-controlled integer overflow that could lead to memory corruption. 2. **Small, Contained Change**: The fix uses the kernel's standard `size_add()` and `size_mul()` helpers from `linux/overflow.h` - exactly like Similar Commit #3 which was noted as using `size_add()` for similar protection. 3. **Critical Subsystem**: TEE handles sensitive cryptographic operations and trusted applications, making security fixes here particularly important. 4. **Low Regression Risk**: The fix only adds overflow protection without changing functionality. The `size_mul()` and `size_add()` functions are well- tested kernel overflow helpers. 5. **User-Controllable Input**: The `num_params` value comes directly from user space through ioctl calls, making this a user-triggerable condition. 6. **Clear Security Impact**: While the commit states "no security consequences because kcalloc() implicitly checks for wrapping," this is defense-in-depth - the size validation should not rely on downstream allocation checks. The fix pattern exactly matches Similar Commit #4 (mm/gup_benchmark.c) which was backported for preventing integer overflow in size calculations with user-provided values. This commit follows the same security principle and uses the same overflow-safe arithmetic helpers that the kernel has standardized on for these scenarios. drivers/tee/tee_core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index d113679b1e2d7..acc7998758ad8 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -10,6 +10,7 @@ #include <linux/fs.h> #include <linux/idr.h> #include <linux/module.h> +#include <linux/overflow.h> #include <linux/slab.h> #include <linux/tee_core.h> #include <linux/uaccess.h> @@ -19,7 +20,7 @@ #define TEE_NUM_DEVICES 32 -#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) +#define TEE_IOCTL_PARAM_SIZE(x) (size_mul(sizeof(struct tee_param), (x))) #define TEE_UUID_NS_NAME_SIZE 128 @@ -487,7 +488,7 @@ static int tee_ioctl_open_session(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -565,7 +566,7 @@ static int tee_ioctl_invoke(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -699,7 +700,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx, if (get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) != buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); @@ -798,7 +799,7 @@ static int tee_ioctl_supp_send(struct tee_context *ctx, get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) > buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); -- 2.39.5

7 months, 2 weeks

1
0
0 0

[PATCH AUTOSEL 6.15 8/9] tee: Prevent size calculation wraparound on 32-bit kernels

by Sasha Levin

From: Jann Horn <jannh(a)google.com> [ Upstream commit 39bb67edcc582b3b386a9ec983da67fa8a10ec03 ] The current code around TEE_IOCTL_PARAM_SIZE() is a bit wrong on 32-bit kernels: Multiplying a user-provided 32-bit value with the size of a structure can wrap around on such platforms. Fix it by using saturating arithmetic for the size calculation. This has no security consequences because, in all users of TEE_IOCTL_PARAM_SIZE(), the subsequent kcalloc() implicitly checks for wrapping. Signed-off-by: Jann Horn <jannh(a)google.com> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> Tested-by: Rouven Czerwinski <rouven.czerwinski(a)linaro.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- **YES** This commit should be backported to stable kernel trees. Here's my extensive analysis: ## Vulnerability Analysis The commit fixes a real integer overflow vulnerability in the TEE (Trusted Execution Environment) subsystem on 32-bit kernels. The issue occurs in the `TEE_IOCTL_PARAM_SIZE()` macro defined as: ```c #define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) ``` Where `struct tee_ioctl_param` is 32 bytes (4 × 8-byte fields). On 32-bit systems, when a user provides a large `num_params` value, the multiplication `32 * num_params` can wrap around, potentially bypassing buffer length validation checks. ## Specific Vulnerable Code Locations The vulnerable pattern appears in 4 locations in `drivers/tee/tee_core.c`: 1. **Line 490**: `tee_ioctl_open_session()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 2. **Line 568**: `tee_ioctl_invoke()` - `sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len` 3. **Line 702**: `tee_ioctl_supp_recv()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len` 4. **Line 801**: `tee_ioctl_supp_send()` - `sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len` ## Attack Scenario On 32-bit systems, if `num_params = 0x08000000` (134,217,728), then: - `TEE_IOCTL_PARAM_SIZE(0x08000000) = 32 * 0x08000000 = 0x100000000` - This wraps to `0x00000000` on 32-bit systems - The size validation `sizeof(arg) + 0 != buf.buf_len` could be bypassed - Subsequent `kcalloc()` would allocate a huge buffer, but the size calculations would be wrong ## Why This Should Be Backported **Matches "YES" backport criteria from similar commits:** 1. **Security Fix**: Like Similar Commit #2 (rtl8188eu security hole) and #4 (gup_benchmark integer overflow), this prevents user-controlled integer overflow that could lead to memory corruption. 2. **Small, Contained Change**: The fix uses the kernel's standard `size_add()` and `size_mul()` helpers from `linux/overflow.h` - exactly like Similar Commit #3 which was noted as using `size_add()` for similar protection. 3. **Critical Subsystem**: TEE handles sensitive cryptographic operations and trusted applications, making security fixes here particularly important. 4. **Low Regression Risk**: The fix only adds overflow protection without changing functionality. The `size_mul()` and `size_add()` functions are well- tested kernel overflow helpers. 5. **User-Controllable Input**: The `num_params` value comes directly from user space through ioctl calls, making this a user-triggerable condition. 6. **Clear Security Impact**: While the commit states "no security consequences because kcalloc() implicitly checks for wrapping," this is defense-in-depth - the size validation should not rely on downstream allocation checks. The fix pattern exactly matches Similar Commit #4 (mm/gup_benchmark.c) which was backported for preventing integer overflow in size calculations with user-provided values. This commit follows the same security principle and uses the same overflow-safe arithmetic helpers that the kernel has standardized on for these scenarios. drivers/tee/tee_core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index d113679b1e2d7..acc7998758ad8 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -10,6 +10,7 @@ #include <linux/fs.h> #include <linux/idr.h> #include <linux/module.h> +#include <linux/overflow.h> #include <linux/slab.h> #include <linux/tee_core.h> #include <linux/uaccess.h> @@ -19,7 +20,7 @@ #define TEE_NUM_DEVICES 32 -#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) +#define TEE_IOCTL_PARAM_SIZE(x) (size_mul(sizeof(struct tee_param), (x))) #define TEE_UUID_NS_NAME_SIZE 128 @@ -487,7 +488,7 @@ static int tee_ioctl_open_session(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -565,7 +566,7 @@ static int tee_ioctl_invoke(struct tee_context *ctx, if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; - if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) + if (size_add(sizeof(arg), TEE_IOCTL_PARAM_SIZE(arg.num_params)) != buf.buf_len) return -EINVAL; if (arg.num_params) { @@ -699,7 +700,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx, if (get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) != buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) != buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); @@ -798,7 +799,7 @@ static int tee_ioctl_supp_send(struct tee_context *ctx, get_user(num_params, &uarg->num_params)) return -EFAULT; - if (sizeof(*uarg) + TEE_IOCTL_PARAM_SIZE(num_params) > buf.buf_len) + if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) > buf.buf_len) return -EINVAL; params = kcalloc(num_params, sizeof(struct tee_param), GFP_KERNEL); -- 2.39.5

7 months, 2 weeks

1
0
0 0

[PATCH v9 0/9] TEE subsystem for protected dma-buf allocations

by Jens Wiklander

Hi, This patch set allocates the protected DMA-bufs from a DMA-heap instantiated from the TEE subsystem. The TEE subsystem handles the DMA-buf allocations since it is the TEE (OP-TEE, AMD-TEE, TS-TEE, or perhaps a future QTEE) which sets up the protection for the memory used for the DMA-bufs. The DMA-heap uses a protected memory pool provided by the backend TEE driver, allowing it to choose how to allocate the protected physical memory. The allocated DMA-bufs must be imported with a new TEE_IOC_SHM_REGISTER_FD before they can be passed as arguments when requesting services from the secure world. Three use-cases (Secure Video Playback, Trusted UI, and Secure Video Recording) have been identified so far to serve as examples of what can be expected. The use-cases have predefined DMA-heap names, "protected,secure-video", "protected,trusted-ui", and "protected,secure-video-record". The backend driver registers protected memory pools for the use-cases it supports. Each use-case has its own protected memory pool since different use-cases require isolation from different parts of the system. A protected memory pool can be based on a static carveout instantiated while probing the TEE backend driver, or dynamically allocated from CMA (dma_alloc_pages()) and made protected as needed by the TEE. This can be tested on a RockPi 4B+ with the following steps: repo init -u https://github.com/jenswi-linaro/manifest.git -m rockpi4.xml \ -b prototype/sdp-v9 repo sync -j8 cd build make toolchains -j$(nproc) make all -j$(nproc) # Copy ../out/rockpi4.img to an SD card and boot the RockPi from that # Connect a monitor to the RockPi # login and at the prompt: gst-launch-1.0 videotestsrc ! \ aesenc key=1f9423681beb9a79215820f6bda73d0f \ iv=e9aa8e834d8d70b7e0d254ff670dd718 serialize-iv=true ! \ aesdec key=1f9423681beb9a79215820f6bda73d0f ! \ kmssink The aesdec module has been hacked to use an OP-TEE TA to decrypt the stream into protected DMA-bufs which are consumed by the kmssink. The primitive QEMU tests from previous patch sets can be tested on RockPi in the same way using: xtest --sdp-basic The primitive tests are tested on QEMU with the following steps: repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \ -b prototype/sdp-v9 repo sync -j8 cd build make toolchains -j$(nproc) make SPMC_AT_EL=1 all -j$(nproc) make SPMC_AT_EL=1 run-only # login and at the prompt: xtest --sdp-basic The SPMC_AT_EL=1 parameter configures the build with FF-A and an SPMC at S-EL1 inside OP-TEE. The parameter can be changed to SPMC_AT_EL=n to test without FF-A using the original SMC ABI instead. Please remember to do %make arm-tf-clean for TF-A to be rebuilt properly using the new configuration. https://optee.readthedocs.io/en/latest/building/prerequisites.html list dependencies required to build the above. The primitive tests are pretty basic, mostly checking that a Trusted Application in the secure world can access and manipulate the memory. There are also some negative tests for out of bounds buffers, etc. Thanks, Jens Changes since V8: * Using dma_alloc_pages() instead of cma_alloc() so the direct dependency on CMA can be removed together with the patches "cma: export cma_alloc() and cma_release()" and "dma-contiguous: export dma_contiguous_default_area". The patch * Renaming the patch "tee: add tee_shm_alloc_cma_phys_mem()" to "tee: add tee_shm_alloc_dma_mem()" * Setting DMA mask for the OP-TEE TEE device based on input from the secure world instead of relying on the parent device so following patches are removed: "tee: tee_device_alloc(): copy dma_mask from parent device" and "optee: pass parent device to tee_device_alloc()". * Adding Sumit Garg's R-B to "tee: refactor params_from_user()" * In the patch "tee: implement protected DMA-heap", map the physical memory passed to tee_protmem_static_pool_alloc(). Changes since V7: * Adding "dma-buf: dma-heap: export declared functions", "cma: export cma_alloc() and cma_release()", and "dma-contiguous: export dma_contiguous_default_area" to export the symbols needed to keep the TEE subsystem as a load module. * Removing CONFIG_TEE_DMABUF_HEAP and CONFIG_TEE_CMA since they aren't needed any longer. * Addressing review comments in "optee: sync secure world ABI headers" * Better align protected memory pool initialization between the smc-abi and ffa-abi parts of the optee driver. * Removing the patch "optee: account for direction while converting parameters" Changes since V6: * Restricted memory is now known as protected memory since to use the same term as https://docs.vulkan.org/guide/latest/protected.html. Update all patches to consistently use protected memory. * In "tee: implement protected DMA-heap" add the hidden config option TEE_DMABUF_HEAP to tell if the DMABUF_HEAPS functions are available for the TEE subsystem * Adding "tee: refactor params_from_user()", broken out from the patch "tee: new ioctl to a register tee_shm from a dmabuf file descriptor" * For "tee: new ioctl to a register tee_shm from a dmabuf file descriptor": - Update commit message to mention protected memory - Remove and open code tee_shm_get_parent_shm() in param_from_user_memref() * In "tee: add tee_shm_alloc_cma_phys_mem" add the hidden config option TEE_CMA to tell if the CMA functions are available for the TEE subsystem * For "tee: tee_device_alloc(): copy dma_mask from parent device" and "optee: pass parent device to tee_device_alloc", added Reviewed-by: Sumit Garg <sumit.garg(a)kernel.org> Changes since V5: * Removing "tee: add restricted memory allocation" and "tee: add TEE_IOC_RSTMEM_FD_INFO" * Adding "tee: implement restricted DMA-heap", "tee: new ioctl to a register tee_shm from a dmabuf file descriptor", "tee: add tee_shm_alloc_cma_phys_mem()", "optee: pass parent device to tee_device_alloc()", and "tee: tee_device_alloc(): copy dma_mask from parent device" * The two TEE driver OPs "rstmem_alloc()" and "rstmem_free()" are replaced with a struct tee_rstmem_pool abstraction. * Replaced the the TEE_IOC_RSTMEM_ALLOC user space API with the DMA-heap API Changes since V4: * Adding the patch "tee: add TEE_IOC_RSTMEM_FD_INFO" needed by the GStreamer demo * Removing the dummy CPU access and mmap functions from the dma_buf_ops * Fixing a compile error in "optee: FF-A: dynamic restricted memory allocation" reported by kernel test robot <lkp(a)intel.com> Changes since V3: * Make the use_case and flags field in struct tee_shm u32's instead of u16's * Add more description for TEE_IOC_RSTMEM_ALLOC in the header file * Import namespace DMA_BUF in module tee, reported by lkp(a)intel.com * Added a note in the commit message for "optee: account for direction while converting parameters" why it's needed * Factor out dynamic restricted memory allocation from "optee: support restricted memory allocation" into two new commits "optee: FF-A: dynamic restricted memory allocation" and "optee: smc abi: dynamic restricted memory allocation" * Guard CMA usage with #ifdef CONFIG_CMA, effectively disabling dynamic restricted memory allocate if CMA isn't configured Changes since the V2 RFC: * Based on v6.12 * Replaced the flags for SVP and Trusted UID memory with a u32 field with unique id for each use case * Added dynamic allocation of restricted memory pools * Added OP-TEE ABI both with and without FF-A for dynamic restricted memory * Added support for FF-A with FFA_LEND Changes since the V1 RFC: * Based on v6.11 * Complete rewrite, replacing the restricted heap with TEE_IOC_RSTMEM_ALLOC Changes since Olivier's post [2]: * Based on Yong Wu's post [1] where much of dma-buf handling is done in the generic restricted heap * Simplifications and cleanup * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap support" * Replaced the word "secure" with "restricted" where applicable Etienne Carriere (1): tee: new ioctl to a register tee_shm from a dmabuf file descriptor Jens Wiklander (8): optee: sync secure world ABI headers dma-buf: dma-heap: export declared functions tee: implement protected DMA-heap tee: refactor params_from_user() tee: add tee_shm_alloc_dma_mem() optee: support protected memory allocation optee: FF-A: dynamic protected memory allocation optee: smc abi: dynamic protected memory allocation drivers/dma-buf/dma-heap.c | 3 + drivers/tee/Makefile | 1 + drivers/tee/optee/Makefile | 1 + drivers/tee/optee/core.c | 10 + drivers/tee/optee/ffa_abi.c | 147 ++++++++- drivers/tee/optee/optee_ffa.h | 27 +- drivers/tee/optee/optee_msg.h | 84 +++++- drivers/tee/optee/optee_private.h | 15 +- drivers/tee/optee/optee_smc.h | 37 ++- drivers/tee/optee/protmem.c | 332 ++++++++++++++++++++ drivers/tee/optee/smc_abi.c | 113 ++++++- drivers/tee/tee_core.c | 155 +++++++--- drivers/tee/tee_heap.c | 487 ++++++++++++++++++++++++++++++ drivers/tee/tee_private.h | 16 + drivers/tee/tee_shm.c | 183 ++++++++++- include/linux/tee_core.h | 71 +++++ include/linux/tee_drv.h | 10 + include/uapi/linux/tee.h | 31 ++ 18 files changed, 1655 insertions(+), 68 deletions(-) create mode 100644 drivers/tee/optee/protmem.c create mode 100644 drivers/tee/tee_heap.c base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e -- 2.43.0

7 months, 2 weeks

5
31
0 0

2026

2025

2024

2023

2022

2021

2020

OP-TEE