Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[joeyli]

Hi Jann,

Thanks for your review and very sorry for my delay!

On Thu, Sep 13, 2018 at 04:31:18PM +0200, Jann Horn wrote:
> +cc keyrings list
> 
> On Thu, Sep 13, 2018 at 4:08 PM Lee, Chun-Yi <joeyli.kernel@xxxxxxxxx> wrote:
> > This patch adds a snapshot keys handler for using the key retention
> > service api to create keys for snapshot image encryption and
> > authentication.
[...snip]
> > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +                             const char *buf, size_t n)
> > +{
> > +       int error = 0;
> > +       char *p;
> > +       int len;
> > +
> > +       if (!capable(CAP_SYS_ADMIN))
> > +               return -EPERM;
> 
> This is wrong, you can't use capable() in a write handler. You'd have
> to use file_ns_capable(), and I think sysfs currently doesn't give you
> a pointer to the struct file.
> If you want to do this in a write handler, you'll have to either get
> rid of this check or plumb through the cred struct pointer.
> Alternatively, you could use some interface that doesn't go through a
> write handler.
>

Thank you for point out this problem.

Actually the evm_write_key() is the example for my code. The
difference is that evm creates interface file on securityfs, but my
implementation is on sysfs:

security/integrity/evm/evm_secfs.c

static ssize_t evm_write_key(struct file *file, const char __user *buf,
			     size_t count, loff_t *ppos)
{
	int i, ret;

	if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_SETUP)) 
		return -EPERM;
...

On the other hand, the writing handler of /sys/power/wake_lock also
uses capable() to check the CAP_BLOCK_SUSPEND capability: 

kernel/power/main.c     
static ssize_t wake_lock_store(struct kobject *kobj,
			       struct kobj_attribute *attr,
			       const char *buf, size_t n)
{
	int error = pm_wake_lock(buf);
	return error ? error : n;
}
power_attr(wake_lock);

kernel/power/wakelock.c
int pm_wake_lock(const char *buf)
{
...
	if (!capable(CAP_BLOCK_SUSPEND))
		return -EPERM;
...


So I confused for when can capable() be used in sysfs interface? Is
capable() only allowed in reading handler? Why the writing handler
of securityfs can use capable()?

> > +
> > +static int user_key_init(void)
> > +{
> > +       struct user_key_payload *ukp;
> > +       struct key *key;
> > +       int err = 0;
> > +
> > +       pr_debug("%s\n", __func__);
> > +
> > +       /* find out swsusp-key */
> > +       key = request_key(&key_type_user, skey.key_name, NULL);
> 
> request_key() looks at current's keyring. That shouldn't happen in a
> write handler.
>

The evm_write_key() also uses request_key() but it's on securityfs. Should
I move my sysfs interface to securityfs?

> > +       if (IS_ERR(key)) {
> > +               pr_err("Request key error: %ld\n", PTR_ERR(key));
> > +               err = PTR_ERR(key);
> > +               return err;
> > +       }
> > +
> > +       down_write(&key->sem);
> > +       ukp = user_key_payload_locked(key);
> > +       if (!ukp) {
> > +               /* key was revoked before we acquired its semaphore */
> > +               err = -EKEYREVOKED;
> > +               goto key_invalid;
> > +       }
> > +       if (invalid_key(ukp->data, ukp->datalen)) {
> > +               err = -EINVAL;
> > +               goto key_invalid;
> > +       }
> > +       skey.key_len = ukp->datalen;
> > +       memcpy(skey.key, ukp->data, ukp->datalen);
> > +       /* burn the original key contents */
> > +       memzero_explicit(ukp->data, ukp->datalen);
> 
> You just zero out the contents of the supplied key? That seems very
> unidiomatic for the keys subsystem, and makes me wonder why you're
> using the keys subsystem for this in the first place. It doesn't look
> like normal use of the keys subsystem.
> 

Because I want that only one decrypted key in kernel memory. Then hibernation
can handle the key more easy. In evm_init_key(), it also burned the key
contents after evm key be initialled: 

security/integrity/evm/evm_crypto.c
int evm_init_key(void)
{
[...snip]
	/* burn the original key contents */
	memset(ekp->decrypted_data, 0, ekp->decrypted_datalen);
	up_read(&evm_key->sem);
	key_put(evm_key);
	return rc;
}

The keys subsystem already handles the interactive with userland and TPM.
That's the reason for using keys subsystem in hibernation. 

> > +key_invalid:
> > +       up_write(&key->sem);
> > +       key_put(key);
> > +
> > +       return err;
> > +}
> > +
> > +/* this function may sleeps */
> > +int snapshot_key_init(void)
> > +{
> > +       int err;
> > +
> > +       pr_debug("%s\n", __func__);
> > +
> > +       if (skey.initialized)
> > +               return 0;
> > +
> > +       hash_tfm = crypto_alloc_shash(hash_alg, 0, CRYPTO_ALG_ASYNC);
> > +       if (IS_ERR(hash_tfm)) {
> > +               pr_err("Can't allocate %s transform: %ld\n",
> > +                       hash_alg, PTR_ERR(hash_tfm));
> > +               return PTR_ERR(hash_tfm);
> > +       }
> > +
> > +       err = trusted_key_init();
> > +       if (err)
> > +               err = user_key_init();
> > +       if (err)
> > +               goto key_fail;
> > +
> > +       skey.initialized = true;
> 
> Does this need a memory barrier to prevent reordering of the
> "skey.initialized = true" assignment before the key is fully
> initialized?
>

Thanks for your reminding. I will add memory barrier here.


Thank a lot!
Joey Lee