| Message ID | 1473886634-24627-2-git-send-email-ebiggers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sep 14, 2016, at 2:57 PM, Eric Biggers <ebiggers@google.com> 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@google.com> > --- > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas
On Wed, Sep 14, 2016 at 03:37:01PM -0600, Andreas Dilger wrote: > On Sep 14, 2016, at 2:57 PM, Eric Biggers <ebiggers@google.com> 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: > fscrypt_fname_usr_to_disk() already returned a length in the "." and ".." cases. So any caller which assumed it returned 0 on success would have already been buggy. Fortunately, there aren't any such callers currently. > > 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. > I agree that the return value is redundant and somewhat error prone. However, this style is already being used for fscrypt_fname_disk_to_usr(), fscrypt_fname_usr_to_disk(), and fname_decrypt(). My patch was primarily intended to make things more consistent by updating fname_encrypt(), which was the odd one out. If you'd prefer, I can instead do a patch to make all these related functions return 0 on success, rather than a length. That would be a somewhat larger patch. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 14, 2016 at 02:57:04PM -0700, Eric Biggers wrote: > I agree that the return value is redundant and somewhat error prone. However, > this style is already being used for fscrypt_fname_disk_to_usr(), > fscrypt_fname_usr_to_disk(), and fname_decrypt(). My patch was primarily > intended to make things more consistent by updating fname_encrypt(), which was > the odd one out. If you'd prefer, I can instead do a patch to make all these > related functions return 0 on success, rather than a length. That would be a > somewhat larger patch. > To see more concretely what it looks like, I went ahead and wrote the "make the functions return 0" version of the patch. I'm sending it to be considered as well. In theory I think it's better, though it's a larger patch. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } /*
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. Signed-off-by: Eric Biggers <ebiggers@google.com> --- fs/crypto/fname.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)