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/incl...
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