Several filename crypto functions: fname_decrypt(),
fscrypt_fname_disk_to_usr(), and fscrypt_fname_usr_to_disk(), returned
the output length on success or -errno on failure. However, the output
length was redundant with the value written to 'oname->len'. It is also
potentially error-prone to make callers have to check for '< 0' instead
of '!= 0'.
Therefore, make these functions return 0 instead of a length, and make
the callers who cared about the return value being a length use
'oname->len' instead. For consistency also make other callers check for
a nonzero result rather than a negative result.
This change also fixes the inconsistency of fname_encrypt() actually
already returning 0 on success, not a length like the other filename
crypto functions and as documented in its function comment.
Signed-off-by: Eric Biggers <[email protected]>
---
fs/crypto/fname.c | 56 +++++++++++++++++++++++++++++++------------------------
fs/ext4/dir.c | 5 +++--
fs/ext4/namei.c | 8 ++++----
fs/ext4/symlink.c | 5 ++---
fs/f2fs/dir.c | 6 +++---
fs/f2fs/namei.c | 6 +++---
6 files changed, 47 insertions(+), 39 deletions(-)
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 6bbc3b1..90697c7 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -35,11 +35,11 @@ static void fname_crypt_complete(struct crypto_async_request *req, int res)
}
/**
- * fname_encrypt() -
+ * fname_encrypt() - encrypt a filename
*
- * This function encrypts the input filename, and returns the length of the
- * ciphertext. Errors are returned as negative numbers. We trust the caller to
- * allocate sufficient memory to oname string.
+ * The caller must have allocated sufficient memory for the @oname string.
+ *
+ * Return: 0 on success, -errno on failure
*/
static int fname_encrypt(struct inode *inode,
const struct qstr *iname, struct fscrypt_str *oname)
@@ -105,20 +105,22 @@ 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 0;
}
-/*
- * fname_decrypt()
- * This function decrypts the input filename, and returns
- * the length of the plaintext.
- * Errors are returned as negative numbers.
- * We trust the caller to allocate sufficient memory to oname string.
+/**
+ * fname_decrypt() - decrypt a filename
+ *
+ * The caller must have allocated sufficient memory for the @oname string.
+ *
+ * Return: 0 on success, -errno on failure
*/
static int fname_decrypt(struct inode *inode,
const struct fscrypt_str *iname,
@@ -168,7 +170,7 @@ static int fname_decrypt(struct inode *inode,
}
oname->len = strnlen(oname->name, iname->len);
- return oname->len;
+ return 0;
}
static const char *lookup_table =
@@ -279,6 +281,10 @@ EXPORT_SYMBOL(fscrypt_fname_free_buffer);
/**
* fscrypt_fname_disk_to_usr() - converts a filename from disk space to user
* space
+ *
+ * The caller must have allocated sufficient memory for the @oname string.
+ *
+ * Return: 0 on success, -errno on failure
*/
int fscrypt_fname_disk_to_usr(struct inode *inode,
u32 hash, u32 minor_hash,
@@ -287,13 +293,12 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
{
const struct qstr qname = FSTR_TO_QSTR(iname);
char buf[24];
- int ret;
if (fscrypt_is_dot_dotdot(&qname)) {
oname->name[0] = '.';
oname->name[iname->len - 1] = '.';
oname->len = iname->len;
- return oname->len;
+ return 0;
}
if (iname->len < FS_CRYPTO_BLOCK_SIZE)
@@ -303,9 +308,9 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
return fname_decrypt(inode, iname, oname);
if (iname->len <= FS_FNAME_CRYPTO_DIGEST_SIZE) {
- ret = digest_encode(iname->name, iname->len, oname->name);
- oname->len = ret;
- return ret;
+ oname->len = digest_encode(iname->name, iname->len,
+ oname->name);
+ return 0;
}
if (hash) {
memcpy(buf, &hash, 4);
@@ -315,15 +320,18 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
}
memcpy(buf + 8, iname->name + iname->len - 16, 16);
oname->name[0] = '_';
- ret = digest_encode(buf, 24, oname->name + 1);
- oname->len = ret + 1;
- return ret + 1;
+ oname->len = 1 + digest_encode(buf, 24, oname->name + 1);
+ return 0;
}
EXPORT_SYMBOL(fscrypt_fname_disk_to_usr);
/**
* fscrypt_fname_usr_to_disk() - converts a filename from user space to disk
* space
+ *
+ * The caller must have allocated sufficient memory for the @oname string.
+ *
+ * Return: 0 on success, -errno on failure
*/
int fscrypt_fname_usr_to_disk(struct inode *inode,
const struct qstr *iname,
@@ -333,7 +341,7 @@ int fscrypt_fname_usr_to_disk(struct inode *inode,
oname->name[0] = '.';
oname->name[iname->len - 1] = '.';
oname->len = iname->len;
- return oname->len;
+ return 0;
}
if (inode->i_crypt_info)
return fname_encrypt(inode, iname, oname);
@@ -367,10 +375,10 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
if (dir->i_crypt_info) {
ret = fscrypt_fname_alloc_buffer(dir, iname->len,
&fname->crypto_buf);
- if (ret < 0)
+ if (ret)
return ret;
ret = fname_encrypt(dir, iname, &fname->crypto_buf);
- if (ret < 0)
+ if (ret)
goto errout;
fname->disk_name.name = fname->crypto_buf.name;
fname->disk_name.len = fname->crypto_buf.len;
diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index 67415e0..4d4b910 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -260,11 +260,12 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
/* Directory is encrypted */
err = fscrypt_fname_disk_to_usr(inode,
0, 0, &de_name, &fstr);
+ de_name = fstr;
fstr.len = save_len;
- if (err < 0)
+ if (err)
goto errout;
if (!dir_emit(ctx,
- fstr.name, err,
+ de_name.name, de_name.len,
le32_to_cpu(de->inode),
get_dtype(sb, de->file_type)))
goto done;
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 34c0142..2243ae2 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -639,7 +639,7 @@ static struct stats dx_show_leaf(struct inode *dir,
res = fscrypt_fname_alloc_buffer(
dir, len,
&fname_crypto_str);
- if (res < 0)
+ if (res)
printk(KERN_WARNING "Error "
"allocating crypto "
"buffer--skipping "
@@ -647,7 +647,7 @@ static struct stats dx_show_leaf(struct inode *dir,
res = fscrypt_fname_disk_to_usr(dir,
0, 0, &de_name,
&fname_crypto_str);
- if (res < 0) {
+ if (res) {
printk(KERN_WARNING "Error "
"converting filename "
"from disk to usr"
@@ -1011,7 +1011,7 @@ static int htree_dirblock_to_tree(struct file *dir_file,
err = fscrypt_fname_disk_to_usr(dir, hinfo->hash,
hinfo->minor_hash, &de_name,
&fname_crypto_str);
- if (err < 0) {
+ if (err) {
count = err;
goto errout;
}
@@ -3144,7 +3144,7 @@ static int ext4_symlink(struct inode *dir,
istr.name = (const unsigned char *) symname;
istr.len = len;
err = fscrypt_fname_usr_to_disk(inode, &istr, &ostr);
- if (err < 0)
+ if (err)
goto err_drop_inode;
sd->len = cpu_to_le16(ostr.len);
disk_link.name = (char *) sd;
diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
index 4d83d9e..55aaf30 100644
--- a/fs/ext4/symlink.c
+++ b/fs/ext4/symlink.c
@@ -67,14 +67,13 @@ static const char *ext4_encrypted_get_link(struct dentry *dentry,
goto errout;
res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
- if (res < 0)
+ if (res)
goto errout;
paddr = pstr.name;
/* Null-terminate the name */
- if (res <= pstr.len)
- paddr[res] = '\0';
+ paddr[pstr.len] = '\0';
if (cpage)
put_page(cpage);
set_delayed_call(done, kfree_link, paddr);
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 9054aea..8716943 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -786,7 +786,7 @@ bool f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
if (f2fs_encrypted_inode(d->inode)) {
int save_len = fstr->len;
- int ret;
+ int err;
de_name.name = f2fs_kmalloc(de_name.len, GFP_NOFS);
if (!de_name.name)
@@ -794,11 +794,11 @@ bool f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
memcpy(de_name.name, d->filename[bit_pos], de_name.len);
- ret = fscrypt_fname_disk_to_usr(d->inode,
+ err = fscrypt_fname_disk_to_usr(d->inode,
(u32)de->hash_code, 0,
&de_name, fstr);
kfree(de_name.name);
- if (ret < 0)
+ if (err)
return true;
de_name = *fstr;
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 73fa356..afd5633 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -449,7 +449,7 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
ostr.name = sd->encrypted_path;
ostr.len = disk_link.len;
err = fscrypt_fname_usr_to_disk(inode, &istr, &ostr);
- if (err < 0)
+ if (err)
goto err_out;
sd->len = cpu_to_le16(ostr.len);
@@ -1048,7 +1048,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
goto errout;
res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
- if (res < 0)
+ if (res)
goto errout;
/* this is broken symlink case */
@@ -1060,7 +1060,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
paddr = pstr.name;
/* Null-terminate the name */
- paddr[res] = '\0';
+ paddr[pstr.len] = '\0';
put_page(cpage);
set_delayed_call(done, kfree_link, paddr);
--
2.8.0.rc3.226.g39d4020
> On Sep 15, 2016, at 11:28 AM, Eric Biggers <[email protected]> wrote:
>
> Several filename crypto functions: fname_decrypt(),
> fscrypt_fname_disk_to_usr(), and fscrypt_fname_usr_to_disk(), returned
> the output length on success or -errno on failure. However, the output
> length was redundant with the value written to 'oname->len'. It is also
> potentially error-prone to make callers have to check for '< 0' instead
> of '!= 0'.
>
> Therefore, make these functions return 0 instead of a length, and make
> the callers who cared about the return value being a length use
> 'oname->len' instead. For consistency also make other callers check for
> a nonzero result rather than a negative result.
>
> This change also fixes the inconsistency of fname_encrypt() actually
> already returning 0 on success, not a length like the other filename
> crypto functions and as documented in its function comment.
>
> Signed-off-by: Eric Biggers <[email protected]>
Reviewed-by: Andreas Dilger <[email protected]>
> ---
> fs/crypto/fname.c | 56 +++++++++++++++++++++++++++++++------------------------
> fs/ext4/dir.c | 5 +++--
> fs/ext4/namei.c | 8 ++++----
> fs/ext4/symlink.c | 5 ++---
> fs/f2fs/dir.c | 6 +++---
> fs/f2fs/namei.c | 6 +++---
> 6 files changed, 47 insertions(+), 39 deletions(-)
>
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 6bbc3b1..90697c7 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -35,11 +35,11 @@ static void fname_crypt_complete(struct crypto_async_request *req, int res)
> }
>
> /**
> - * fname_encrypt() -
> + * fname_encrypt() - encrypt a filename
> *
> - * This function encrypts the input filename, and returns the length of the
> - * ciphertext. Errors are returned as negative numbers. We trust the caller to
> - * allocate sufficient memory to oname string.
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
> */
> static int fname_encrypt(struct inode *inode,
> const struct qstr *iname, struct fscrypt_str *oname)
> @@ -105,20 +105,22 @@ 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 0;
> }
>
> -/*
> - * fname_decrypt()
> - * This function decrypts the input filename, and returns
> - * the length of the plaintext.
> - * Errors are returned as negative numbers.
> - * We trust the caller to allocate sufficient memory to oname string.
> +/**
> + * fname_decrypt() - decrypt a filename
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
> */
> static int fname_decrypt(struct inode *inode,
> const struct fscrypt_str *iname,
> @@ -168,7 +170,7 @@ static int fname_decrypt(struct inode *inode,
> }
>
> oname->len = strnlen(oname->name, iname->len);
> - return oname->len;
> + return 0;
> }
>
> static const char *lookup_table =
> @@ -279,6 +281,10 @@ EXPORT_SYMBOL(fscrypt_fname_free_buffer);
> /**
> * fscrypt_fname_disk_to_usr() - converts a filename from disk space to user
> * space
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
> */
> int fscrypt_fname_disk_to_usr(struct inode *inode,
> u32 hash, u32 minor_hash,
> @@ -287,13 +293,12 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
> {
> const struct qstr qname = FSTR_TO_QSTR(iname);
> char buf[24];
> - int ret;
>
> if (fscrypt_is_dot_dotdot(&qname)) {
> oname->name[0] = '.';
> oname->name[iname->len - 1] = '.';
> oname->len = iname->len;
> - return oname->len;
> + return 0;
> }
>
> if (iname->len < FS_CRYPTO_BLOCK_SIZE)
> @@ -303,9 +308,9 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
> return fname_decrypt(inode, iname, oname);
>
> if (iname->len <= FS_FNAME_CRYPTO_DIGEST_SIZE) {
> - ret = digest_encode(iname->name, iname->len, oname->name);
> - oname->len = ret;
> - return ret;
> + oname->len = digest_encode(iname->name, iname->len,
> + oname->name);
> + return 0;
> }
> if (hash) {
> memcpy(buf, &hash, 4);
> @@ -315,15 +320,18 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
> }
> memcpy(buf + 8, iname->name + iname->len - 16, 16);
> oname->name[0] = '_';
> - ret = digest_encode(buf, 24, oname->name + 1);
> - oname->len = ret + 1;
> - return ret + 1;
> + oname->len = 1 + digest_encode(buf, 24, oname->name + 1);
> + return 0;
> }
> EXPORT_SYMBOL(fscrypt_fname_disk_to_usr);
>
> /**
> * fscrypt_fname_usr_to_disk() - converts a filename from user space to disk
> * space
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
> */
> int fscrypt_fname_usr_to_disk(struct inode *inode,
> const struct qstr *iname,
> @@ -333,7 +341,7 @@ int fscrypt_fname_usr_to_disk(struct inode *inode,
> oname->name[0] = '.';
> oname->name[iname->len - 1] = '.';
> oname->len = iname->len;
> - return oname->len;
> + return 0;
> }
> if (inode->i_crypt_info)
> return fname_encrypt(inode, iname, oname);
> @@ -367,10 +375,10 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
> if (dir->i_crypt_info) {
> ret = fscrypt_fname_alloc_buffer(dir, iname->len,
> &fname->crypto_buf);
> - if (ret < 0)
> + if (ret)
> return ret;
> ret = fname_encrypt(dir, iname, &fname->crypto_buf);
> - if (ret < 0)
> + if (ret)
> goto errout;
> fname->disk_name.name = fname->crypto_buf.name;
> fname->disk_name.len = fname->crypto_buf.len;
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index 67415e0..4d4b910 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -260,11 +260,12 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
> /* Directory is encrypted */
> err = fscrypt_fname_disk_to_usr(inode,
> 0, 0, &de_name, &fstr);
> + de_name = fstr;
> fstr.len = save_len;
> - if (err < 0)
> + if (err)
> goto errout;
> if (!dir_emit(ctx,
> - fstr.name, err,
> + de_name.name, de_name.len,
> le32_to_cpu(de->inode),
> get_dtype(sb, de->file_type)))
> goto done;
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 34c0142..2243ae2 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -639,7 +639,7 @@ static struct stats dx_show_leaf(struct inode *dir,
> res = fscrypt_fname_alloc_buffer(
> dir, len,
> &fname_crypto_str);
> - if (res < 0)
> + if (res)
> printk(KERN_WARNING "Error "
> "allocating crypto "
> "buffer--skipping "
> @@ -647,7 +647,7 @@ static struct stats dx_show_leaf(struct inode *dir,
> res = fscrypt_fname_disk_to_usr(dir,
> 0, 0, &de_name,
> &fname_crypto_str);
> - if (res < 0) {
> + if (res) {
> printk(KERN_WARNING "Error "
> "converting filename "
> "from disk to usr"
> @@ -1011,7 +1011,7 @@ static int htree_dirblock_to_tree(struct file *dir_file,
> err = fscrypt_fname_disk_to_usr(dir, hinfo->hash,
> hinfo->minor_hash, &de_name,
> &fname_crypto_str);
> - if (err < 0) {
> + if (err) {
> count = err;
> goto errout;
> }
> @@ -3144,7 +3144,7 @@ static int ext4_symlink(struct inode *dir,
> istr.name = (const unsigned char *) symname;
> istr.len = len;
> err = fscrypt_fname_usr_to_disk(inode, &istr, &ostr);
> - if (err < 0)
> + if (err)
> goto err_drop_inode;
> sd->len = cpu_to_le16(ostr.len);
> disk_link.name = (char *) sd;
> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> index 4d83d9e..55aaf30 100644
> --- a/fs/ext4/symlink.c
> +++ b/fs/ext4/symlink.c
> @@ -67,14 +67,13 @@ static const char *ext4_encrypted_get_link(struct dentry *dentry,
> goto errout;
>
> res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
> - if (res < 0)
> + if (res)
> goto errout;
>
> paddr = pstr.name;
>
> /* Null-terminate the name */
> - if (res <= pstr.len)
> - paddr[res] = '\0';
> + paddr[pstr.len] = '\0';
> if (cpage)
> put_page(cpage);
> set_delayed_call(done, kfree_link, paddr);
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 9054aea..8716943 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -786,7 +786,7 @@ bool f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
>
> if (f2fs_encrypted_inode(d->inode)) {
> int save_len = fstr->len;
> - int ret;
> + int err;
>
> de_name.name = f2fs_kmalloc(de_name.len, GFP_NOFS);
> if (!de_name.name)
> @@ -794,11 +794,11 @@ bool f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
>
> memcpy(de_name.name, d->filename[bit_pos], de_name.len);
>
> - ret = fscrypt_fname_disk_to_usr(d->inode,
> + err = fscrypt_fname_disk_to_usr(d->inode,
> (u32)de->hash_code, 0,
> &de_name, fstr);
> kfree(de_name.name);
> - if (ret < 0)
> + if (err)
> return true;
>
> de_name = *fstr;
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 73fa356..afd5633 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -449,7 +449,7 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> ostr.name = sd->encrypted_path;
> ostr.len = disk_link.len;
> err = fscrypt_fname_usr_to_disk(inode, &istr, &ostr);
> - if (err < 0)
> + if (err)
> goto err_out;
>
> sd->len = cpu_to_le16(ostr.len);
> @@ -1048,7 +1048,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
> goto errout;
>
> res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
> - if (res < 0)
> + if (res)
> goto errout;
>
> /* this is broken symlink case */
> @@ -1060,7 +1060,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
> paddr = pstr.name;
>
> /* Null-terminate the name */
> - paddr[res] = '\0';
> + paddr[pstr.len] = '\0';
>
> put_page(cpage);
> set_delayed_call(done, kfree_link, paddr);
> --
> 2.8.0.rc3.226.g39d4020
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers, Andreas
On Thu, Sep 15, 2016 at 10:28:49AM -0700, Eric Biggers wrote:
> Several filename crypto functions: fname_decrypt(),
> fscrypt_fname_disk_to_usr(), and fscrypt_fname_usr_to_disk(), returned
> the output length on success or -errno on failure. However, the output
> length was redundant with the value written to 'oname->len'. It is also
> potentially error-prone to make callers have to check for '< 0' instead
> of '!= 0'.
>
> Therefore, make these functions return 0 instead of a length, and make
> the callers who cared about the return value being a length use
> 'oname->len' instead. For consistency also make other callers check for
> a nonzero result rather than a negative result.
>
> This change also fixes the inconsistency of fname_encrypt() actually
> already returning 0 on success, not a length like the other filename
> crypto functions and as documented in its function comment.
>
> Signed-off-by: Eric Biggers <[email protected]>
Acked-by: Jaegeuk Kim <[email protected]>
> ---
> fs/crypto/fname.c | 56 +++++++++++++++++++++++++++++++------------------------
> fs/ext4/dir.c | 5 +++--
> fs/ext4/namei.c | 8 ++++----
> fs/ext4/symlink.c | 5 ++---
> fs/f2fs/dir.c | 6 +++---
> fs/f2fs/namei.c | 6 +++---
> 6 files changed, 47 insertions(+), 39 deletions(-)
>
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 6bbc3b1..90697c7 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -35,11 +35,11 @@ static void fname_crypt_complete(struct crypto_async_request *req, int res)
> }
>
> /**
> - * fname_encrypt() -
> + * fname_encrypt() - encrypt a filename
> *
> - * This function encrypts the input filename, and returns the length of the
> - * ciphertext. Errors are returned as negative numbers. We trust the caller to
> - * allocate sufficient memory to oname string.
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
> */
> static int fname_encrypt(struct inode *inode,
> const struct qstr *iname, struct fscrypt_str *oname)
> @@ -105,20 +105,22 @@ 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 0;
> }
>
> -/*
> - * fname_decrypt()
> - * This function decrypts the input filename, and returns
> - * the length of the plaintext.
> - * Errors are returned as negative numbers.
> - * We trust the caller to allocate sufficient memory to oname string.
> +/**
> + * fname_decrypt() - decrypt a filename
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
> */
> static int fname_decrypt(struct inode *inode,
> const struct fscrypt_str *iname,
> @@ -168,7 +170,7 @@ static int fname_decrypt(struct inode *inode,
> }
>
> oname->len = strnlen(oname->name, iname->len);
> - return oname->len;
> + return 0;
> }
>
> static const char *lookup_table =
> @@ -279,6 +281,10 @@ EXPORT_SYMBOL(fscrypt_fname_free_buffer);
> /**
> * fscrypt_fname_disk_to_usr() - converts a filename from disk space to user
> * space
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
> */
> int fscrypt_fname_disk_to_usr(struct inode *inode,
> u32 hash, u32 minor_hash,
> @@ -287,13 +293,12 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
> {
> const struct qstr qname = FSTR_TO_QSTR(iname);
> char buf[24];
> - int ret;
>
> if (fscrypt_is_dot_dotdot(&qname)) {
> oname->name[0] = '.';
> oname->name[iname->len - 1] = '.';
> oname->len = iname->len;
> - return oname->len;
> + return 0;
> }
>
> if (iname->len < FS_CRYPTO_BLOCK_SIZE)
> @@ -303,9 +308,9 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
> return fname_decrypt(inode, iname, oname);
>
> if (iname->len <= FS_FNAME_CRYPTO_DIGEST_SIZE) {
> - ret = digest_encode(iname->name, iname->len, oname->name);
> - oname->len = ret;
> - return ret;
> + oname->len = digest_encode(iname->name, iname->len,
> + oname->name);
> + return 0;
> }
> if (hash) {
> memcpy(buf, &hash, 4);
> @@ -315,15 +320,18 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
> }
> memcpy(buf + 8, iname->name + iname->len - 16, 16);
> oname->name[0] = '_';
> - ret = digest_encode(buf, 24, oname->name + 1);
> - oname->len = ret + 1;
> - return ret + 1;
> + oname->len = 1 + digest_encode(buf, 24, oname->name + 1);
> + return 0;
> }
> EXPORT_SYMBOL(fscrypt_fname_disk_to_usr);
>
> /**
> * fscrypt_fname_usr_to_disk() - converts a filename from user space to disk
> * space
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
> */
> int fscrypt_fname_usr_to_disk(struct inode *inode,
> const struct qstr *iname,
> @@ -333,7 +341,7 @@ int fscrypt_fname_usr_to_disk(struct inode *inode,
> oname->name[0] = '.';
> oname->name[iname->len - 1] = '.';
> oname->len = iname->len;
> - return oname->len;
> + return 0;
> }
> if (inode->i_crypt_info)
> return fname_encrypt(inode, iname, oname);
> @@ -367,10 +375,10 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
> if (dir->i_crypt_info) {
> ret = fscrypt_fname_alloc_buffer(dir, iname->len,
> &fname->crypto_buf);
> - if (ret < 0)
> + if (ret)
> return ret;
> ret = fname_encrypt(dir, iname, &fname->crypto_buf);
> - if (ret < 0)
> + if (ret)
> goto errout;
> fname->disk_name.name = fname->crypto_buf.name;
> fname->disk_name.len = fname->crypto_buf.len;
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index 67415e0..4d4b910 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -260,11 +260,12 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
> /* Directory is encrypted */
> err = fscrypt_fname_disk_to_usr(inode,
> 0, 0, &de_name, &fstr);
> + de_name = fstr;
> fstr.len = save_len;
> - if (err < 0)
> + if (err)
> goto errout;
> if (!dir_emit(ctx,
> - fstr.name, err,
> + de_name.name, de_name.len,
> le32_to_cpu(de->inode),
> get_dtype(sb, de->file_type)))
> goto done;
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 34c0142..2243ae2 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -639,7 +639,7 @@ static struct stats dx_show_leaf(struct inode *dir,
> res = fscrypt_fname_alloc_buffer(
> dir, len,
> &fname_crypto_str);
> - if (res < 0)
> + if (res)
> printk(KERN_WARNING "Error "
> "allocating crypto "
> "buffer--skipping "
> @@ -647,7 +647,7 @@ static struct stats dx_show_leaf(struct inode *dir,
> res = fscrypt_fname_disk_to_usr(dir,
> 0, 0, &de_name,
> &fname_crypto_str);
> - if (res < 0) {
> + if (res) {
> printk(KERN_WARNING "Error "
> "converting filename "
> "from disk to usr"
> @@ -1011,7 +1011,7 @@ static int htree_dirblock_to_tree(struct file *dir_file,
> err = fscrypt_fname_disk_to_usr(dir, hinfo->hash,
> hinfo->minor_hash, &de_name,
> &fname_crypto_str);
> - if (err < 0) {
> + if (err) {
> count = err;
> goto errout;
> }
> @@ -3144,7 +3144,7 @@ static int ext4_symlink(struct inode *dir,
> istr.name = (const unsigned char *) symname;
> istr.len = len;
> err = fscrypt_fname_usr_to_disk(inode, &istr, &ostr);
> - if (err < 0)
> + if (err)
> goto err_drop_inode;
> sd->len = cpu_to_le16(ostr.len);
> disk_link.name = (char *) sd;
> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> index 4d83d9e..55aaf30 100644
> --- a/fs/ext4/symlink.c
> +++ b/fs/ext4/symlink.c
> @@ -67,14 +67,13 @@ static const char *ext4_encrypted_get_link(struct dentry *dentry,
> goto errout;
>
> res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
> - if (res < 0)
> + if (res)
> goto errout;
>
> paddr = pstr.name;
>
> /* Null-terminate the name */
> - if (res <= pstr.len)
> - paddr[res] = '\0';
> + paddr[pstr.len] = '\0';
> if (cpage)
> put_page(cpage);
> set_delayed_call(done, kfree_link, paddr);
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 9054aea..8716943 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -786,7 +786,7 @@ bool f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
>
> if (f2fs_encrypted_inode(d->inode)) {
> int save_len = fstr->len;
> - int ret;
> + int err;
>
> de_name.name = f2fs_kmalloc(de_name.len, GFP_NOFS);
> if (!de_name.name)
> @@ -794,11 +794,11 @@ bool f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
>
> memcpy(de_name.name, d->filename[bit_pos], de_name.len);
>
> - ret = fscrypt_fname_disk_to_usr(d->inode,
> + err = fscrypt_fname_disk_to_usr(d->inode,
> (u32)de->hash_code, 0,
> &de_name, fstr);
> kfree(de_name.name);
> - if (ret < 0)
> + if (err)
> return true;
>
> de_name = *fstr;
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 73fa356..afd5633 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -449,7 +449,7 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> ostr.name = sd->encrypted_path;
> ostr.len = disk_link.len;
> err = fscrypt_fname_usr_to_disk(inode, &istr, &ostr);
> - if (err < 0)
> + if (err)
> goto err_out;
>
> sd->len = cpu_to_le16(ostr.len);
> @@ -1048,7 +1048,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
> goto errout;
>
> res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
> - if (res < 0)
> + if (res)
> goto errout;
>
> /* this is broken symlink case */
> @@ -1060,7 +1060,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
> paddr = pstr.name;
>
> /* Null-terminate the name */
> - paddr[res] = '\0';
> + paddr[pstr.len] = '\0';
>
> put_page(cpage);
> set_delayed_call(done, kfree_link, paddr);
> --
> 2.8.0.rc3.226.g39d4020
------------------------------------------------------------------------------
On Thu, Sep 15, 2016 at 01:24:01PM -0700, Jaegeuk Kim wrote:
> On Thu, Sep 15, 2016 at 10:28:49AM -0700, Eric Biggers wrote:
> > Several filename crypto functions: fname_decrypt(),
> > fscrypt_fname_disk_to_usr(), and fscrypt_fname_usr_to_disk(), returned
> > the output length on success or -errno on failure. However, the output
> > length was redundant with the value written to 'oname->len'. It is also
> > potentially error-prone to make callers have to check for '< 0' instead
> > of '!= 0'.
> >
> > Therefore, make these functions return 0 instead of a length, and make
> > the callers who cared about the return value being a length use
> > 'oname->len' instead. For consistency also make other callers check for
> > a nonzero result rather than a negative result.
> >
> > This change also fixes the inconsistency of fname_encrypt() actually
> > already returning 0 on success, not a length like the other filename
> > crypto functions and as documented in its function comment.
> >
> > Signed-off-by: Eric Biggers <[email protected]>
>
> Acked-by: Jaegeuk Kim <[email protected]>
Thanks, applied.
- Ted