Hi Tyler
i have some new comments about this patch.
On Tuesday, July 26, 2011 06:19:33 AM Tyler Hicks wrote:
> Fixes a regression caused by b5695d04634fa4ccca7dcbc05bb4a66522f02e0b
>
> Kernel keyring keys containing eCryptfs authentication tokens should not
> be write locked when calling out to ecryptfsd to wrap and unwrap file
> encryption keys. The eCryptfs kernel code can not hold the key's write
> lock because ecryptfsd needs to request the key after receiving such a
> request from the kernel.
>
> Without this fix, all file opens and creates will timeout and fail when
> using the eCryptfs PKI infrastructure. This is not an issue when using
> passphrase-based mount keys, which is the most widely deployed eCryptfs
> configuration.
>
> Signed-off-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxx>
> Cc: Roberto Sassu <roberto.sassu@xxxxxxxxx>
> Cc: Alexis Hafner1 <haf@xxxxxxxxxxxxxx>
> Cc: <da_fox@xxxxxxxxxxxxxxxxx>
> ---
> fs/ecryptfs/keystore.c | 65
> +++++++++++++++++++++++++++-------------------- 1 files changed, 37
> insertions(+), 28 deletions(-)
>
> diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> index 27a7fef..1e6e199 100644
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -1138,13 +1138,16 @@ ecryptfs_get_auth_tok_sig(char **sig, struct
> ecryptfs_auth_tok *auth_tok)
>
> /**
> * decrypt_pki_encrypted_session_key - Decrypt the session key with the
> given auth_tok. + * @auth_tok_key: The authentication token key to unlock
> and put when done with + * @auth_tok
> * @auth_tok: The key authentication token used to decrypt the session key
> * @crypt_stat: The cryptographic context
> *
> * Returns zero on success; non-zero error otherwise.
> */
> static int
> -decrypt_pki_encrypted_session_key(struct ecryptfs_auth_tok *auth_tok,
> +decrypt_pki_encrypted_session_key(struct key *auth_tok_key,
> + struct ecryptfs_auth_tok *auth_tok,
> struct ecryptfs_crypt_stat *crypt_stat)
> {
> u8 cipher_code = 0;
> @@ -1159,14 +1162,15 @@ decrypt_pki_encrypted_session_key(struct
> ecryptfs_auth_tok *auth_tok, if (rc) {
> printk(KERN_ERR "Unrecognized auth tok type: [%d]\n",
> auth_tok->token_type);
> - goto out;
> + goto out_unlock;
> }
> rc = write_tag_64_packet(auth_tok_sig, &(auth_tok->session_key),
> &payload, &payload_len);
> if (rc) {
> ecryptfs_printk(KERN_ERR, "Failed to write tag 64 packet\n");
> - goto out;
> + goto out_unlock;
> }
> + up_write(&(auth_tok_key->sem));
> rc = ecryptfs_send_message(payload, payload_len, &msg_ctx);
> if (rc) {
> ecryptfs_printk(KERN_ERR, "Error sending message to "
> @@ -1180,12 +1184,13 @@ decrypt_pki_encrypted_session_key(struct
> ecryptfs_auth_tok *auth_tok, rc = -EIO;
> goto out;
> }
> + down_write(&(auth_tok_key->sem));
> rc = parse_tag_65_packet(&(auth_tok->session_key),
> &cipher_code, msg);
> if (rc) {
> printk(KERN_ERR "Failed to parse tag 65 packet; rc = [%d]\n",
> rc);
> - goto out;
> + goto out_unlock;
> }
> auth_tok->session_key.flags |= ECRYPTFS_CONTAINS_DECRYPTED_KEY;
> memcpy(crypt_stat->key, auth_tok->session_key.decrypted_key,
> @@ -1195,7 +1200,7 @@ decrypt_pki_encrypted_session_key(struct
> ecryptfs_auth_tok *auth_tok, if (rc) {
> ecryptfs_printk(KERN_ERR, "Cipher code [%d] is invalid\n",
> cipher_code)
> - goto out;
> + goto out_unlock;
> }
> crypt_stat->flags |= ECRYPTFS_KEY_VALID;
> if (ecryptfs_verbosity > 0) {
> @@ -1203,6 +1208,8 @@ decrypt_pki_encrypted_session_key(struct
> ecryptfs_auth_tok *auth_tok, ecryptfs_dump_hex(crypt_stat->key,
> crypt_stat->key_size);
> }
> +out_unlock:
> + up_write(&(auth_tok_key->sem));
> out:
> if (msg)
> kfree(msg);
> @@ -1868,11 +1875,6 @@ int ecryptfs_parse_packet_set(struct
> ecryptfs_crypt_stat *crypt_stat, * just one will be sufficient to decrypt
> to get the FEK. */
> find_next_matching_auth_tok:
> found_auth_tok = 0;
> - if (auth_tok_key) {
> - up_write(&(auth_tok_key->sem));
> - key_put(auth_tok_key);
> - auth_tok_key = NULL;
> - }
> list_for_each_entry(auth_tok_list_item, &auth_tok_list, list) {
> candidate_auth_tok = &auth_tok_list_item->auth_tok;
> if (unlikely(ecryptfs_verbosity > 0)) {
> @@ -1902,6 +1904,10 @@ find_next_matching_auth_tok:
> ecryptfs_printk(KERN_ERR, "Could not find a usable "
> "authentication token\n");
> rc = -EIO;
> + if (auth_tok_key) {
> + up_write(&(auth_tok_key->sem));
> + key_put(auth_tok_key);
> + }
> goto out_wipe_list;
> }
I think that unlocking the key here is not necessary,
since it can be done, in the code below, each time a
valid authentication token is found in the keyring.
It is only needed to add the code for unlocking the key
if the authentication token type is not ECRYPTFS_PRIVATE_KEY
nor ECRYPTFS_PASSWORD.
> found_matching_auth_tok:
> @@ -1909,7 +1915,8 @@ found_matching_auth_tok:
> memcpy(&(candidate_auth_tok->token.private_key),
> &(matching_auth_tok->token.private_key),
> sizeof(struct ecryptfs_private_key));
> - rc = decrypt_pki_encrypted_session_key(candidate_auth_tok,
> + rc = decrypt_pki_encrypted_session_key(auth_tok_key,
> + candidate_auth_tok,
> crypt_stat);
> } else if (candidate_auth_tok->token_type == ECRYPTFS_PASSWORD) {
> memcpy(&(candidate_auth_tok->token.password),
> @@ -1917,6 +1924,8 @@ found_matching_auth_tok:
> sizeof(struct ecryptfs_password));
> rc = decrypt_passphrase_encrypted_session_key(
> candidate_auth_tok, crypt_stat);
> + up_write(&(auth_tok_key->sem));
> + key_put(auth_tok_key);
If i understand correctly, 'candidate_auth_tok' is allocated
during the packets parsing, while the element which we
want to protect is 'matching_auth_tok'. So, in this case, it
seems that the key unlock can be performed just after the
memcpy().
Roberto Sassu
> }
> if (rc) {
> struct ecryptfs_auth_tok_list_item *auth_tok_list_item_tmp;
> @@ -1956,15 +1965,12 @@ found_matching_auth_tok:
> out_wipe_list:
> wipe_auth_tok_list(&auth_tok_list);
> out:
> - if (auth_tok_key) {
> - up_write(&(auth_tok_key->sem));
> - key_put(auth_tok_key);
> - }
> return rc;
> }
>
> static int
> -pki_encrypt_session_key(struct ecryptfs_auth_tok *auth_tok,
> +pki_encrypt_session_key(struct key *auth_tok_key,
> + struct ecryptfs_auth_tok *auth_tok,
> struct ecryptfs_crypt_stat *crypt_stat,
> struct ecryptfs_key_record *key_rec)
> {
> @@ -1979,6 +1985,8 @@ pki_encrypt_session_key(struct ecryptfs_auth_tok
> *auth_tok, crypt_stat->cipher,
> crypt_stat->key_size),
> crypt_stat, &payload, &payload_len);
> + up_write(&(auth_tok_key->sem));
> + key_put(auth_tok_key);
> if (rc) {
> ecryptfs_printk(KERN_ERR, "Error generating tag 66 packet\n");
> goto out;
> @@ -2008,6 +2016,8 @@ out:
> * write_tag_1_packet - Write an RFC2440-compatible tag 1 (public key)
> packet * @dest: Buffer into which to write the packet
> * @remaining_bytes: Maximum number of bytes that can be writtn
> + * @auth_tok_key: The authentication token key to unlock and put when done
> with + * @auth_tok
> * @auth_tok: The authentication token used for generating the tag 1
> packet * @crypt_stat: The cryptographic context
> * @key_rec: The key record struct for the tag 1 packet
> @@ -2018,7 +2028,7 @@ out:
> */
> static int
> write_tag_1_packet(char *dest, size_t *remaining_bytes,
> - struct ecryptfs_auth_tok *auth_tok,
> + struct key *auth_tok_key, struct ecryptfs_auth_tok *auth_tok,
> struct ecryptfs_crypt_stat *crypt_stat,
> struct ecryptfs_key_record *key_rec, size_t *packet_size)
> {
> @@ -2039,12 +2049,15 @@ write_tag_1_packet(char *dest, size_t
> *remaining_bytes, memcpy(key_rec->enc_key,
> auth_tok->session_key.encrypted_key,
> auth_tok->session_key.encrypted_key_size);
> + up_write(&(auth_tok_key->sem));
> + key_put(auth_tok_key);
> goto encrypted_session_key_set;
> }
> if (auth_tok->session_key.encrypted_key_size == 0)
> auth_tok->session_key.encrypted_key_size =
> auth_tok->token.private_key.key_size;
> - rc = pki_encrypt_session_key(auth_tok, crypt_stat, key_rec);
> + rc = pki_encrypt_session_key(auth_tok_key, auth_tok, crypt_stat,
> + key_rec);
> if (rc) {
> printk(KERN_ERR "Failed to encrypt session key via a key "
> "module; rc = [%d]\n", rc);
> @@ -2421,6 +2434,8 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> &max, auth_tok,
> crypt_stat, key_rec,
> &written);
> + up_write(&(auth_tok_key->sem));
> + key_put(auth_tok_key);
> if (rc) {
> ecryptfs_printk(KERN_WARNING, "Error "
> "writing tag 3 packet\n");
> @@ -2438,8 +2453,8 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> }
> (*len) += written;
> } else if (auth_tok->token_type == ECRYPTFS_PRIVATE_KEY) {
> - rc = write_tag_1_packet(dest_base + (*len),
> - &max, auth_tok,
> + rc = write_tag_1_packet(dest_base + (*len), &max,
> + auth_tok_key, auth_tok,
> crypt_stat, key_rec, &written);
> if (rc) {
> ecryptfs_printk(KERN_WARNING, "Error "
> @@ -2448,14 +2463,13 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> }
> (*len) += written;
> } else {
> + up_write(&(auth_tok_key->sem));
> + key_put(auth_tok_key);
> ecryptfs_printk(KERN_WARNING, "Unsupported "
> "authentication token type\n");
> rc = -EINVAL;
> goto out_free;
> }
> - up_write(&(auth_tok_key->sem));
> - key_put(auth_tok_key);
> - auth_tok_key = NULL;
> }
> if (likely(max > 0)) {
> dest_base[(*len)] = 0x00;
> @@ -2468,11 +2482,6 @@ out_free:
> out:
> if (rc)
> (*len) = 0;
> - if (auth_tok_key) {
> - up_write(&(auth_tok_key->sem));
> - key_put(auth_tok_key);
> - }
> -
> mutex_unlock(&crypt_stat->keysig_list_mutex);
> return rc;
> }
--
To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
