Re: [PATCH 1/3] tee: add support for session's client UUID generation

[Vesa JÃÃskelÃinen]

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