Re: [PATCH] fscrypto: make fname_encrypt() actually return length of ciphertext

On Sep 14, 2016, at 2:57 PM, Eric Biggers [off-list ref] 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

Cheers, Andreas