On Sep 14, 2016, at 2:57 PM, Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> This makes the return value match the comment. Previously it would
> actually return 0 if encryption was successful. No callers currently
> care, but this change should reduce the chance of future bugs.
This may be introducing a subtle bug in fscrypt_fname_usr_to_disk(), since
that function returns the status from fname_encrypt() directly and now it
returns the name length instead of 0 on success:
int fscrypt_fname_usr_to_disk(struct inode *inode,
const struct qstr *iname,
struct fscrypt_str *oname)
{
if (fscrypt_is_dot_dotdot(iname)) {
oname->name[0] = '.';
oname->name[iname->len - 1] = '.';
oname->len = iname->len;
return oname->len;
}
if (inode->i_crypt_info)
return fname_encrypt(inode, iname, oname);
/*
* Without a proper key, a user is not allowed to modify the filenames
* in a directory. Consequently, a user space name cannot be mapped to
* a disk-space name
*/
return -EACCES;
}
This percolates further up to some of the callers, but in the cases that I
saw the check is "if (err < 0)" and the positive value is either ignored
or overwritten before being returned further up the call chain. However,
that could be easily missed in the future and somewhere up the call chain
doing "if (rc)" would suddenly start to fail.
Since both "struct fscrypt_str" and "struct qstr" already hold the length
I don't think there is any benefit to returning the length to the caller.
Since (IMHO) this creates a non-trivial chance of introducing bugs in the
future it makes more sense to just change the function comment to match the
actual behaviour.
Cheers, Andreas
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> ---
> fs/crypto/fname.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 3108806..7f3239c 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -103,12 +103,13 @@ static int fname_encrypt(struct inode *inode,
> }
> kfree(alloc_buf);
> skcipher_request_free(req);
> - if (res < 0)
> + if (res < 0) {
> printk_ratelimited(KERN_ERR
> "%s: Error (error code %d)\n", __func__, res);
> -
> + return res;
> + }
> oname->len = ciphertext_len;
> - return res;
> + return ciphertext_len;
> }
>
> /*
> --
> 2.8.0.rc3.226.g39d4020
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
