Re-order these checks to check if "i" is a valid array index before using it. This prevents a potential off by one read access.
Fixes: d6e290837e50 ("tee: add Qualcomm TEE driver") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org --- drivers/tee/qcomtee/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c index cc17a48d0ab7..ac134452cc9c 100644 --- a/drivers/tee/qcomtee/call.c +++ b/drivers/tee/qcomtee/call.c @@ -308,7 +308,7 @@ static int qcomtee_params_from_args(struct tee_param *params, }
/* Release any IO and OO objects not processed. */ - for (; u[i].type && i < num_params; i++) { + for (; i < num_params && u[i].type; i++) { if (u[i].type == QCOMTEE_ARG_TYPE_OO || u[i].type == QCOMTEE_ARG_TYPE_IO) qcomtee_object_put(u[i].o);
On Thu, Sep 18, 2025 at 12:50:26PM +0300, Dan Carpenter wrote:
Re-order these checks to check if "i" is a valid array index before using it. This prevents a potential off by one read access.
Fixes: d6e290837e50 ("tee: add Qualcomm TEE driver") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org
drivers/tee/qcomtee/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Sumit Garg sumit.garg@oss.qualcomm.com
-Sumit
diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c index cc17a48d0ab7..ac134452cc9c 100644 --- a/drivers/tee/qcomtee/call.c +++ b/drivers/tee/qcomtee/call.c @@ -308,7 +308,7 @@ static int qcomtee_params_from_args(struct tee_param *params, } /* Release any IO and OO objects not processed. */
- for (; u[i].type && i < num_params; i++) {
- for (; i < num_params && u[i].type; i++) { if (u[i].type == QCOMTEE_ARG_TYPE_OO || u[i].type == QCOMTEE_ARG_TYPE_IO) qcomtee_object_put(u[i].o);
-- 2.51.0
On Fri, Sep 19, 2025 at 7:21 AM Sumit Garg sumit.garg@kernel.org wrote:
On Thu, Sep 18, 2025 at 12:50:26PM +0300, Dan Carpenter wrote:
Re-order these checks to check if "i" is a valid array index before using it. This prevents a potential off by one read access.
Fixes: d6e290837e50 ("tee: add Qualcomm TEE driver") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org
drivers/tee/qcomtee/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Sumit Garg sumit.garg@oss.qualcomm.com
Applied.
/Jens
-Sumit
diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c index cc17a48d0ab7..ac134452cc9c 100644 --- a/drivers/tee/qcomtee/call.c +++ b/drivers/tee/qcomtee/call.c @@ -308,7 +308,7 @@ static int qcomtee_params_from_args(struct tee_param *params, }
/* Release any IO and OO objects not processed. */
for (; u[i].type && i < num_params; i++) {
for (; i < num_params && u[i].type; i++) { if (u[i].type == QCOMTEE_ARG_TYPE_OO || u[i].type == QCOMTEE_ARG_TYPE_IO) qcomtee_object_put(u[i].o);-- 2.51.0
On 9/18/2025 7:50 PM, Dan Carpenter wrote:
Re-order these checks to check if "i" is a valid array index before using it. This prevents a potential off by one read access.
Fixes: d6e290837e50 ("tee: add Qualcomm TEE driver") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org
drivers/tee/qcomtee/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c index cc17a48d0ab7..ac134452cc9c 100644 --- a/drivers/tee/qcomtee/call.c +++ b/drivers/tee/qcomtee/call.c @@ -308,7 +308,7 @@ static int qcomtee_params_from_args(struct tee_param *params, } /* Release any IO and OO objects not processed. */
- for (; u[i].type && i < num_params; i++) {
- for (; i < num_params && u[i].type; i++) { if (u[i].type == QCOMTEE_ARG_TYPE_OO || u[i].type == QCOMTEE_ARG_TYPE_IO) qcomtee_object_put(u[i].o);
This is not required, considering the sequence of clean up, this would never happen. `i` at least have been accessed once in the switch above.
Regards, Amir
On 9/24/2025 8:48 AM, Amirreza Zarrabi wrote:
On 9/18/2025 7:50 PM, Dan Carpenter wrote:
Re-order these checks to check if "i" is a valid array index before using it. This prevents a potential off by one read access.
Fixes: d6e290837e50 ("tee: add Qualcomm TEE driver") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org
drivers/tee/qcomtee/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c index cc17a48d0ab7..ac134452cc9c 100644 --- a/drivers/tee/qcomtee/call.c +++ b/drivers/tee/qcomtee/call.c @@ -308,7 +308,7 @@ static int qcomtee_params_from_args(struct tee_param *params, } /* Release any IO and OO objects not processed. */
- for (; u[i].type && i < num_params; i++) {
- for (; i < num_params && u[i].type; i++) { if (u[i].type == QCOMTEE_ARG_TYPE_OO || u[i].type == QCOMTEE_ARG_TYPE_IO) qcomtee_object_put(u[i].o);
This is not required, considering the sequence of clean up, this would never happen. `i` at least have been accessed once in the switch above.
Regards, Amir
Also, size of u is always num_params + 1 for the ending 0. (basically means `i < num_params` can be removed).
Anyway, it does not hurt :).
Regards, Amir
On Wed, Sep 24, 2025 at 08:58:45AM +1000, Amirreza Zarrabi wrote:
On 9/24/2025 8:48 AM, Amirreza Zarrabi wrote:
On 9/18/2025 7:50 PM, Dan Carpenter wrote:
Re-order these checks to check if "i" is a valid array index before using it. This prevents a potential off by one read access.
Fixes: d6e290837e50 ("tee: add Qualcomm TEE driver") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org
drivers/tee/qcomtee/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c index cc17a48d0ab7..ac134452cc9c 100644 --- a/drivers/tee/qcomtee/call.c +++ b/drivers/tee/qcomtee/call.c @@ -308,7 +308,7 @@ static int qcomtee_params_from_args(struct tee_param *params, } /* Release any IO and OO objects not processed. */
- for (; u[i].type && i < num_params; i++) {
- for (; i < num_params && u[i].type; i++) { if (u[i].type == QCOMTEE_ARG_TYPE_OO || u[i].type == QCOMTEE_ARG_TYPE_IO) qcomtee_object_put(u[i].o);
This is not required, considering the sequence of clean up, this would never happen. `i` at least have been accessed once in the switch above.
Regards, Amir
Also, size of u is always num_params + 1 for the ending 0. (basically means `i < num_params` can be removed).
Yes. This is true.
regards, dan carpenter
On Wed, Sep 24, 2025 at 9:36 AM Dan Carpenter dan.carpenter@linaro.org wrote:
On Wed, Sep 24, 2025 at 08:58:45AM +1000, Amirreza Zarrabi wrote:
On 9/24/2025 8:48 AM, Amirreza Zarrabi wrote:
On 9/18/2025 7:50 PM, Dan Carpenter wrote:
Re-order these checks to check if "i" is a valid array index before using it. This prevents a potential off by one read access.
Fixes: d6e290837e50 ("tee: add Qualcomm TEE driver") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org
drivers/tee/qcomtee/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c index cc17a48d0ab7..ac134452cc9c 100644 --- a/drivers/tee/qcomtee/call.c +++ b/drivers/tee/qcomtee/call.c @@ -308,7 +308,7 @@ static int qcomtee_params_from_args(struct tee_param *params, }
/* Release any IO and OO objects not processed. */
- for (; u[i].type && i < num_params; i++) {
- for (; i < num_params && u[i].type; i++) { if (u[i].type == QCOMTEE_ARG_TYPE_OO || u[i].type == QCOMTEE_ARG_TYPE_IO) qcomtee_object_put(u[i].o);
This is not required, considering the sequence of clean up, this would never happen. `i` at least have been accessed once in the switch above.
Regards, Amir
Also, size of u is always num_params + 1 for the ending 0. (basically means `i < num_params` can be removed).
Yes. This is true.
So this patch isn't needed. I'll drop it if no one objects.
Cheers, Jens
On Wed, Sep 24, 2025 at 11:21:34AM +0200, Jens Wiklander wrote:
On Wed, Sep 24, 2025 at 9:36 AM Dan Carpenter dan.carpenter@linaro.org wrote:
On Wed, Sep 24, 2025 at 08:58:45AM +1000, Amirreza Zarrabi wrote:
On 9/24/2025 8:48 AM, Amirreza Zarrabi wrote:
On 9/18/2025 7:50 PM, Dan Carpenter wrote:
Re-order these checks to check if "i" is a valid array index before using it. This prevents a potential off by one read access.
Fixes: d6e290837e50 ("tee: add Qualcomm TEE driver") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org
drivers/tee/qcomtee/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c index cc17a48d0ab7..ac134452cc9c 100644 --- a/drivers/tee/qcomtee/call.c +++ b/drivers/tee/qcomtee/call.c @@ -308,7 +308,7 @@ static int qcomtee_params_from_args(struct tee_param *params, }
/* Release any IO and OO objects not processed. */
- for (; u[i].type && i < num_params; i++) {
- for (; i < num_params && u[i].type; i++) { if (u[i].type == QCOMTEE_ARG_TYPE_OO || u[i].type == QCOMTEE_ARG_TYPE_IO) qcomtee_object_put(u[i].o);
This is not required, considering the sequence of clean up, this would never happen. `i` at least have been accessed once in the switch above.
Regards, Amir
Also, size of u is always num_params + 1 for the ending 0. (basically means `i < num_params` can be removed).
Yes. This is true.
So this patch isn't needed. I'll drop it if no one objects.
The patch makes the code better though... It never really makes sense to use a variable first and then check if it's valid later. In this case the check isn't required.
Ideally the code would only have one limit. We could either do:
for (; i < num_params; i++) { Or: for (; u[i].type != QCOMTEE_ARG_TYPE_INV; i++) {
Either way works...
regards, dan carpenter
On 9/24/2025 7:56 PM, Dan Carpenter wrote:
On Wed, Sep 24, 2025 at 11:21:34AM +0200, Jens Wiklander wrote:
On Wed, Sep 24, 2025 at 9:36 AM Dan Carpenter dan.carpenter@linaro.org wrote:
On Wed, Sep 24, 2025 at 08:58:45AM +1000, Amirreza Zarrabi wrote:
On 9/24/2025 8:48 AM, Amirreza Zarrabi wrote:
On 9/18/2025 7:50 PM, Dan Carpenter wrote:
Re-order these checks to check if "i" is a valid array index before using it. This prevents a potential off by one read access.
Fixes: d6e290837e50 ("tee: add Qualcomm TEE driver") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org
drivers/tee/qcomtee/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c index cc17a48d0ab7..ac134452cc9c 100644 --- a/drivers/tee/qcomtee/call.c +++ b/drivers/tee/qcomtee/call.c @@ -308,7 +308,7 @@ static int qcomtee_params_from_args(struct tee_param *params, }
/* Release any IO and OO objects not processed. */
- for (; u[i].type && i < num_params; i++) {
- for (; i < num_params && u[i].type; i++) { if (u[i].type == QCOMTEE_ARG_TYPE_OO || u[i].type == QCOMTEE_ARG_TYPE_IO) qcomtee_object_put(u[i].o);
This is not required, considering the sequence of clean up, this would never happen. `i` at least have been accessed once in the switch above.
Regards, Amir
Also, size of u is always num_params + 1 for the ending 0. (basically means `i < num_params` can be removed).
Yes. This is true.
So this patch isn't needed. I'll drop it if no one objects.
The patch makes the code better though... It never really makes sense to use a variable first and then check if it's valid later. In this case the check isn't required.
Ideally the code would only have one limit. We could either do:
for (; i < num_params; i++) { Or: for (; u[i].type != QCOMTEE_ARG_TYPE_INV; i++) {
Either way works...
regards, dan carpenter
Originally, it was written as
for (; u[i].type != QCOMTEE_ARG_TYPE_INV; i++) { ...
but changed trough out the review process. I do not have any preference. But if having it as
for (; i < num_params && u[i].type; i++) { ...
is more readable, let's keep it.
Regards, Amir
On Wed, Sep 24, 2025 at 08:48:29AM +1000, Amirreza Zarrabi wrote:
On 9/18/2025 7:50 PM, Dan Carpenter wrote:
Re-order these checks to check if "i" is a valid array index before using it. This prevents a potential off by one read access.
Fixes: d6e290837e50 ("tee: add Qualcomm TEE driver") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org
drivers/tee/qcomtee/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c index cc17a48d0ab7..ac134452cc9c 100644 --- a/drivers/tee/qcomtee/call.c +++ b/drivers/tee/qcomtee/call.c @@ -308,7 +308,7 @@ static int qcomtee_params_from_args(struct tee_param *params, } /* Release any IO and OO objects not processed. */
- for (; u[i].type && i < num_params; i++) {
- for (; i < num_params && u[i].type; i++) { if (u[i].type == QCOMTEE_ARG_TYPE_OO || u[i].type == QCOMTEE_ARG_TYPE_IO) qcomtee_object_put(u[i].o);
This is not required, considering the sequence of clean up, this would never happen. `i` at least have been accessed once in the switch above.
Only the first iteration has been accessed. The rest no.
regards, dan carpenter
op-tee@lists.trustedfirmware.org
-
Amirreza Zarrabi -
Dan Carpenter -
Jens Wiklander -
Sumit Garg