Messages in this thread | | | Subject | Re: [PATCH 1/3] tee: add support for session's client UUID generation | From | Vesa Jääskeläinen <> | Date | Sat, 25 Apr 2020 09:16:57 +0300 |
| |
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
| |