All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>,
	"Theodore Y. Ts'o" <tytso@mit.edu>,
	Jaegeuk Kim <jaegeuk@kernel.org>,
	kernel@pengutronix.de, James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	James Bottomley <jejb@linux.ibm.com>,
	Mimi Zohar <zohar@linux.ibm.com>,
	Sumit Garg <sumit.garg@linaro.org>,
	David Howells <dhowells@redhat.com>,
	linux-fscrypt@vger.kernel.org, linux-crypto@vger.kernel.org,
	linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org, keyrings@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] fscrypt: support trusted keys
Date: Mon, 9 Aug 2021 13:52:01 -0700	[thread overview]
Message-ID: <YRGVcaquAJiuc8bp@gmail.com> (raw)
In-Reply-To: <20210809094408.4iqwsx77u64usfx6@kernel.org>

On Mon, Aug 09, 2021 at 12:44:08PM +0300, Jarkko Sakkinen wrote:
> > @@ -577,28 +578,44 @@ static int get_keyring_key(u32 key_id, u32 type,
> >  	key_ref_t ref;
> >  	struct key *key;
> >  	const struct fscrypt_provisioning_key_payload *payload;
> > -	int err;
> > +	int err = 0;
> >  
> >  	ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
> >  	if (IS_ERR(ref))
> >  		return PTR_ERR(ref);
> >  	key = key_ref_to_ptr(ref);
> >  
> > -	if (key->type != &key_type_fscrypt_provisioning)
> > -		goto bad_key;
> > -	payload = key->payload.data[0];
> > +	if (key->type == &key_type_fscrypt_provisioning) {
> 
> Why does fscrypt have own key type, and does not extend 'encrypted' with a
> new format [*]?

Are you referring to the "fscrypt-provisioning" key type?  That is an existing
feature (which in most cases isn't used, but there is a use case that requires
it), not something being added by this patch.  We just needed a key type where
userspace can add a raw key to the kernel and not be able to read it back (so
like the "logon" key type), but also have the kernel enforce that that key is
only used for fscrypt with a particular KDF version, and not with other random
kernel features.  The "encrypted" key type wouldn't have worked for this at all;
it's a totally different thing.

> > +	} else if (IS_REACHABLE(CONFIG_TRUSTED_KEYS) && key->type == &key_type_trusted) {
> > +		struct trusted_key_payload *tkp;
> > +
> > +		/* avoid reseal changing payload while we memcpy key */
> > +		down_read(&key->sem);
> > +		tkp = key->payload.data[0];
> > +		if (!tkp || tkp->key_len < FSCRYPT_MIN_KEY_SIZE ||
> > +		    tkp->key_len > FSCRYPT_MAX_KEY_SIZE) {
> > +			up_read(&key->sem);
> > +			err = -EINVAL;
> > +			goto out_put;
> > +		}
> > +
> > +		secret->size = tkp->key_len;
> > +		memcpy(secret->raw, tkp->key, secret->size);
> > +		up_read(&key->sem);
> > +	} else {
> 
> 
> I don't think this is right, or at least it does not follow the pattern
> in [*]. I.e. you should rather use trusted key to seal your fscrypt key.

What's the benefit of the extra layer of indirection over just using a "trusted"
key directly?  The use case for "encrypted" keys is not at all clear to me.

- Eric

  parent reply	other threads:[~2021-08-09 20:52 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-06 15:09 Ahmad Fatoum
2021-08-09  9:44 ` Jarkko Sakkinen
2021-08-09 10:00   ` Ahmad Fatoum
2021-08-09 10:02     ` Ahmad Fatoum
2021-08-10 18:02     ` Jarkko Sakkinen
2021-08-09 20:52   ` Eric Biggers [this message]
2021-08-10 18:06     ` Jarkko Sakkinen
2021-08-10 18:46       ` Eric Biggers
2021-08-10 21:21         ` Jarkko Sakkinen
2021-08-10 21:27           ` Eric Biggers
2021-08-11  0:17             ` Jarkko Sakkinen
2021-08-11 11:34               ` Mimi Zohar
2021-08-11 17:16                 ` Eric Biggers
2021-08-12  0:54                   ` Mimi Zohar
2021-08-17 13:04                     ` Ahmad Fatoum
2021-08-17 13:55                       ` Mimi Zohar
2021-08-17 14:13                         ` Ahmad Fatoum
2021-08-17 14:24                           ` Mimi Zohar
2021-08-18  2:09                             ` Jarkko Sakkinen
2021-08-18  4:53                             ` Sumit Garg
2021-08-09 21:24 ` Eric Biggers
2021-08-10  7:41   ` Ahmad Fatoum
2021-08-10 17:35     ` Eric Biggers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YRGVcaquAJiuc8bp@gmail.com \
    --to=ebiggers@kernel.org \
    --cc=a.fatoum@pengutronix.de \
    --cc=dhowells@redhat.com \
    --cc=jaegeuk@kernel.org \
    --cc=jarkko@kernel.org \
    --cc=jejb@linux.ibm.com \
    --cc=jmorris@namei.org \
    --cc=kernel@pengutronix.de \
    --cc=keyrings@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=serge@hallyn.com \
    --cc=sumit.garg@linaro.org \
    --cc=tytso@mit.edu \
    --cc=zohar@linux.ibm.com \
    --subject='Re: [PATCH v2] fscrypt: support trusted keys' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.