| Message ID | 1575979801-32569-1-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] fs: introduce is_dot_or_dotdot helper for cleanup | expand |
On Tue, Dec 10, 2019 at 08:10:01PM +0800, Tiezhu Yang wrote: > There exists many similar and duplicate codes to check "." and "..", > so introduce is_dot_or_dotdot helper to make the code more clean. > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
On Tue, Dec 10, 2019 at 08:10:01PM +0800, Tiezhu Yang wrote: > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c > index 3da3707..ef7eba8 100644 > --- a/fs/crypto/fname.c > +++ b/fs/crypto/fname.c > @@ -11,21 +11,11 @@ > * This has not yet undergone a rigorous security audit. > */ > > +#include <linux/namei.h> > #include <linux/scatterlist.h> > #include <crypto/skcipher.h> > #include "fscrypt_private.h" > > -static inline bool fscrypt_is_dot_dotdot(const struct qstr *str) > -{ > - if (str->len == 1 && str->name[0] == '.') > - return true; > - > - if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.') > - return true; > - > - return false; > -} > - > /** > * fname_encrypt() - encrypt a filename > * > @@ -255,7 +245,7 @@ int fscrypt_fname_disk_to_usr(struct inode *inode, > const struct qstr qname = FSTR_TO_QSTR(iname); > struct fscrypt_digested_name digested_name; > > - if (fscrypt_is_dot_dotdot(&qname)) { > + if (is_dot_or_dotdot(qname.name, qname.len)) { There's no need for the 'qname' variable anymore. Can you please remove it and do: if (is_dot_or_dotdot(iname->name, iname->len)) { > diff --git a/include/linux/namei.h b/include/linux/namei.h > index 7fe7b87..aba114a 100644 > --- a/include/linux/namei.h > +++ b/include/linux/namei.h > @@ -92,4 +92,14 @@ retry_estale(const long error, const unsigned int flags) > return error == -ESTALE && !(flags & LOOKUP_REVAL); > } > > +static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) > +{ > + if (unlikely(name[0] == '.')) { > + if (len < 2 || (len == 2 && name[1] == '.')) > + return true; > + } > + > + return false; > +} This doesn't handle the len=0 case. Did you check that none of the users pass in zero-length names? It looks like fscrypt_fname_disk_to_usr() can, if the directory entry on-disk has a zero-length name. Currently it will return -EUCLEAN in that case, but with this patch it may think it's the name ".". So I think there needs to either be a len >= 1 check added, *or* you need to make an argument for why it's okay to not care about the empty name case. - Eric
On Tue, Dec 10, 2019 at 11:19:13AM -0800, Eric Biggers wrote: > > +static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) > > +{ > > + if (unlikely(name[0] == '.')) { > > + if (len < 2 || (len == 2 && name[1] == '.')) > > + return true; > > + } > > + > > + return false; > > +} > > This doesn't handle the len=0 case. Did you check that none of the users pass > in zero-length names? It looks like fscrypt_fname_disk_to_usr() can, if the > directory entry on-disk has a zero-length name. Currently it will return > -EUCLEAN in that case, but with this patch it may think it's the name ".". > > So I think there needs to either be a len >= 1 check added, *or* you need to > make an argument for why it's okay to not care about the empty name case. Frankly, the only caller that matters in practice is link_path_walk(); _that_ is by far the hottest path that might make use of that thing. BTW, the callers that might end up passing 0 for len really ought to take a good look at another thing - that name[0] is, in fact, mapped. Something along the lines of if (name + len > end_of_buffer) sod off if (<that function>(name, len)) .... is not enough, for obvious reasons.
On 12/11/2019 03:19 AM, Eric Biggers wrote: > On Tue, Dec 10, 2019 at 08:10:01PM +0800, Tiezhu Yang wrote: >> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c >> index 3da3707..ef7eba8 100644 >> --- a/fs/crypto/fname.c >> +++ b/fs/crypto/fname.c >> @@ -11,21 +11,11 @@ >> * This has not yet undergone a rigorous security audit. >> */ >> >> +#include <linux/namei.h> >> #include <linux/scatterlist.h> >> #include <crypto/skcipher.h> >> #include "fscrypt_private.h" >> >> -static inline bool fscrypt_is_dot_dotdot(const struct qstr *str) >> -{ >> - if (str->len == 1 && str->name[0] == '.') >> - return true; >> - >> - if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.') >> - return true; >> - >> - return false; >> -} >> - >> /** >> * fname_encrypt() - encrypt a filename >> * >> @@ -255,7 +245,7 @@ int fscrypt_fname_disk_to_usr(struct inode *inode, >> const struct qstr qname = FSTR_TO_QSTR(iname); >> struct fscrypt_digested_name digested_name; >> >> - if (fscrypt_is_dot_dotdot(&qname)) { >> + if (is_dot_or_dotdot(qname.name, qname.len)) { > There's no need for the 'qname' variable anymore. Can you please remove it and > do: > > if (is_dot_or_dotdot(iname->name, iname->len)) { Hi Eric, Thanks for your review, I will do it in the v5 patch. > >> diff --git a/include/linux/namei.h b/include/linux/namei.h >> index 7fe7b87..aba114a 100644 >> --- a/include/linux/namei.h >> +++ b/include/linux/namei.h >> @@ -92,4 +92,14 @@ retry_estale(const long error, const unsigned int flags) >> return error == -ESTALE && !(flags & LOOKUP_REVAL); >> } >> >> +static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) >> +{ >> + if (unlikely(name[0] == '.')) { >> + if (len < 2 || (len == 2 && name[1] == '.')) >> + return true; >> + } >> + >> + return false; >> +} > This doesn't handle the len=0 case. Did you check that none of the users pass > in zero-length names? It looks like fscrypt_fname_disk_to_usr() can, if the > directory entry on-disk has a zero-length name. Currently it will return > -EUCLEAN in that case, but with this patch it may think it's the name ".". > > So I think there needs to either be a len >= 1 check added, *or* you need to > make an argument for why it's okay to not care about the empty name case. Anyway, let me modify the if condition "len < 2" to "len == 1". static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) { if (unlikely(name[0] == '.')) { if (len == 1 || (len == 2 && name[1] == '.')) return true; } return false; } I will send v5 patch as soon as possible. Thanks, Tiezhu Yang > > - Eric
On Tue, Dec 10, 2019 at 11:19:13AM -0800, Eric Biggers wrote: > > +static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) > > +{ > > + if (unlikely(name[0] == '.')) { > > + if (len < 2 || (len == 2 && name[1] == '.')) > > + return true; > > + } > > + > > + return false; > > +} > > This doesn't handle the len=0 case. Did you check that none of the users pass > in zero-length names? It looks like fscrypt_fname_disk_to_usr() can, if the > directory entry on-disk has a zero-length name. Currently it will return > -EUCLEAN in that case, but with this patch it may think it's the name ".". Trying to wrench this back on track ... fscrypt_fname_disk_to_usr is called by: fscrypt_get_symlink(): if (cstr.len == 0) return ERR_PTR(-EUCLEAN); ext4_readdir(): Does not currently check de->name_len. I believe this check should be added to __ext4_check_dir_entry() because a zero-length directory entry can affect both encrypted and non-encrypted directory entries. dx_show_leaf(): Same as ext4_readdir(). Should probably call ext4_check_dir_entry()? htree_dirblock_to_tree(): Would be covered by a fix to ext4_check_dir_entry(). f2fs_fill_dentries(): if (de->name_len == 0) { ... ubifs_readdir(): Does not currently check de->name_len. Also affects non-encrypted directory entries. So of the six callers, two of them already check the dirent length for being zero, and four of them ought to anyway, but don't. I think they should be fixed, but clearly we don't historically check for this kind of data corruption (strangely), so I don't think that's a reason to hold up this patch until the individual filesystems are fixed.
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c index 3da3707..ef7eba8 100644 --- a/fs/crypto/fname.c +++ b/fs/crypto/fname.c @@ -11,21 +11,11 @@ * This has not yet undergone a rigorous security audit. */ +#include <linux/namei.h> #include <linux/scatterlist.h> #include <crypto/skcipher.h> #include "fscrypt_private.h" -static inline bool fscrypt_is_dot_dotdot(const struct qstr *str) -{ - if (str->len == 1 && str->name[0] == '.') - return true; - - if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.') - return true; - - return false; -} - /** * fname_encrypt() - encrypt a filename * @@ -255,7 +245,7 @@ int fscrypt_fname_disk_to_usr(struct inode *inode, const struct qstr qname = FSTR_TO_QSTR(iname); struct fscrypt_digested_name digested_name; - if (fscrypt_is_dot_dotdot(&qname)) { + if (is_dot_or_dotdot(qname.name, qname.len)) { oname->name[0] = '.'; oname->name[iname->len - 1] = '.'; oname->len = iname->len; @@ -323,7 +313,7 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname, memset(fname, 0, sizeof(struct fscrypt_name)); fname->usr_fname = iname; - if (!IS_ENCRYPTED(dir) || fscrypt_is_dot_dotdot(iname)) { + if (!IS_ENCRYPTED(dir) || is_dot_or_dotdot(iname->name, iname->len)) { fname->disk_name.name = (unsigned char *)iname->name; fname->disk_name.len = iname->len; return 0; diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c index f91db24..2014f8f 100644 --- a/fs/ecryptfs/crypto.c +++ b/fs/ecryptfs/crypto.c @@ -1991,16 +1991,6 @@ int ecryptfs_encrypt_and_encode_filename( return rc; } -static bool is_dot_dotdot(const char *name, size_t name_size) -{ - if (name_size == 1 && name[0] == '.') - return true; - else if (name_size == 2 && name[0] == '.' && name[1] == '.') - return true; - - return false; -} - /** * ecryptfs_decode_and_decrypt_filename - converts the encoded cipher text name to decoded plaintext * @plaintext_name: The plaintext name diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 5a888a0..3d5e684 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -2767,17 +2767,6 @@ static inline bool f2fs_cp_error(struct f2fs_sb_info *sbi) return is_set_ckpt_flags(sbi, CP_ERROR_FLAG); } -static inline bool is_dot_dotdot(const struct qstr *str) -{ - if (str->len == 1 && str->name[0] == '.') - return true; - - if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.') - return true; - - return false; -} - static inline bool f2fs_may_extent_tree(struct inode *inode) { struct f2fs_sb_info *sbi = F2FS_I_SB(inode); diff --git a/fs/f2fs/hash.c b/fs/f2fs/hash.c index 5bc4dcd..ef155c2 100644 --- a/fs/f2fs/hash.c +++ b/fs/f2fs/hash.c @@ -15,6 +15,7 @@ #include <linux/cryptohash.h> #include <linux/pagemap.h> #include <linux/unicode.h> +#include <linux/namei.h> #include "f2fs.h" @@ -82,7 +83,7 @@ static f2fs_hash_t __f2fs_dentry_hash(const struct qstr *name_info, if (fname && !fname->disk_name.name) return cpu_to_le32(fname->hash); - if (is_dot_dotdot(name_info)) + if (is_dot_or_dotdot(name, len)) return 0; /* Initialize the default seed for the hash checksum functions */ diff --git a/fs/namei.c b/fs/namei.c index d6c91d1..f3a4439 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2451,10 +2451,8 @@ static int lookup_one_len_common(const char *name, struct dentry *base, if (!len) return -EACCES; - if (unlikely(name[0] == '.')) { - if (len < 2 || (len == 2 && name[1] == '.')) - return -EACCES; - } + if (is_dot_or_dotdot(name, len)) + return -EACCES; while (len--) { unsigned int c = *(const unsigned char *)name++; diff --git a/include/linux/namei.h b/include/linux/namei.h index 7fe7b87..aba114a 100644 --- a/include/linux/namei.h +++ b/include/linux/namei.h @@ -92,4 +92,14 @@ retry_estale(const long error, const unsigned int flags) return error == -ESTALE && !(flags & LOOKUP_REVAL); } +static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) +{ + if (unlikely(name[0] == '.')) { + if (len < 2 || (len == 2 && name[1] == '.')) + return true; + } + + return false; +} + #endif /* _LINUX_NAMEI_H */
There exists many similar and duplicate codes to check "." and "..", so introduce is_dot_or_dotdot helper to make the code more clean. Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- v4: - rename is_dot_dotdot() to is_dot_or_dotdot() v3: - use "name" and "len" as arguments instead of qstr - move is_dot_dotdot() to include/linux/namei.h v2: - use the better performance implementation of is_dot_dotdot - make it static inline and move it to include/linux/fs.h fs/crypto/fname.c | 16 +++------------- fs/ecryptfs/crypto.c | 10 ---------- fs/f2fs/f2fs.h | 11 ----------- fs/f2fs/hash.c | 3 ++- fs/namei.c | 6 ++---- include/linux/namei.h | 10 ++++++++++ 6 files changed, 17 insertions(+), 39 deletions(-)