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

From: joeyli
Date: Tue Oct 02 2018 - 03:55:16 EST


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