core: tee_svc.c: allow to pass non-NULL memref of size 0 #6405
Conversation
|
I do not often have to say this, but the commit message is a bit long with all the citations from the specs. It takes a while to see what you're trying to achieve. Just describing the what and why should be enough in this case. It's OK with a few references, but save those after the what and why. Please add a test case for this. A new case 1041 if you can't find a relevant one to extend. |
vincent-mailhol
force-pushed
the
svc-allow-memref-of-size-0
branch
from
b79a3e9
to
eb01587
Compare
I amended the commit description (I left the PR text untouched). Please let me know if this addresses your concerns.
It took me a bit of time to figure out that you were referring to another code base. I never used |
You can easily create a full development environment as explained here: https://optee.readthedocs.io/en/latest/building/devices/qemu.html#qemu-v8 |
There was a problem hiding this comment.
I agree with this change.
As Jens said, a test in optee_test would be nice (for maintenance purpose) and commit message could be simplified. Suggestion:
core: tee_svc.c: allow to pass non-NULL memref of size 0
Allows TAs to pass non-NULL memref of size zero to other TAs by
changing the non-NULL pointer into a NULL one in such a case.
GP TEE Internal Core API does not forbid such memref parameter
whereas the previous implementation generated a
TEE_ERROR_BAD_PARAMETERS error code when converting such memref
buffer pointer into a physical memory address.
This change is specifically needed to allow a TA to forward a REE
client memref for which GP TEE Client API explicitly allows such
non-NULL address zero sized memref. It also makes the TA implementation
more flexible when dealing with its own memref.
Note: this change impacts both TEE_OpenTASession() and
TEE_InvokeTACommand().
vincent-mailhol
force-pushed
the
svc-allow-memref-of-size-0
branch
from
eb01587
to
2ced958
Compare
@jforissier Thanks for the suggestion. I am aware of this. I could use it but I am not familiar with repo which makes it hard for me to modify (and I have no plan to invest time into studying repo). I am using the different modules my way. It takes a bit more time to onboard a new piece to the project, but I enjoy the flexibility. I solved my last issues, so now I am fine.
@etienne-lms Yes, as I commented, I will do it, but be patient. I did not plan for this extra step when submitting my patch, and, as always, time being a scarce resource, I prefer not to commit on when I will do it. Thanks for the rephrasing. I like it. I did small changes:
|
Good to know 👍 Actually you don't have to know anything about |
Yes, very few steps, like this. Although I'd just use the QEMU v8 manifest (like Jerome linked to above) nowadays instead of default v7. |
We're usually not merging untested code. It's also easier to review code if there's an example that exercises it and shows that it works. |
Refer to my sentence just before the one you quoted:
I am doing this on my free time, so please forgive if my progress is slow. If you think this is taking too long, maybe I can close the pull request and reopen it when ready? |
Add a subtest to assert that the implementation allows to forward non-NULL memref from a TA to another TA. Regression 1016 already contains tests related to forwarding memref between TAs. Thus extend this existing test with the subtest described above instead of writing a new one. The Global Platform specification allows this, however, at the time of writing, optee-os will panic. A fix is proposed at [1]. [1] core: tee_svc.c: allow to pass non-NULL memref of size 0 Link: OP-TEE/optee_os#6405 Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
|
@jenswi-linaro and @etienne-lms, as promised here is a test: OP-TEE/optee_test#708. Of course, merging this PR is a prerequisite for the test to success. |
Add a subtest to assert that the implementation allows to forward non-NULL memref from a TA to another TA. Regression 1016 already contains tests related to forwarding memref between TAs. Thus extend this existing test with the subtest described above instead of writing a new one. The Global Platform specification allows this, however, at the time of writing, optee-os will panic. A fix is proposed at [1]. [1] core: tee_svc.c: allow to pass non-NULL memref of size 0 Link: OP-TEE/optee_os#6405 Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
There was a problem hiding this comment.
Hi Vincent, thanks for the optee_test counter part. It will help this P-R to land.
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for this change.
@jforissier, could you enable CI test for this P-R and for the related optee_test P-R OP-TEE/optee_test#708?
Add a subtest to assert that the implementation allows to forward non-NULL memref from a TA to another TA. Regression 1016 already contains tests related to forwarding memref between TAs. Thus extend this existing test with the subtest described above instead of writing a new one. The Global Platform specification allows this, however, at the time of writing, optee-os will panic. A fix is proposed at [1]. [1] core: tee_svc.c: allow to pass non-NULL memref of size 0 Link: OP-TEE/optee_os#6405 Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Allow TAs to pass non-NULL memref of size zero to other TAs by
changing the non-NULL pointer into a NULL one in such a case. GP TEE
Internal Core API does not forbid such memref parameter [1] whereas
the previous implementation generated a TEE_ERROR_BAD_PARAMETERS error
code when converting such memref buffer pointer into a physical memory
address.
This change is specifically needed to allow a TA to forward a REE
client memref for which GP TEE Client API explicitly allows such
non-NULL address zero sized memref [2]. It also makes the TA
implementation more flexible when dealing with its own memref.
[1] TEE Internal Core API Specification – Public Release v1.3.1,
§4.9.4 "Operation Parameters in the Internal Client API"
Table 4-15: "Interpretation of params[i] on Entry to Internal Client
API"
[2] TEE Client API Specification v1.0, §4.5.4 TEEC_RegisterSharedMemory,
paragraph "Implementers' Notes"
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
vincent-mailhol
force-pushed
the
svc-allow-memref-of-size-0
branch
from
2ced958
to
fbe99a6
Compare
Add a subtest to assert that the implementation allows to forward non-NULL memref from a TA to another TA. Regression 1016 already contains tests related to forwarding memref between TAs. Thus extend this existing test with the subtest described above instead of writing a new one. The Global Platform specification allows this, however, at the time of writing, optee-os will panic. A fix is proposed at [1]. [1] core: tee_svc.c: allow to pass non-NULL memref of size 0 Link: OP-TEE/optee_os#6405 Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Add a subtest to assert that the implementation allows to forward non-NULL memref from a TA to another TA. Regression 1016 already contains tests related to forwarding memref between TAs. Thus extend this existing test with the subtest described above instead of writing a new one. The Global Platform specification allows this, however, at the time of writing, optee-os will panic. A fix is proposed at [1]. [1] core: tee_svc.c: allow to pass non-NULL memref of size 0 Link: OP-TEE/optee_os#6405 Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Add a subtest to assert that the implementation allows to forward non-NULL memref from a TA to another TA. Regression 1016 already contains tests related to forwarding memref between TAs. Thus extend this existing test with the subtest described above instead of writing a new one. The Global Platform specification allows this, however, at the time of writing, optee-os will panic. A fix is proposed at [1]. [1] core: tee_svc.c: allow to pass non-NULL memref of size 0 Link: OP-TEE/optee_os#6405 Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Add a subtest to assert that the implementation allows to forward non-NULL memref from a TA to another TA. Regression 1016 already contains tests related to forwarding memref between TAs. Thus extend this existing test with the subtest described above instead of writing a new one. The Global Platform specification allows this, however, at the time of writing, optee-os will panic. A fix is proposed at [1]. [1] core: tee_svc.c: allow to pass non-NULL memref of size 0 Link: OP-TEE/optee_os#6405 Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
When calling
TEE_InvokeTACommand()with a non-NULL memref of size zero, functiontee_svc_copy_param()will returnTEE_ERROR_BAD_PARAMETERSresulting in a panic later on.In regard to the
TEE_PARAM_TYPE_MEMREF_{INPUT,OUTPUT,INOUT}types, the Global Platform specification says [1]:Note that the specification only specifies that
NULLimplies a size of zero. The reciprocal: "a size of zero impliesNULL" is undefined.It is even more confusing considering that non-
NULLbuffer with a size of zero are explicitly allowed in the client API [2]:This could become a pitfall if a TA forwards a memref received from the REE to another TA.
Allow the TAs to pass non-NULL memref of size zero to other TAs through
TEE_InvokeTACommand()by changing the non-NULL pointer into aNULLone in such a case.[1] TEE Internal Core API Specification – Public Release v1.3.1,
§4.9.4 "Operation Parameters in the Internal Client API"
Table 4-15: "Interpretation of params[i] on Entry to Internal Client API"
[2] TEE Client API Specification v1.0,
§4.5.4 TEEC_RegisterSharedMemory,
paragraph "Implementers' Notes"