lkml.org 
[lkml]   [2020]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/3] tee: add support for session's client UUID generation
From
Date
Hi Dan,

Thanks for comments!

On 2020-04-23 20:35, Dan Carpenter wrote:
> On Thu, Apr 23, 2020 at 06:16:59PM +0300, Vesa Jääskeläinen wrote:
>> +static int uuid_v5(uuid_t *uuid, const uuid_t *ns, const void *name,
>> + size_t size)
>> +{
>> + unsigned char hash[SHA1_DIGEST_SIZE];
>> + struct crypto_shash *shash = NULL;
>> + struct shash_desc *desc = NULL;
>> + int rc;
>> +
>> + shash = crypto_alloc_shash("sha1", 0, 0);
>> + if (IS_ERR(shash)) {
>> + rc = PTR_ERR(shash);
>> + pr_err("shash(sha1) allocation failed\n");
>> + return rc;
>> + }
>> +
>> + desc = kzalloc(sizeof(*desc) + crypto_shash_descsize(shash),
>> + GFP_KERNEL);
>> + if (IS_ERR(desc)) {
>
>
> kzalloc() returns NULL on error.

Err...Right. Will fix for V2. Thanks for noticing this.

I was probably confused by this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/slab.h?h=v5.7-rc2#n553

As there is no error handling case documented for kmalloc and friends --
I was just looking that as there are non-zero error cases in that
referenced line (ZERO_SIZE_PTR) so we need to check if pointer has error
value as it was with crypto_alloc_shash().

But looking at users of kzalloc then convention seems to be just check
for NULL.

Probably referenced code is then expected to cause page fault on error
case as ZERO_SIZE_PTR is not error value as such.

>
>> + rc = PTR_ERR(desc);
>> + goto out;
>
> Please choose a label name so that we can tell what the goto will do
> like "goto free_shash;".

Ok. Will update for V2.

>> + }
>> +
>> + desc->tfm = shash;
>> +
>> + rc = crypto_shash_init(desc);
>> + if (rc < 0)
>> + goto out2;
>> +
>> + rc = crypto_shash_update(desc, (const u8 *)ns, sizeof(*ns));
>> + if (rc < 0)
>> + goto out2;
>> +
>> + rc = crypto_shash_update(desc, (const u8 *)name, size);
>> + if (rc < 0)
>> + goto out2;
>> +
>> + rc = crypto_shash_final(desc, hash);
>> + if (rc < 0)
>> + goto out2;
>> +
>> + memcpy(uuid->b, hash, UUID_SIZE);
>> +
>> + /* Tag for version 5 */
>> + uuid->b[6] = (hash[6] & 0x0F) | 0x50;
>> + uuid->b[8] = (hash[8] & 0x3F) | 0x80;
>> +
>> +out2:
>> + kfree(desc);
>> +
>> +out:
>> + crypto_free_shash(shash);
>> + return rc;
>> +}
>> +
>> +int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
>> + const u8 connection_data[TEE_IOCTL_UUID_LEN])
>> +{
>> + const char *application_id = NULL;
>> + gid_t ns_grp = (gid_t)-1;
>> + kgid_t grp = INVALID_GID;
>> + char *name = NULL;
>> + int rc;
>> +
>> + if (connection_method == TEE_IOCTL_LOGIN_PUBLIC) {
>> + /* Nil UUID to be passed to TEE environment */
>> + uuid_copy(uuid, &uuid_null);
>> + return 0;
>> + }
>> +
>> + /*
>> + * In Linux environment client UUID is based on UUIDv5.
>> + *
>> + * Determine client UUID with following semantics for 'name':
>> + *
>> + * For TEEC_LOGIN_USER:
>> + * uid=<uid>
>> + *
>> + * For TEEC_LOGIN_GROUP:
>> + * gid=<gid>
>> + *
>> + */
>> +
>> + name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
>> + if (!name)
>> + return -ENOMEM;
>> +
>> + switch (connection_method) {
>> + case TEE_IOCTL_LOGIN_USER:
>> + scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
>> + current_euid().val);
>
> It doesn't make sense to use sCnprintf() instead of snprintf() if we're
> not going to preserve the error code. Probably it's better to preserve
> the error code actually so we can avoid the strlen(name) later on.

Ok. I assume you meant here using snprintf where we can capture the
error situation of too large output string. scnprintf does not seem to
have error returning capabilities.

I assume you meant something like this:

name_len = snprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
current_euid().val);
if (name_len >= TEE_UUID_NS_NAME_SIZE) {
rc = -E2BIG;
goto out_free_name;
}

Will wait a bit more for comments before sending V2.

I already updated my devel branch with these for those wanting to play
around with the updates:
https://github.com/vesajaaskelainen/linux/commits/optee-login-checks

Thanks,
Vesa Jääskeläinen

\
 
 \ /
  Last update: 2020-04-25 08:19    [W:0.067 / U:2.648 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site