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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 25 Apr 2020 09:16:57 +0300
From:   Vesa Jääskeläinen 
        <vesa.jaaskelainen@...sala.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     op-tee@...ts.trustedfirmware.org,
        Jens Wiklander <jens.wiklander@...aro.org>,
        Rijo Thomas <Rijo-john.Thomas@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Devaraj Rangasamy <Devaraj.Rangasamy@....com>,
        Hongbo Yao <yaohongbo@...wei.com>,
        Colin Ian King <colin.king@...onical.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] tee: add support for session's client UUID generation

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