On Thu Mar 17, 2011 at 12:48:52PM +0100, Roberto Sassu <[email protected]> wrote:
> Authentication tokens content may change if another requestor calls the
> update() method of the corresponding key. The new function
> ecryptfs_verify_auth_tok_from_key() retrieves the authentication token from
> the provided key and verifies if it is still valid before being used to
> encrypt or decrypt an eCryptfs file.
>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> fs/ecryptfs/ecryptfs_kernel.h | 3 +-
> fs/ecryptfs/keystore.c | 107 ++++++++++++++++++++++++++++-------------
> fs/ecryptfs/main.c | 4 +-
> 3 files changed, 77 insertions(+), 37 deletions(-)
>
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index 8282031..85728e9 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -333,7 +333,6 @@ struct ecryptfs_global_auth_tok {
> u32 flags;
> struct list_head mount_crypt_stat_list;
> struct key *global_auth_tok_key;
> - struct ecryptfs_auth_tok *global_auth_tok;
> unsigned char sig[ECRYPTFS_SIG_SIZE_HEX + 1];
> };
>
> @@ -726,6 +725,8 @@ int ecryptfs_get_tfm_and_mutex_for_cipher_name(struct crypto_blkcipher **tfm,
> int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
> struct ecryptfs_auth_tok **auth_tok,
> char *sig);
> +int ecryptfs_verify_auth_tok_from_key(struct key *auth_tok_key,
> + struct ecryptfs_auth_tok **auth_tok);
This shouldn't go in the header file. It only seems to be used in
keystore.c.
> int ecryptfs_write_lower(struct inode *ecryptfs_inode, char *data,
> loff_t offset, size_t size);
> int ecryptfs_write_lower_page_segment(struct inode *ecryptfs_inode,
> diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> index 36b68a6..e35d745 100644
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -405,25 +405,47 @@ out:
>
> static int
> ecryptfs_find_global_auth_tok_for_sig(
> - struct ecryptfs_global_auth_tok **global_auth_tok,
> + struct key **auth_tok_key,
> + struct ecryptfs_auth_tok **auth_tok,
> struct ecryptfs_mount_crypt_stat *mount_crypt_stat, char *sig)
> {
> struct ecryptfs_global_auth_tok *walker;
> int rc = 0;
>
> - (*global_auth_tok) = NULL;
> + (*auth_tok_key) = NULL;
> + (*auth_tok) = NULL;
> mutex_lock(&mount_crypt_stat->global_auth_tok_list_mutex);
> list_for_each_entry(walker,
> &mount_crypt_stat->global_auth_tok_list,
> mount_crypt_stat_list) {
> if (memcmp(walker->sig, sig, ECRYPTFS_SIG_SIZE_HEX) == 0) {
Since we're adding more logic inside this conditional, I'd like to see
something like this:
if (!memcmp(...))
continue;
Then proceed with all the newly added logic.
The eCryptfs source is full of long, too descriptive function and
variable names. The less nesting we do now, the easier it will be to
read later. :)
> + if (walker->flags & ECRYPTFS_AUTH_TOK_INVALID) {
> + rc = -EINVAL;
> + goto out;
> + }
> +
> rc = key_validate(walker->global_auth_tok_key);
> - if (!rc)
> - (*global_auth_tok) = walker;
> + if (rc)
> + goto out;
> +
> + rc = ecryptfs_verify_auth_tok_from_key(
> + walker->global_auth_tok_key, auth_tok);
> + if (rc) {
> + printk(KERN_WARNING
> + "Invalidating auth tok with sig = [%s]\n",
> + sig);
Off by one space on the indenting here.
> + walker->flags |= ECRYPTFS_AUTH_TOK_INVALID;
> + key_put(walker->global_auth_tok_key);
> + walker->global_auth_tok_key = NULL;
> + mount_crypt_stat->num_global_auth_toks--;
It looks like num_global_auth_toks can go away. We increment and
decrement it, but never actually check it.
> + goto out;
> + }
> + (*auth_tok_key) = walker->global_auth_tok_key;
> + key_get(*auth_tok_key);
> goto out;
> }
> }
> - rc = -EINVAL;
> + rc = -ENOENT;
> out:
> mutex_unlock(&mount_crypt_stat->global_auth_tok_list_mutex);
> return rc;
> @@ -451,14 +473,11 @@ ecryptfs_find_auth_tok_for_sig(
> struct ecryptfs_mount_crypt_stat *mount_crypt_stat,
> char *sig)
> {
> - struct ecryptfs_global_auth_tok *global_auth_tok;
> int rc = 0;
>
> - (*auth_tok_key) = NULL;
> - (*auth_tok) = NULL;
> - if (ecryptfs_find_global_auth_tok_for_sig(&global_auth_tok,
> - mount_crypt_stat, sig)) {
> -
> + rc = ecryptfs_find_global_auth_tok_for_sig(auth_tok_key, auth_tok,
> + mount_crypt_stat, sig);
> + if (rc == -ENOENT) {
> /* if the flag ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY is set in the
> * mount_crypt_stat structure, we prevent to use auth toks that
> * are not inserted through the ecryptfs_add_global_auth_tok
> @@ -470,8 +489,8 @@ ecryptfs_find_auth_tok_for_sig(
>
> rc = ecryptfs_keyring_auth_tok_for_sig(auth_tok_key, auth_tok,
> sig);
> - } else
> - (*auth_tok) = global_auth_tok->global_auth_tok;
> + }
> +
> return rc;
> }
>
> @@ -1566,7 +1585,23 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
> (*auth_tok_key) = NULL;
> goto out;
> }
> - (*auth_tok) = ecryptfs_get_key_payload_data(*auth_tok_key);
> +
> + rc = ecryptfs_verify_auth_tok_from_key(*auth_tok_key, auth_tok);
> + if (rc) {
> + key_put(*auth_tok_key);
> + (*auth_tok_key) = NULL;
> + goto out;
> + }
> +out:
> + return rc;
> +}
> +
> +int ecryptfs_verify_auth_tok_from_key(struct key *auth_tok_key,
> + struct ecryptfs_auth_tok **auth_tok)
Can be marked static.
> +{
> + int rc = 0;
> +
> + (*auth_tok) = ecryptfs_get_key_payload_data(auth_tok_key);
> if (ecryptfs_verify_version((*auth_tok)->version)) {
> printk(KERN_ERR
> "Data structure version mismatch. "
> @@ -1576,19 +1611,14 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
> ECRYPTFS_VERSION_MAJOR,
> ECRYPTFS_VERSION_MINOR);
> rc = -EINVAL;
> - goto out_release_key;
> + goto out;
> }
> if ((*auth_tok)->token_type != ECRYPTFS_PASSWORD
> && (*auth_tok)->token_type != ECRYPTFS_PRIVATE_KEY) {
> printk(KERN_ERR "Invalid auth_tok structure "
> "returned from key query\n");
> rc = -EINVAL;
> - goto out_release_key;
> - }
> -out_release_key:
> - if (rc) {
> - key_put(*auth_tok_key);
> - (*auth_tok_key) = NULL;
> + goto out;
> }
> out:
> return rc;
> @@ -2325,13 +2355,14 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> size_t max)
> {
> struct ecryptfs_auth_tok *auth_tok;
> - struct ecryptfs_global_auth_tok *global_auth_tok;
> + struct key *auth_tok_key = NULL;
> struct ecryptfs_mount_crypt_stat *mount_crypt_stat =
> &ecryptfs_superblock_to_private(
> ecryptfs_dentry->d_sb)->mount_crypt_stat;
> size_t written;
> struct ecryptfs_key_record *key_rec;
> struct ecryptfs_key_sig *key_sig;
> + int auth_tok_count = 0;
> int rc = 0;
>
> (*len) = 0;
> @@ -2344,21 +2375,18 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> list_for_each_entry(key_sig, &crypt_stat->keysig_list,
> crypt_stat_list) {
> memset(key_rec, 0, sizeof(*key_rec));
> - rc = ecryptfs_find_global_auth_tok_for_sig(&global_auth_tok,
> + rc = ecryptfs_find_global_auth_tok_for_sig(&auth_tok_key,
> + &auth_tok,
> mount_crypt_stat,
> key_sig->keysig);
> if (rc) {
> - printk(KERN_ERR "Error attempting to get the global "
> - "auth_tok; rc = [%d]\n", rc);
> - goto out_free;
> - }
> - if (global_auth_tok->flags & ECRYPTFS_AUTH_TOK_INVALID) {
> printk(KERN_WARNING
> "Skipping invalid auth tok with sig = [%s]\n",
"invalid auth tok" isn't necessarily accurate here. A global auth tok
with that sig simply may not have been found.
> - global_auth_tok->sig);
> + key_sig->keysig);
> + rc = 0;
> continue;
In my opinion, this is an error condition that can't be ignored. The
user trusts us to wrap the file encryption key of all newly created
files with the key associated with this auth tok. If the auth tok is not
found or is unusable, I don't feel like we should act like everything is
going as planned. What do you think?
> }
> - auth_tok = global_auth_tok->global_auth_tok;
> + auth_tok_count++;
> if (auth_tok->token_type == ECRYPTFS_PASSWORD) {
> rc = write_tag_3_packet((dest_base + (*len)),
> &max, auth_tok,
> @@ -2367,7 +2395,7 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> if (rc) {
> ecryptfs_printk(KERN_WARNING, "Error "
> "writing tag 3 packet\n");
> - goto out_free;
> + goto out_release_key;
> }
> (*len) += written;
> /* Write auth tok signature packet */
> @@ -2377,7 +2405,7 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> if (rc) {
> ecryptfs_printk(KERN_ERR, "Error writing "
> "auth tok signature packet\n");
> - goto out_free;
> + goto out_release_key;
> }
> (*len) += written;
> } else if (auth_tok->token_type == ECRYPTFS_PRIVATE_KEY) {
> @@ -2387,15 +2415,23 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> if (rc) {
> ecryptfs_printk(KERN_WARNING, "Error "
> "writing tag 1 packet\n");
> - goto out_free;
> + goto out_release_key;
> }
> (*len) += written;
> } else {
> ecryptfs_printk(KERN_WARNING, "Unsupported "
> "authentication token type\n");
> rc = -EINVAL;
> - goto out_free;
> + goto out_release_key;
> }
> + key_put(auth_tok_key);
> + }
> + if (!auth_tok_count) {
> + printk(KERN_WARNING
> + "Unable to create a new file without a valid "
> + "authentication token\n");
> + rc = -EINVAL;
> + goto out_free;
> }
> if (likely(max > 0)) {
> dest_base[(*len)] = 0x00;
> @@ -2403,6 +2439,9 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> ecryptfs_printk(KERN_ERR, "Error writing boundary byte\n");
> rc = -EIO;
> }
> +out_release_key:
> + if (rc)
> + key_put(auth_tok_key);
> out_free:
> kmem_cache_free(ecryptfs_key_record_cache, key_rec);
> out:
> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> index 758323a..f079473 100644
> --- a/fs/ecryptfs/main.c
> +++ b/fs/ecryptfs/main.c
> @@ -241,14 +241,14 @@ static int ecryptfs_init_global_auth_toks(
> struct ecryptfs_mount_crypt_stat *mount_crypt_stat)
> {
> struct ecryptfs_global_auth_tok *global_auth_tok;
> + struct ecryptfs_auth_tok *auth_tok;
> int rc = 0;
>
> list_for_each_entry(global_auth_tok,
> &mount_crypt_stat->global_auth_tok_list,
> mount_crypt_stat_list) {
> rc = ecryptfs_keyring_auth_tok_for_sig(
> - &global_auth_tok->global_auth_tok_key,
> - &global_auth_tok->global_auth_tok,
> + &global_auth_tok->global_auth_tok_key, &auth_tok,
> global_auth_tok->sig);
> if (rc) {
> printk(KERN_ERR "Could not find valid key in user "
> --
> 1.7.4
>
On Friday, March 18, 2011 04:05:39 am Tyler Hicks wrote:
> On Thu Mar 17, 2011 at 12:48:52PM +0100, Roberto Sassu <[email protected]> wrote:
> > Authentication tokens content may change if another requestor calls the
> > update() method of the corresponding key. The new function
> > ecryptfs_verify_auth_tok_from_key() retrieves the authentication token from
> > the provided key and verifies if it is still valid before being used to
> > encrypt or decrypt an eCryptfs file.
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
> > ---
> > fs/ecryptfs/ecryptfs_kernel.h | 3 +-
> > fs/ecryptfs/keystore.c | 107 ++++++++++++++++++++++++++++-------------
> > fs/ecryptfs/main.c | 4 +-
> > 3 files changed, 77 insertions(+), 37 deletions(-)
> >
> > diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> > index 8282031..85728e9 100644
> > --- a/fs/ecryptfs/ecryptfs_kernel.h
> > +++ b/fs/ecryptfs/ecryptfs_kernel.h
> > @@ -333,7 +333,6 @@ struct ecryptfs_global_auth_tok {
> > u32 flags;
> > struct list_head mount_crypt_stat_list;
> > struct key *global_auth_tok_key;
> > - struct ecryptfs_auth_tok *global_auth_tok;
> > unsigned char sig[ECRYPTFS_SIG_SIZE_HEX + 1];
> > };
> >
> > @@ -726,6 +725,8 @@ int ecryptfs_get_tfm_and_mutex_for_cipher_name(struct crypto_blkcipher **tfm,
> > int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
> > struct ecryptfs_auth_tok **auth_tok,
> > char *sig);
> > +int ecryptfs_verify_auth_tok_from_key(struct key *auth_tok_key,
> > + struct ecryptfs_auth_tok **auth_tok);
>
> This shouldn't go in the header file. It only seems to be used in
> keystore.c.
>
> > int ecryptfs_write_lower(struct inode *ecryptfs_inode, char *data,
> > loff_t offset, size_t size);
> > int ecryptfs_write_lower_page_segment(struct inode *ecryptfs_inode,
> > diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> > index 36b68a6..e35d745 100644
> > --- a/fs/ecryptfs/keystore.c
> > +++ b/fs/ecryptfs/keystore.c
> > @@ -405,25 +405,47 @@ out:
> >
> > static int
> > ecryptfs_find_global_auth_tok_for_sig(
> > - struct ecryptfs_global_auth_tok **global_auth_tok,
> > + struct key **auth_tok_key,
> > + struct ecryptfs_auth_tok **auth_tok,
> > struct ecryptfs_mount_crypt_stat *mount_crypt_stat, char *sig)
> > {
> > struct ecryptfs_global_auth_tok *walker;
> > int rc = 0;
> >
> > - (*global_auth_tok) = NULL;
> > + (*auth_tok_key) = NULL;
> > + (*auth_tok) = NULL;
> > mutex_lock(&mount_crypt_stat->global_auth_tok_list_mutex);
> > list_for_each_entry(walker,
> > &mount_crypt_stat->global_auth_tok_list,
> > mount_crypt_stat_list) {
> > if (memcmp(walker->sig, sig, ECRYPTFS_SIG_SIZE_HEX) == 0) {
>
> Since we're adding more logic inside this conditional, I'd like to see
> something like this:
>
> if (!memcmp(...))
> continue;
>
> Then proceed with all the newly added logic.
>
> The eCryptfs source is full of long, too descriptive function and
> variable names. The less nesting we do now, the easier it will be to
> read later. :)
>
> > + if (walker->flags & ECRYPTFS_AUTH_TOK_INVALID) {
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > +
> > rc = key_validate(walker->global_auth_tok_key);
> > - if (!rc)
> > - (*global_auth_tok) = walker;
> > + if (rc)
> > + goto out;
> > +
> > + rc = ecryptfs_verify_auth_tok_from_key(
> > + walker->global_auth_tok_key, auth_tok);
> > + if (rc) {
> > + printk(KERN_WARNING
> > + "Invalidating auth tok with sig = [%s]\n",
> > + sig);
>
> Off by one space on the indenting here.
>
> > + walker->flags |= ECRYPTFS_AUTH_TOK_INVALID;
> > + key_put(walker->global_auth_tok_key);
> > + walker->global_auth_tok_key = NULL;
> > + mount_crypt_stat->num_global_auth_toks--;
>
> It looks like num_global_auth_toks can go away. We increment and
> decrement it, but never actually check it.
>
> > + goto out;
> > + }
> > + (*auth_tok_key) = walker->global_auth_tok_key;
> > + key_get(*auth_tok_key);
> > goto out;
> > }
> > }
> > - rc = -EINVAL;
> > + rc = -ENOENT;
> > out:
> > mutex_unlock(&mount_crypt_stat->global_auth_tok_list_mutex);
> > return rc;
> > @@ -451,14 +473,11 @@ ecryptfs_find_auth_tok_for_sig(
> > struct ecryptfs_mount_crypt_stat *mount_crypt_stat,
> > char *sig)
> > {
> > - struct ecryptfs_global_auth_tok *global_auth_tok;
> > int rc = 0;
> >
> > - (*auth_tok_key) = NULL;
> > - (*auth_tok) = NULL;
> > - if (ecryptfs_find_global_auth_tok_for_sig(&global_auth_tok,
> > - mount_crypt_stat, sig)) {
> > -
> > + rc = ecryptfs_find_global_auth_tok_for_sig(auth_tok_key, auth_tok,
> > + mount_crypt_stat, sig);
> > + if (rc == -ENOENT) {
> > /* if the flag ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY is set in the
> > * mount_crypt_stat structure, we prevent to use auth toks that
> > * are not inserted through the ecryptfs_add_global_auth_tok
> > @@ -470,8 +489,8 @@ ecryptfs_find_auth_tok_for_sig(
> >
> > rc = ecryptfs_keyring_auth_tok_for_sig(auth_tok_key, auth_tok,
> > sig);
> > - } else
> > - (*auth_tok) = global_auth_tok->global_auth_tok;
> > + }
> > +
> > return rc;
> > }
> >
> > @@ -1566,7 +1585,23 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
> > (*auth_tok_key) = NULL;
> > goto out;
> > }
> > - (*auth_tok) = ecryptfs_get_key_payload_data(*auth_tok_key);
> > +
> > + rc = ecryptfs_verify_auth_tok_from_key(*auth_tok_key, auth_tok);
> > + if (rc) {
> > + key_put(*auth_tok_key);
> > + (*auth_tok_key) = NULL;
> > + goto out;
> > + }
> > +out:
> > + return rc;
> > +}
> > +
> > +int ecryptfs_verify_auth_tok_from_key(struct key *auth_tok_key,
> > + struct ecryptfs_auth_tok **auth_tok)
>
> Can be marked static.
>
> > +{
> > + int rc = 0;
> > +
> > + (*auth_tok) = ecryptfs_get_key_payload_data(auth_tok_key);
> > if (ecryptfs_verify_version((*auth_tok)->version)) {
> > printk(KERN_ERR
> > "Data structure version mismatch. "
> > @@ -1576,19 +1611,14 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
> > ECRYPTFS_VERSION_MAJOR,
> > ECRYPTFS_VERSION_MINOR);
> > rc = -EINVAL;
> > - goto out_release_key;
> > + goto out;
> > }
> > if ((*auth_tok)->token_type != ECRYPTFS_PASSWORD
> > && (*auth_tok)->token_type != ECRYPTFS_PRIVATE_KEY) {
> > printk(KERN_ERR "Invalid auth_tok structure "
> > "returned from key query\n");
> > rc = -EINVAL;
> > - goto out_release_key;
> > - }
> > -out_release_key:
> > - if (rc) {
> > - key_put(*auth_tok_key);
> > - (*auth_tok_key) = NULL;
> > + goto out;
> > }
> > out:
> > return rc;
> > @@ -2325,13 +2355,14 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> > size_t max)
> > {
> > struct ecryptfs_auth_tok *auth_tok;
> > - struct ecryptfs_global_auth_tok *global_auth_tok;
> > + struct key *auth_tok_key = NULL;
> > struct ecryptfs_mount_crypt_stat *mount_crypt_stat =
> > &ecryptfs_superblock_to_private(
> > ecryptfs_dentry->d_sb)->mount_crypt_stat;
> > size_t written;
> > struct ecryptfs_key_record *key_rec;
> > struct ecryptfs_key_sig *key_sig;
> > + int auth_tok_count = 0;
> > int rc = 0;
> >
> > (*len) = 0;
> > @@ -2344,21 +2375,18 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> > list_for_each_entry(key_sig, &crypt_stat->keysig_list,
> > crypt_stat_list) {
> > memset(key_rec, 0, sizeof(*key_rec));
> > - rc = ecryptfs_find_global_auth_tok_for_sig(&global_auth_tok,
> > + rc = ecryptfs_find_global_auth_tok_for_sig(&auth_tok_key,
> > + &auth_tok,
> > mount_crypt_stat,
> > key_sig->keysig);
> > if (rc) {
> > - printk(KERN_ERR "Error attempting to get the global "
> > - "auth_tok; rc = [%d]\n", rc);
> > - goto out_free;
> > - }
> > - if (global_auth_tok->flags & ECRYPTFS_AUTH_TOK_INVALID) {
> > printk(KERN_WARNING
> > "Skipping invalid auth tok with sig = [%s]\n",
>
> "invalid auth tok" isn't necessarily accurate here. A global auth tok
> with that sig simply may not have been found.
>
> > - global_auth_tok->sig);
> > + key_sig->keysig);
> > + rc = 0;
> > continue;
>
> In my opinion, this is an error condition that can't be ignored. The
> user trusts us to wrap the file encryption key of all newly created
> files with the key associated with this auth tok. If the auth tok is not
> found or is unusable, I don't feel like we should act like everything is
> going as planned. What do you think?
>
Hi Tyler
yes, returning an error probably is the best choice, as this is a
way to say a new file cannot be created as expected.
Roberto
> > }
> > - auth_tok = global_auth_tok->global_auth_tok;
> > + auth_tok_count++;
> > if (auth_tok->token_type == ECRYPTFS_PASSWORD) {
> > rc = write_tag_3_packet((dest_base + (*len)),
> > &max, auth_tok,
> > @@ -2367,7 +2395,7 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> > if (rc) {
> > ecryptfs_printk(KERN_WARNING, "Error "
> > "writing tag 3 packet\n");
> > - goto out_free;
> > + goto out_release_key;
> > }
> > (*len) += written;
> > /* Write auth tok signature packet */
> > @@ -2377,7 +2405,7 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> > if (rc) {
> > ecryptfs_printk(KERN_ERR, "Error writing "
> > "auth tok signature packet\n");
> > - goto out_free;
> > + goto out_release_key;
> > }
> > (*len) += written;
> > } else if (auth_tok->token_type == ECRYPTFS_PRIVATE_KEY) {
> > @@ -2387,15 +2415,23 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> > if (rc) {
> > ecryptfs_printk(KERN_WARNING, "Error "
> > "writing tag 1 packet\n");
> > - goto out_free;
> > + goto out_release_key;
> > }
> > (*len) += written;
> > } else {
> > ecryptfs_printk(KERN_WARNING, "Unsupported "
> > "authentication token type\n");
> > rc = -EINVAL;
> > - goto out_free;
> > + goto out_release_key;
> > }
> > + key_put(auth_tok_key);
> > + }
> > + if (!auth_tok_count) {
> > + printk(KERN_WARNING
> > + "Unable to create a new file without a valid "
> > + "authentication token\n");
> > + rc = -EINVAL;
> > + goto out_free;
> > }
> > if (likely(max > 0)) {
> > dest_base[(*len)] = 0x00;
> > @@ -2403,6 +2439,9 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> > ecryptfs_printk(KERN_ERR, "Error writing boundary byte\n");
> > rc = -EIO;
> > }
> > +out_release_key:
> > + if (rc)
> > + key_put(auth_tok_key);
> > out_free:
> > kmem_cache_free(ecryptfs_key_record_cache, key_rec);
> > out:
> > diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> > index 758323a..f079473 100644
> > --- a/fs/ecryptfs/main.c
> > +++ b/fs/ecryptfs/main.c
> > @@ -241,14 +241,14 @@ static int ecryptfs_init_global_auth_toks(
> > struct ecryptfs_mount_crypt_stat *mount_crypt_stat)
> > {
> > struct ecryptfs_global_auth_tok *global_auth_tok;
> > + struct ecryptfs_auth_tok *auth_tok;
> > int rc = 0;
> >
> > list_for_each_entry(global_auth_tok,
> > &mount_crypt_stat->global_auth_tok_list,
> > mount_crypt_stat_list) {
> > rc = ecryptfs_keyring_auth_tok_for_sig(
> > - &global_auth_tok->global_auth_tok_key,
> > - &global_auth_tok->global_auth_tok,
> > + &global_auth_tok->global_auth_tok_key, &auth_tok,
> > global_auth_tok->sig);
> > if (rc) {
> > printk(KERN_ERR "Could not find valid key in user "
>
>
>
On Thu Mar 17, 2011 at 12:48:53PM +0100, Roberto Sassu <[email protected]> wrote:
> The ecryptfs_find_auth_tok_for_sig() call is moved before the
> mutex_lock(s->tfm_mutex) instruction in order to avoid possible deadlocks
> that may occur by holding the lock on the two semaphores 'key->sem' and
> 's->tfm_mutex' in reverse order.
>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> fs/ecryptfs/keystore.c | 42 +++++++++++++++++++++++-------------------
> 1 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> index e35d745..d066217 100644
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -538,6 +538,7 @@ ecryptfs_write_tag_70_packet(char *dest, size_t *remaining_bytes,
> char *filename, size_t filename_size)
> {
> struct ecryptfs_write_tag_70_packet_silly_stack *s;
> + struct ecryptfs_auth_tok *auth_tok;
Why declare a new ecryptfs_auth_tok struct pointer here? The idea behind
the ecryptfs_write_tag_70_packet_silly_stack struct is to keep stack
size at a minimum. Since it already has s->auth_tok, I don't see why we
would need to declare a new one.
> struct key *auth_tok_key = NULL;
> int rc = 0;
>
> @@ -550,6 +551,16 @@ ecryptfs_write_tag_70_packet(char *dest, size_t *remaining_bytes,
> }
> s->desc.flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> (*packet_size) = 0;
> + rc = ecryptfs_find_auth_tok_for_sig(
> + &auth_tok_key,
> + &auth_tok, mount_crypt_stat,
> + mount_crypt_stat->global_default_fnek_sig);
> + if (rc) {
> + printk(KERN_ERR "%s: Error attempting to find auth tok for "
> + "fnek sig [%s]; rc = [%d]\n", __func__,
> + mount_crypt_stat->global_default_fnek_sig, rc);
> + goto out;
> + }
> rc = ecryptfs_get_tfm_and_mutex_for_cipher_name(
> &s->desc.tfm,
> &s->tfm_mutex, mount_crypt_stat->global_default_fn_cipher_name);
> @@ -635,16 +646,7 @@ ecryptfs_write_tag_70_packet(char *dest, size_t *remaining_bytes,
> goto out_free_unlock;
> }
> dest[s->i++] = s->cipher_code;
> - rc = ecryptfs_find_auth_tok_for_sig(
> - &auth_tok_key,
> - &s->auth_tok, mount_crypt_stat,
> - mount_crypt_stat->global_default_fnek_sig);
> - if (rc) {
> - printk(KERN_ERR "%s: Error attempting to find auth tok for "
> - "fnek sig [%s]; rc = [%d]\n", __func__,
> - mount_crypt_stat->global_default_fnek_sig, rc);
> - goto out_free_unlock;
> - }
> + s->auth_tok = auth_tok;
> /* TODO: Support other key modules than passphrase for
> * filename encryption */
> if (s->auth_tok->token_type != ECRYPTFS_PASSWORD) {
> @@ -831,6 +833,7 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
> char *data, size_t max_packet_size)
> {
> struct ecryptfs_parse_tag_70_packet_silly_stack *s;
> + struct ecryptfs_auth_tok *auth_tok;
Same here as above.
Tyler
> struct key *auth_tok_key = NULL;
> int rc = 0;
>
> @@ -898,6 +901,15 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
> __func__, s->cipher_code);
> goto out;
> }
> + rc = ecryptfs_find_auth_tok_for_sig(&auth_tok_key,
> + &auth_tok, mount_crypt_stat,
> + s->fnek_sig_hex);
> + if (rc) {
> + printk(KERN_ERR "%s: Error attempting to find auth tok for "
> + "fnek sig [%s]; rc = [%d]\n", __func__, s->fnek_sig_hex,
> + rc);
> + goto out;
> + }
> rc = ecryptfs_get_tfm_and_mutex_for_cipher_name(&s->desc.tfm,
> &s->tfm_mutex,
> s->cipher_string);
> @@ -944,15 +956,7 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
> * >= ECRYPTFS_MAX_IV_BYTES. */
> memset(s->iv, 0, ECRYPTFS_MAX_IV_BYTES);
> s->desc.info = s->iv;
> - rc = ecryptfs_find_auth_tok_for_sig(&auth_tok_key,
> - &s->auth_tok, mount_crypt_stat,
> - s->fnek_sig_hex);
> - if (rc) {
> - printk(KERN_ERR "%s: Error attempting to find auth tok for "
> - "fnek sig [%s]; rc = [%d]\n", __func__, s->fnek_sig_hex,
> - rc);
> - goto out_free_unlock;
> - }
> + s->auth_tok = auth_tok;
> /* TODO: Support other key modules than passphrase for
> * filename encryption */
> if (s->auth_tok->token_type != ECRYPTFS_PASSWORD) {
> --
> 1.7.4
>
On Friday, March 18, 2011 04:30:50 pm Tyler Hicks wrote:
> On Thu Mar 17, 2011 at 12:48:53PM +0100, Roberto Sassu <[email protected]> wrote:
> > The ecryptfs_find_auth_tok_for_sig() call is moved before the
> > mutex_lock(s->tfm_mutex) instruction in order to avoid possible deadlocks
> > that may occur by holding the lock on the two semaphores 'key->sem' and
> > 's->tfm_mutex' in reverse order.
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
> > ---
> > fs/ecryptfs/keystore.c | 42 +++++++++++++++++++++++-------------------
> > 1 files changed, 23 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> > index e35d745..d066217 100644
> > --- a/fs/ecryptfs/keystore.c
> > +++ b/fs/ecryptfs/keystore.c
> > @@ -538,6 +538,7 @@ ecryptfs_write_tag_70_packet(char *dest, size_t *remaining_bytes,
> > char *filename, size_t filename_size)
> > {
> > struct ecryptfs_write_tag_70_packet_silly_stack *s;
> > + struct ecryptfs_auth_tok *auth_tok;
>
> Why declare a new ecryptfs_auth_tok struct pointer here? The idea behind
> the ecryptfs_write_tag_70_packet_silly_stack struct is to keep stack
> size at a minimum. Since it already has s->auth_tok, I don't see why we
> would need to declare a new one.
>
Hi Tyler
i did this because i was thinking that 'tfm_mutex' is protecting
the 'auth_tok' field of the ecryptfs_parse_tag_70_packet_silly_stack
structure. Sorry, my mistake.
Thanks
Roberto
> > struct key *auth_tok_key = NULL;
> > int rc = 0;
> >
> > @@ -550,6 +551,16 @@ ecryptfs_write_tag_70_packet(char *dest, size_t *remaining_bytes,
> > }
> > s->desc.flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> > (*packet_size) = 0;
> > + rc = ecryptfs_find_auth_tok_for_sig(
> > + &auth_tok_key,
> > + &auth_tok, mount_crypt_stat,
> > + mount_crypt_stat->global_default_fnek_sig);
> > + if (rc) {
> > + printk(KERN_ERR "%s: Error attempting to find auth tok for "
> > + "fnek sig [%s]; rc = [%d]\n", __func__,
> > + mount_crypt_stat->global_default_fnek_sig, rc);
> > + goto out;
> > + }
> > rc = ecryptfs_get_tfm_and_mutex_for_cipher_name(
> > &s->desc.tfm,
> > &s->tfm_mutex, mount_crypt_stat->global_default_fn_cipher_name);
> > @@ -635,16 +646,7 @@ ecryptfs_write_tag_70_packet(char *dest, size_t *remaining_bytes,
> > goto out_free_unlock;
> > }
> > dest[s->i++] = s->cipher_code;
> > - rc = ecryptfs_find_auth_tok_for_sig(
> > - &auth_tok_key,
> > - &s->auth_tok, mount_crypt_stat,
> > - mount_crypt_stat->global_default_fnek_sig);
> > - if (rc) {
> > - printk(KERN_ERR "%s: Error attempting to find auth tok for "
> > - "fnek sig [%s]; rc = [%d]\n", __func__,
> > - mount_crypt_stat->global_default_fnek_sig, rc);
> > - goto out_free_unlock;
> > - }
> > + s->auth_tok = auth_tok;
> > /* TODO: Support other key modules than passphrase for
> > * filename encryption */
> > if (s->auth_tok->token_type != ECRYPTFS_PASSWORD) {
> > @@ -831,6 +833,7 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
> > char *data, size_t max_packet_size)
> > {
> > struct ecryptfs_parse_tag_70_packet_silly_stack *s;
> > + struct ecryptfs_auth_tok *auth_tok;
>
> Same here as above.
>
> Tyler
>
> > struct key *auth_tok_key = NULL;
> > int rc = 0;
> >
> > @@ -898,6 +901,15 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
> > __func__, s->cipher_code);
> > goto out;
> > }
> > + rc = ecryptfs_find_auth_tok_for_sig(&auth_tok_key,
> > + &auth_tok, mount_crypt_stat,
> > + s->fnek_sig_hex);
> > + if (rc) {
> > + printk(KERN_ERR "%s: Error attempting to find auth tok for "
> > + "fnek sig [%s]; rc = [%d]\n", __func__, s->fnek_sig_hex,
> > + rc);
> > + goto out;
> > + }
> > rc = ecryptfs_get_tfm_and_mutex_for_cipher_name(&s->desc.tfm,
> > &s->tfm_mutex,
> > s->cipher_string);
> > @@ -944,15 +956,7 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
> > * >= ECRYPTFS_MAX_IV_BYTES. */
> > memset(s->iv, 0, ECRYPTFS_MAX_IV_BYTES);
> > s->desc.info = s->iv;
> > - rc = ecryptfs_find_auth_tok_for_sig(&auth_tok_key,
> > - &s->auth_tok, mount_crypt_stat,
> > - s->fnek_sig_hex);
> > - if (rc) {
> > - printk(KERN_ERR "%s: Error attempting to find auth tok for "
> > - "fnek sig [%s]; rc = [%d]\n", __func__, s->fnek_sig_hex,
> > - rc);
> > - goto out_free_unlock;
> > - }
> > + s->auth_tok = auth_tok;
> > /* TODO: Support other key modules than passphrase for
> > * filename encryption */
> > if (s->auth_tok->token_type != ECRYPTFS_PASSWORD) {
>
>
>
On Thu Mar 17, 2011 at 12:48:51PM +0100, Roberto Sassu <[email protected]> wrote:
> The size of the 'keysig' array is incremented of one byte in order to make
> room for the NULL character.
Hello Roberto - I'm assuming that this change is because you want to
print the keysig in patch 3/5. I don't see where we are currently trying
to print the keysig, so I'll wait until you resubmit this patch set to
merge this one.
Along with what a patch does, please try to explain why it is useful in
the commit message.
Tyler
>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> fs/ecryptfs/ecryptfs_kernel.h | 2 +-
> fs/ecryptfs/keystore.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index e007534..8282031 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -233,7 +233,7 @@ ecryptfs_get_key_payload_data(struct key *key)
>
> struct ecryptfs_key_sig {
> struct list_head crypt_stat_list;
> - char keysig[ECRYPTFS_SIG_SIZE_HEX];
> + char keysig[ECRYPTFS_SIG_SIZE_HEX + 1];
> };
>
> struct ecryptfs_filename {
> diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> index 4feb78c..36b68a6 100644
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -2425,6 +2425,7 @@ int ecryptfs_add_keysig(struct ecryptfs_crypt_stat *crypt_stat, char *sig)
> return -ENOMEM;
> }
> memcpy(new_key_sig->keysig, sig, ECRYPTFS_SIG_SIZE_HEX);
> + new_key_sig->keysig[ECRYPTFS_SIG_SIZE_HEX] = '\0';
> /* Caller must hold keysig_list_mutex */
> list_add(&new_key_sig->crypt_stat_list, &crypt_stat->keysig_list);
>
> --
> 1.7.4
>
On Thu Mar 17, 2011 at 12:48:50PM +0100, Roberto Sassu <[email protected]> wrote:
> The pointer '(*auth_tok_key)' is set to NULL in case request_key() fails,
> in order to prevent its use by functions calling
> ecryptfs_keyring_auth_tok_for_sig().
Thanks Roberto - I'm going to go ahead and merge this one into my #next
tree. Feel free to drop it from this patch set when you resend.
Tyler
>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> fs/ecryptfs/keystore.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> index c1436cf..4feb78c 100644
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -1563,6 +1563,7 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
> printk(KERN_ERR "Could not find key with description: [%s]\n",
> sig);
> rc = process_request_key_err(PTR_ERR(*auth_tok_key));
> + (*auth_tok_key) = NULL;
> goto out;
> }
> (*auth_tok) = ecryptfs_get_key_payload_data(*auth_tok_key);
> --
> 1.7.4
>