On Mon, 29 Jul 2019 at 12:39, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi Sumit,
On Tue, Jul 9, 2019 at 11:36 AM Sumit Garg sumit.garg@linaro.org wrote:
On Tue, 9 Jul 2019 at 12:33, Jens Wiklander jens.wiklander@linaro.org wrote:
On Tue, Jul 09, 2019 at 11:26:19AM +0530, Sumit Garg wrote:
Thanks Jens for your comments.
On Mon, 8 Jul 2019 at 21:09, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi Sumit,
On Thu, Jun 13, 2019 at 04:00:29PM +0530, Sumit Garg wrote:
There are use-cases where user-space shouldn't be allowed to communicate directly with a TEE device which is dedicated to provide a specific service for a kernel client. So add a private login method for kernel clients and disallow user-space to open-session using this login method.
Signed-off-by: Sumit Garg sumit.garg@linaro.org
drivers/tee/tee_core.c | 6 ++++++ include/uapi/linux/tee.h | 2 ++ 2 files changed, 8 insertions(+)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index 0f16d9f..4581bd1 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -334,6 +334,12 @@ static int tee_ioctl_open_session(struct tee_context *ctx, goto out; }
if (arg.clnt_login == TEE_IOCTL_LOGIN_REE_KERNEL) {TEE_IOCTL_LOGIN_REE_KERNEL is defined as 0x80000000 which is in the range specified and implementation defined by the GP spec. I wonder if we shouldn't filter the entire implementation defined range instead of just this value.
Agree. Will rather check for entire implementation defined range: 0x80000000 - 0xFFFFFFFF.
I had a second thought on this. It would be more restrictive for user-space TEE client library which may need to use implementation defined login method. So either we could define specific ranges for kernel and user-space or we can start with single login method reserved for kernel.
I think we should reserve a range for kernel internal use. Only reserving a single single login for kernel could force us to restrict the API once more later, better to take a chunk now and be done with it. Half of 0x80000000 - 0xFFFFFFFF is probably more than enough too to leave a range for user space too.
Ok then, will rather reserve this range for kernel.
pr_err("login method not allowed for user-space client\n");pr_debug(), if it's needed at all.
Ok will use pr_debug() instead.
rc = -EPERM;goto out;}rc = ctx->teedev->desc->ops->open_session(ctx, &arg, params); if (rc) goto out;diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h index 4b9eb06..f33c69c 100644 --- a/include/uapi/linux/tee.h +++ b/include/uapi/linux/tee.h @@ -172,6 +172,8 @@ struct tee_ioctl_buf_data { #define TEE_IOCTL_LOGIN_APPLICATION 4 #define TEE_IOCTL_LOGIN_USER_APPLICATION 5 #define TEE_IOCTL_LOGIN_GROUP_APPLICATION 6 +/* Private login method for REE kernel clients */
It's worth noting that this is filtered by the TEE framework, compared to everything else which is treated opaquely.
IIUC, you are referring to login filter in optee_os. Change to prevent filter for this login method is part of this PR [1].
No, I was referring to the changes in tee_ioctl_open_session() above. It's relevant for user space to know since it will be prevented from using that range of login identifiers.
Ok, so you mean to extend the comment here for user-space to know that this login method/range is filtered by the TEE framework. Will do that.
This will restrict the user space API, but I think the risk of breakage is minimal as OP-TEE is the only in-tree driver registering in the TEE framework. I'm not aware of any out-of-tree drivers registering.
I am not sure if I follow you here. How do you expect this change to break out-of-tree TEE driver registration?
It's a change in common code that put restrictions on the API.
Okay.
-Sumit
Thanks, Jens
-Sumit
Thanks, Jens
-Sumit
+#define TEE_IOCTL_LOGIN_REE_KERNEL 0x80000000
/**
- struct tee_ioctl_param - parameter
-- 2.7.4
Thanks, Jens