[PATCH 1/2] firmware: qcom: pas-tee: Always initialize resource table output size

[PATCH 1/2] firmware: qcom: pas-tee: Always initialize resource table output size

[Krzysztof Kozlowski]

qcom_pas_tee_get_rsc_table() sets the "output_rt_size" only when the first
TEE invocation reports a size smaller than the input table, or when the
second invocation returns a non-zero size.  If the TEE reports a size
bigger or equal to the input size on the first call, but then returns
size of 0 on the second call, the function returns NULL (no table) and
leaves "output_rt_size" output parameter untouched.

Caller of qcom_pas_tee_get_rsc_table() treats NULL as success, thus
uninitialized "output_rt_size" output argument could in theory lead to
out-of-bounds accesses further.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-bb15726c-4185-460a-a5f0-f779d5ee144a
Fixes: b6f7978da0c4 ("firmware: qcom: Add a PAS TEE service")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_pas_tee.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/firmware/qcom/qcom_pas_tee.c b/drivers/firmware/qcom/qcom_pas_tee.c
index ac33a00687aa..ba9957023ec8 100644
--- a/drivers/firmware/qcom/qcom_pas_tee.c
+++ b/drivers/firmware/qcom/qcom_pas_tee.c
@@ -275,6 +275,8 @@ static void *qcom_pas_tee_get_rsc_table(struct device *dev,
 			rt_buf = kmemdup(rt_shm_va, *output_rt_size, GFP_KERNEL);
 			if (!rt_buf)
 				return ERR_PTR(-ENOMEM);
+		} else {
+			*output_rt_size = 0;
 		}
 	} else {
 		*output_rt_size = 0;
-- 
2.53.0

[PATCH 2/2] firmware: qcom: pas-tee: Fix purely theoretical out-of-bounds read of resource table

[Krzysztof Kozlowski]

qcom_pas_tee_get_rsc_table() first queries the TEE for the size of the
resource table, allocates a shared memory buffer of that size and then
invokes the TEE again to fill the buffer.  TEE can update the returned
size, but the driver does not check that the size returned by the second
call fits in the allocated buffer.

Theoretically and unlikely, some erroneous or compromised TEE returning
a bigger size leads to kmemdup() reading past the end of the shared
memory buffer and leaking adjacent kernel memory.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-dc984fcd-4884-4518-a98d-9490de9ffce5
Fixes: b6f7978da0c4 ("firmware: qcom: Add a PAS TEE service")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_pas_tee.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_pas_tee.c b/drivers/firmware/qcom/qcom_pas_tee.c
index ba9957023ec8..cd84befa34d7 100644
--- a/drivers/firmware/qcom/qcom_pas_tee.c
+++ b/drivers/firmware/qcom/qcom_pas_tee.c
@@ -244,9 +244,9 @@ static void *qcom_pas_tee_get_rsc_table(struct device *dev,
 	}
 
 	if (param[1].u.memref.size >= input_rt_size) {
+		size_t rt_shm_size = param[1].u.memref.size;
 		struct tee_shm *rt_shm __free(shm_free) =
-			tee_shm_alloc_kernel_buf(data->ctx,
-						 param[1].u.memref.size);
+			tee_shm_alloc_kernel_buf(data->ctx, rt_shm_size);
 		void *rt_shm_va;
 
 		if (IS_ERR_OR_NULL(rt_shm)) {
@@ -270,6 +270,12 @@ static void *qcom_pas_tee_get_rsc_table(struct device *dev,
 			return ret ? ERR_PTR(ret) : ERR_PTR(-EINVAL);
 		}
 
+		if (param[1].u.memref.size > rt_shm_size) {
+			dev_err(dev, "PAS get RT returned too big size: %zu > %zu\n",
+				param[1].u.memref.size, rt_shm_size);
+			return ERR_PTR(-EINVAL);
+		}
+
 		if (param[1].u.memref.size) {
 			*output_rt_size = param[1].u.memref.size;
 			rt_buf = kmemdup(rt_shm_va, *output_rt_size, GFP_KERNEL);
-- 
2.53.0

Re: [PATCH 1/2] firmware: qcom: pas-tee: Always initialize resource table output size

[Konrad Dybcio]

On 11/2/26 4:01 PM, Krzysztof Kozlowski wrote:
> qcom_pas_tee_get_rsc_table() sets the "output_rt_size" only when the first
> TEE invocation reports a size smaller than the input table, or when the
> second invocation returns a non-zero size.  If the TEE reports a size
> bigger or equal to the input size on the first call, but then returns
> size of 0 on the second call, the function returns NULL (no table) and
> leaves "output_rt_size" output parameter untouched.
> 
> Caller of qcom_pas_tee_get_rsc_table() treats NULL as success, thus
> uninitialized "output_rt_size" output argument could in theory lead to
> out-of-bounds accesses further.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/bug/linux-bb15726c-4185-460a-a5f0-f779d5ee144a
> Fixes: b6f7978da0c4 ("firmware: qcom: Add a PAS TEE service")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> ---


Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad