Skip to content

Commit

Permalink
crypto API: make sure TEE_Attribute parameters are readable
Browse files Browse the repository at this point in the history 
Fixes OP-TEE#161.

Services that take a TEE_Attribute array for input must check that the
memory is readable before using it. This is accomplished by
check_attr_read_access(), which is either called directly by the system
service or by tee_svc_cryp_check_attr(). Buffers pointed to by 'reference'
attributes are also validated.
Then, it is no longer necessary to check accessibility in other
functions such as tee_svc_cryp_obj_store_attr_raw().

Signed-off-by: Jerome Forissier <[email protected]>
  • Loading branch information
@jforissier
jforissier committed Dec 19, 2014
1 parent e2d57f4 commit fb19c84
Showing 1 changed file with 73 additions and 38 deletions.
111 changes: 73 additions & 38 deletions core/tee/tee_svc_cryp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -901,8 +901,7 @@ TEE_Result tee_svc_cryp_obj_reset(uint32_t obj)
return TEE_SUCCESS;
}

static TEE_Result tee_svc_cryp_obj_store_attr_raw(struct tee_ta_session *sess,
uint16_t conv_func,
static TEE_Result tee_svc_cryp_obj_store_attr_raw(uint16_t conv_func,
const TEE_Attribute *attr,
void *data, size_t data_size)
{
Expand All @@ -921,9 +920,9 @@ static TEE_Result tee_svc_cryp_obj_store_attr_raw(struct tee_ta_session *sess,
/* No conversion data size has to match exactly */
if (attr->content.ref.length != data_size)
return TEE_ERROR_BAD_PARAMETERS;
return tee_svc_copy_from_user(sess, data,
attr->content.ref.buffer,
data_size);
memcpy(data, attr->content.ref.buffer, data_size);
return TEE_SUCCESS;

case TEE_TYPE_CONV_FUNC_SECRET:
if (!TEE_ALIGNMENT_IS_OK(data, struct tee_cryp_obj_secret))
return TEE_ERROR_BAD_STATE;
Expand All @@ -934,23 +933,12 @@ static TEE_Result tee_svc_cryp_obj_store_attr_raw(struct tee_ta_session *sess,
(data_size - sizeof(struct tee_cryp_obj_secret)))
return TEE_ERROR_BAD_PARAMETERS;

res = tee_svc_copy_from_user(sess, obj + 1,
attr->content.ref.buffer,
attr->content.ref.length);
if (res == TEE_SUCCESS)
obj->key_size = attr->content.ref.length;
return res;
memcpy(obj + 1, attr->content.ref.buffer,
attr->content.ref.length);
obj->key_size = attr->content.ref.length;
return TEE_SUCCESS;

case TEE_TYPE_CONV_FUNC_BIGNUM:
/* Check data can be accessed */
res = tee_mmu_check_access_rights(
sess->ctx,
TEE_MEMORY_ACCESS_READ | TEE_MEMORY_ACCESS_ANY_OWNER,
(tee_uaddr_t)attr->content.ref.buffer,
attr->content.ref.length);
if (res != TEE_SUCCESS)
return res;

/*
* Read the array of bytes (stored in attr->content.ref.buffer)
* and convert it to a bignum (pointed to by data)
Expand Down Expand Up @@ -978,17 +966,50 @@ static TEE_Result tee_svc_cryp_obj_store_attr_raw(struct tee_ta_session *sess,
}
}


static TEE_Result check_attr_read_access(struct tee_ta_ctx *ctx,
const TEE_Attribute *attrs,
uint32_t attr_count)
{
TEE_Result res;
uint32_t n;

res = tee_mmu_check_access_rights(ctx, TEE_MEMORY_ACCESS_READ |
TEE_MEMORY_ACCESS_ANY_OWNER,
(tee_uaddr_t)attrs,
attr_count * sizeof(TEE_Attribute));

if (res != TEE_SUCCESS)
return res;

for (n = 0; n < attr_count; n++) {
if (attrs[n].attributeID & TEE_ATTR_BIT_VALUE)
continue;
res = tee_mmu_check_access_rights(ctx, TEE_MEMORY_ACCESS_READ |
TEE_MEMORY_ACCESS_ANY_OWNER,
(tee_uaddr_t)
attrs[n].content.ref.buffer,
attrs[n].content.ref.length);
if (res != TEE_SUCCESS)
return res;
}

return TEE_SUCCESS;
}

enum attr_usage {
ATTR_USAGE_POPULATE,
ATTR_USAGE_GENERATE_KEY
};

static TEE_Result tee_svc_cryp_check_attr(
enum attr_usage usage,
const struct tee_cryp_obj_type_props *type_props,
TEE_Attribute *attrs,
uint32_t attr_count)
static TEE_Result tee_svc_cryp_check_attr(struct tee_ta_ctx *ctx,
enum attr_usage usage,
const struct tee_cryp_obj_type_props
*type_props,
TEE_Attribute *attrs,
uint32_t attr_count)
{
TEE_Result res;
uint32_t required_flag;
uint32_t opt_flag;
bool all_opt_needed;
Expand All @@ -997,6 +1018,10 @@ static TEE_Result tee_svc_cryp_check_attr(
uint32_t attrs_found = 0;
size_t n;

res = check_attr_read_access(ctx, attrs, attr_count);
if (res != TEE_SUCCESS)
return res;

if (usage == ATTR_USAGE_POPULATE) {
required_flag = TEE_TYPE_ATTR_REQUIRED;
opt_flag = TEE_TYPE_ATTR_OPTIONAL_GROUP;
Expand Down Expand Up @@ -1054,7 +1079,6 @@ static TEE_Result tee_svc_cryp_check_attr(
}

static TEE_Result tee_svc_cryp_obj_populate_type(
struct tee_ta_session *sess,
struct tee_obj *o,
const struct tee_cryp_obj_type_props *type_props,
const TEE_Attribute *attrs,
Expand Down Expand Up @@ -1083,7 +1107,7 @@ static TEE_Result tee_svc_cryp_obj_populate_type(

res =
tee_svc_cryp_obj_store_attr_raw(
sess, type_props->type_attrs[idx].conv_func,
type_props->type_attrs[idx].conv_func,
attrs + n, raw_data, raw_size);
if (res != TEE_SUCCESS)
return res;
Expand Down Expand Up @@ -1139,13 +1163,12 @@ TEE_Result tee_svc_cryp_obj_populate(uint32_t obj, TEE_Attribute *attrs,
if (!type_props)
return TEE_ERROR_NOT_IMPLEMENTED;

res = tee_svc_cryp_check_attr(ATTR_USAGE_POPULATE, type_props, attrs,
attr_count);
res = tee_svc_cryp_check_attr(sess->ctx, ATTR_USAGE_POPULATE,
type_props, attrs, attr_count);
if (res != TEE_SUCCESS)
return res;

res = tee_svc_cryp_obj_populate_type(sess, o, type_props, attrs,
attr_count);
res = tee_svc_cryp_obj_populate_type(o, type_props, attrs, attr_count);
if (res == TEE_SUCCESS)
o->info.handleFlags |= TEE_HANDLE_FLAG_INITIALIZED;

Expand Down Expand Up @@ -1279,7 +1302,6 @@ static TEE_Result tee_svc_obj_generate_key_dsa(
}

static TEE_Result tee_svc_obj_generate_key_dh(
struct tee_ta_session *sess,
struct tee_obj *o, const struct tee_cryp_obj_type_props *type_props,
uint32_t key_size __unused,
const TEE_Attribute *params, uint32_t param_count)
Expand All @@ -1292,8 +1314,8 @@ static TEE_Result tee_svc_obj_generate_key_dh(
TEE_ASSERT(sizeof(struct dh_keypair) == o->data_size);

/* Copy the present attributes into the obj before starting */
res = tee_svc_cryp_obj_populate_type(
sess, o, type_props, params, param_count);
res = tee_svc_cryp_obj_populate_type(o, type_props, params,
param_count);
if (res != TEE_SUCCESS)
return res;

Expand Down Expand Up @@ -1356,8 +1378,9 @@ TEE_Result tee_svc_obj_generate_key(
if (key_size > type_props->max_size)
return TEE_ERROR_NOT_SUPPORTED;

res = tee_svc_cryp_check_attr(ATTR_USAGE_GENERATE_KEY, type_props,
(TEE_Attribute *)params, param_count);
res = tee_svc_cryp_check_attr(sess->ctx, ATTR_USAGE_GENERATE_KEY,
type_props, (TEE_Attribute *)params,
param_count);
if (res != TEE_SUCCESS)
return res;

Expand Down Expand Up @@ -1413,8 +1436,8 @@ TEE_Result tee_svc_obj_generate_key(
break;

case TEE_TYPE_DH_KEYPAIR:
res = tee_svc_obj_generate_key_dh(
sess, o, type_props, key_size, params, param_count);
res = tee_svc_obj_generate_key_dh(o, type_props, key_size,
params, param_count);
if (res != TEE_SUCCESS)
return res;
break;
Expand Down Expand Up @@ -2087,6 +2110,10 @@ TEE_Result tee_svc_cryp_derive_key(uint32_t state, const TEE_Attribute *params,
if (res != TEE_SUCCESS)
return res;

res = check_attr_read_access(sess->ctx, params, param_count);
if (res != TEE_SUCCESS)
return res;

if ((param_count != 1) ||
(params[0].attributeID != TEE_ATTR_DH_PUBLIC_VALUE))
return TEE_ERROR_BAD_PARAMETERS;
Expand Down Expand Up @@ -2519,6 +2546,10 @@ TEE_Result tee_svc_asymm_operate(uint32_t state, const TEE_Attribute *params,
if (res != TEE_SUCCESS)
return res;

res = check_attr_read_access(sess->ctx, params, num_params);
if (res != TEE_SUCCESS)
return res;

res = tee_obj_get(sess->ctx, cs->key1, &o);
if (res != TEE_SUCCESS)
return res;
Expand Down Expand Up @@ -2673,6 +2704,10 @@ TEE_Result tee_svc_asymm_verify(uint32_t state, const TEE_Attribute *params,
if (res != TEE_SUCCESS)
return res;

res = check_attr_read_access(sess->ctx, params, num_params);
if (res != TEE_SUCCESS)
return res;

res = tee_obj_get(sess->ctx, cs->key1, &o);
if (res != TEE_SUCCESS)
return res;
Expand Down

1 comment on commit fb19c84

@jenswi-linaro
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an improvement, but when we're running TEE Core in SRAM and TAs in DRAM there's a window where the pointers in the attribute list can be attacked (provided that it's possible to change content in TrustZone protected DRAM in an attack).

Since this patch any way improves the situation I recommend that we take it and later fix the remaining problem.

Reviewed-by: Jens Wiklander <[email protected]>

Please sign in to comment.