linaro-swg / linux

Linux kernel source tree
Other
41 stars 79 forks source link

optee: check shm reference are consistent in offset/size #61

Closed etienne-lms closed 6 years ago

etienne-lms commented 6 years ago

This change prevents userland from referencing TEE shared memory outside the area initially allocated by its owner. Prior this change an application could not reference or access memory it did not own but it could reference memory not explicitly allocated by owner but still allocated to the owner due to the memory allocation granule.

Related to LHG-283.

Reported-by: Alexandre Jutras alexandre.jutras@nxp.com Signed-off-by: Etienne Carriere etienne.carriere@linaro.org

etienne-lms commented 6 years ago

Discussion with Alex (LHG-283) on this tended to propose such check (offset/size inside initially alloced shm) in the libteec. Maybe it would be a better place than the optee kernel driver.

Note also that this change is useful only for buffers in the static shm and the sdp memories. Dynamic shm buffer are registered into the Tee with their offset and size, hence outbound reference at invocation time, even inside the memory allocation/mapping granule, are rejected from the Tee core (if i did not misunderstand the code).

jenswi-linaro commented 6 years ago

The kernel should check the bounds for these shm references regardless if the secure world would do so too or not. Wouldn't it be better to check this in params_from_user() instead? This is the earliest place and there's already some sanitation going on.

etienne-lms commented 6 years ago

ok, make sense (in case you doubted). i will update the p-r. I guess some tests in xtest would be nice. I made something in the sdp part (xtest 1014), I will push it. I will also update the xtest regression 1018 you recently got merged accordingly (since it currently tests out bound above the 4kB granule, it never falls into this grey land).

etienne-lms commented 6 years ago

Will preparing the update i was thinking of the kernel client api: moving these checks in params_from_user() we will only check the userland references not the kernel land clients. I think this is ok (programming errors in kernel land are likely to break everything and this tiny sanity tests will not help that much). Do you think it is ok too?

etienne-lms commented 6 years ago

comments addressed

jenswi-linaro commented 6 years ago

Looks good to me.

jenswi-linaro commented 6 years ago

@jforissier, please merge this. @etienne-lms, please post this on LKML.

etienne-lms commented 6 years ago

@jenswi-linaro, patch posted: https://lkml.org/lkml/2018/4/27/431

edited: 2nd try :) https://lkml.org/lkml/2018/4/29/39