From mboxrd@z Thu Jan  1 00:00:00 1970
From: jarkko.sakkinen@linux.intel.com (Jarkko Sakkinen)
Date: Fri, 31 Aug 2018 14:05:43 +0300
Subject: [RFC] KEYS: inject an MKTME specific safety check in the keyctl
	revoke path
In-Reply-To: <20180814020538.GA18424@alison-desk.jf.intel.com>
References: <20180814020538.GA18424@alison-desk.jf.intel.com>
Message-ID: <20180831110543.GB9346@linux.intel.com>
To: linux-security-module@vger.kernel.org
List-Id: linux-security-module.vger.kernel.org

On Mon, Aug 13, 2018 at 07:05:38PM -0700, Alison Schofield wrote:
> This RFC is asking for feedback on a problem I'm running into using
> the Kernel Key Service for MKTME (MultiKey Total Memory Encryption).
> 
> I previously posted an RFC with the proposal to create a new key type
> "mktme" to support MKTME (Multi-Key Total Memory Encryption).
> https://www.spinics.net/lists/keyrings/msg03702.html
> 
> The MKTME key service maps userspace keys to hardware keyids. Those
> keys are used in a new system call that encrypts memory. The keys
> need to be tightly controlled. One example is that userspace keys
> should not be revoked while the hardware keyid slot is still in use.

What is the new syscall? Can you point to a description?

> 
> The KEY_FLAG_KEEP bit offers good control. The mktme service uses that
> bit to prevent userspace keys from disappearing without the service
> being notified.
> 
> Problem is that we need a safe and synchronous way to revoke keys. The
> way .revoke methods function now, the key service type is called late
> in the revoke process. The mktme key service has no means to reject the
> request. So, even if the mktme service sanity checks the request in its
> .revoke method, it's too late to do anything about it.

I have trouble understanding the problem. I'm just seeing what you need
but I don't know why you need it...

> 
> This proposal inserts an MKTME specific check earlier into the existing
> keyctl <revoke> path. If it is safe to revoke the key, mktme key service
> will turn off KEY_FLAG_KEEP and let the revoke continue (and succeed).
> Otherwise, not safe, KEY_FLAG_KEEP stays on, and the revoke continues
> (and fails).
> 
> I considered proposing a new keyctl <option> just for this mktme 'safe
> revoke' request. I wonder if that might be the preferred method for
> inserting this type specific behavior?
> 
> Hoping that from this description and the diff you can understand the
> issue and suggest alternative solutions if needed. Thanks for looking!
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  security/keys/internal.h   |  6 ++++++
>  security/keys/keyctl.c     | 14 ++++++++++++++
>  security/keys/mktme_keys.c | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 9f8208dc0e55..1b6425d0d1ab 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -316,4 +316,10 @@ static inline void key_check(const struct key *key)
>  
>  #endif
>  
> +#ifdef CONFIG_MKTME_KEYS
> +extern int mktme_safe_revoke(struct key *key);
> +#else
> +static inline int mktme_safe_revoke(struct key *key) { return 0; }
> +#endif /* CONFIG_MKTME_KEYS */
> +
>  #endif /* _INTERNAL_H */
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 1ffe60bb2845..7b5f98af1d54 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -387,6 +387,20 @@ long keyctl_revoke_key(key_serial_t id)
>  
>  	key = key_ref_to_ptr(key_ref);
>  	ret = 0;
> +
> +	/*
> +	 * MKTME (Multi-Key Total Memory Encryption) Keys require a
> +	 * sanity check before allowing a revoke. If the sanity check
> +	 * passes, the mktme key service will clear KEY_FLAG_KEEP bit
> +	 * and the revoke will proceed.
> +	 */

I think this should be moved to the kdoc of the function.

> +	if (strcmp(key->type->name, "mktme") == 0)  {
> +		if (mktme_safe_revoke(key)) {
> +			ret = -EINVAL;
> +			goto error;
> +		}
> +	}

What about -EBUSY instead? There is something wrong here because you
ignore the return value of mktme_safe_revoke() and substitute your own
return value. Should be:

ret = mktme_safe_revoke(key);
if (ret)
	goto error;

I think this should renamed simply as mktme_revoke_key() and document in
the function long description what measures it need to take to revoke a
key.

> +
>  	if (test_bit(KEY_FLAG_KEEP, &key->flags))
>  		ret = -EPERM;
>  	else
> diff --git a/security/keys/mktme_keys.c b/security/keys/mktme_keys.c
> index b937bbe6bcdb..887b483d7b38 100644
> --- a/security/keys/mktme_keys.c
> +++ b/security/keys/mktme_keys.c
> @@ -67,6 +67,39 @@ static int mktme_clear_programmed_key(int keyid)
>  	return ret;
>  }
>  
> +/*
> + * mktme_safe_revoke() is called during the revoke process to
> + * determine if it is safe to revoke a key. If this check passes,
> + * the revoke proceeds, otherwise an error is returned to userspace.
> + * The important error case here is outstanding memory mappings using
> + * the corresponding hardware keyid.
> + */

Please use kdoc.

> +int mktme_safe_revoke(struct key *key)
> +{
> +	int keyid, vma_count;
> +	int ret = -1;

So you choose to use a magic number instead of relaying to the standard
errnos?

> +
> +	mktme_map_lock();
> +	keyid = mktme_map_keyid_from_serial(key->serial);
> +	if (keyid <= 0)
> +		goto out;
> +
> +	vma_count = vma_read_encrypt_ref(keyid);
> +	if (vma_count > 0) {
> +		pr_debug("mktme not freeing keyid[%d] encrypt_count[%d]\n",
> +			 keyid, vma_count);

Wasn't it busy using it? Maybe the log message could be more exact.

> +		goto out;
> +	}
> +	mktme_clear_programmed_key(keyid);
> +	/* Clearing KEY_FLAG_KEEP flag allows the revoke to proceed */

Is this comment necessary?

> +	clear_bit(KEY_FLAG_KEEP, &key->flags);
> +	ret = 0;
> +out:
> +	mktme_map_unlock();
> +	return ret;
> +}
> +
> +
> @@ -315,6 +348,7 @@ int mktme_instantiate(struct key *key, struct key_preparsed_payload *prep)
>  
>  	mktme_map_lock();
>  	ret = mktme_program_key(key->serial, kprog);
> +	set_bit(KEY_FLAG_KEEP, &key->flags);
>  	mktme_map_unlock();
>  out:
>  	kzfree(datablob);
> -- 
> 2.14.1
> 

What about just waiting by adding callback to the MMU notifier when the
count is zero and using a wait queue?

/Jarkko