Looking at the TEE_Param definition:
typedef union { struct { void *buffer; uint32_t size; } memref; struct { uint32_t a; uint32_t b; } value; } TEE_Param;
there seems to be an implicit assumption of 32-bit pointers. There are two things going on here:
- The 'memref' struct is larger than the 'value' structure, since the 'void *' is 8-bytes long on a 64-bit target.
- The 'size' field is a uint32_t, which causes a pointer mismatch with a lot of API calls that are looking for a size_t.
The first is only an issue if either we are expecting this param to be a specific size, or we expect some kind of correlation between 'buffer' and 'a', and 'size' and 'b'.
The second means that instead of being able to use ¶m[n].memref.size, it is necessary to do something like:
uint32_t local_size;
...
local_size = param[n].memref.size; result = call(..., &local_size, ...); param[n].memref.size = local_size;
in addition to security concerns with overflow on the size fields.
My question then, how do we want to handle this? Assuming we want to make the TEE 64-bit compatible, what is the right answer? As well, what do relevant standards/specs say about this type (the file right above this definition says based on "GP TEE Internal API Specification Version 0.11", which I have not seen).
Thanks, David
On Mon, Apr 18, 2016 at 11:51 PM, David Brown david.brown@linaro.org wrote:
Looking at the TEE_Param definition:
typedef union { struct { void *buffer; uint32_t size; } memref; struct { uint32_t a; uint32_t b; } value; } TEE_Param;
there seems to be an implicit assumption of 32-bit pointers. There are two things going on here:
The 'memref' struct is larger than the 'value' structure, since the 'void *' is 8-bytes long on a 64-bit target.
The 'size' field is a uint32_t, which causes a pointer mismatch with a lot of API calls that are looking for a size_t.
The first is only an issue if either we are expecting this param to be a specific size, or we expect some kind of correlation between 'buffer' and 'a', and 'size' and 'b'.
The second means that instead of being able to use ¶m[n].memref.size, it is necessary to do something like:
uint32_t local_size; ... local_size = param[n].memref.size; result = call(..., &local_size, ...); param[n].memref.size = local_size;
Yes, this is inconvenient.
in addition to security concerns with overflow on the size fields.
My question then, how do we want to handle this? Assuming we want to make the TEE 64-bit compatible, what is the right answer? As well, what do relevant standards/specs say about this type (the file right above this definition says based on "GP TEE Internal API Specification Version 0.11", which I have not seen).
The comment is out of date, we're now at version 1.1. The definition is exactly as in the 1.1 spec.
We can address the overflow on the size field by checking the size before filling in the parameter for the TA. Internally in OP-TEE we're passing parameters to TAs via struct utee_params, which is later translated into a TEE_Param before TA_InvokeCommandEntryPoint() and TA_OpenSessionEntryPoint() are called. Then there's also TEE_OpenTASession() TEE_InvokeTACommand() which also uses the TEE_Param type.
I think we should raise this issue with GP.
Regards, Jens
Hello,
Based on GP discussion ,and the unofficial and unreleased GP API Core Spec 1.1.1, the "size" field type has been reverted to size_t.
Cheers,
Cedric.
On 19 April 2016 at 08:30, Jens Wiklander jens.wiklander@linaro.org wrote:
On Mon, Apr 18, 2016 at 11:51 PM, David Brown david.brown@linaro.org wrote:
Looking at the TEE_Param definition:
typedef union { struct { void *buffer; uint32_t size; } memref; struct { uint32_t a; uint32_t b; } value; } TEE_Param;
there seems to be an implicit assumption of 32-bit pointers. There are two things going on here:
The 'memref' struct is larger than the 'value' structure, since the 'void *' is 8-bytes long on a 64-bit target.
The 'size' field is a uint32_t, which causes a pointer mismatch with a lot of API calls that are looking for a size_t.
The first is only an issue if either we are expecting this param to be a specific size, or we expect some kind of correlation between 'buffer' and 'a', and 'size' and 'b'.
The second means that instead of being able to use ¶m[n].memref.size, it is necessary to do something like:
uint32_t local_size; ... local_size = param[n].memref.size; result = call(..., &local_size, ...); param[n].memref.size = local_size;Yes, this is inconvenient.
in addition to security concerns with overflow on the size fields.
My question then, how do we want to handle this? Assuming we want to make the TEE 64-bit compatible, what is the right answer? As well, what do relevant standards/specs say about this type (the file right above this definition says based on "GP TEE Internal API Specification Version 0.11", which I have not seen).
The comment is out of date, we're now at version 1.1. The definition is exactly as in the 1.1 spec.
We can address the overflow on the size field by checking the size before filling in the parameter for the TA. Internally in OP-TEE we're passing parameters to TAs via struct utee_params, which is later translated into a TEE_Param before TA_InvokeCommandEntryPoint() and TA_OpenSessionEntryPoint() are called. Then there's also TEE_OpenTASession() TEE_InvokeTACommand() which also uses the TEE_Param type.
I think we should raise this issue with GP.
Regards, Jens _______________________________________________ Tee-dev mailing list Tee-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/tee-dev
-
Cedric Chaumont -
David Brown -
Jens Wiklander