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(-) 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 */ -- 2.1.0
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.
[-- Attachment #1: Type: text/plain, Size: 4261 bytes --] On Jan 28, 2020, at 3:11 PM, Matthew Wilcox <willy@infradead.org> wrote: > > > I've tried to get Ted's opinion on this a few times with radio silence. > Or email is broken. Anyone else care to offer an opinion? Maybe I'm missing something, but I think the discussion of the len == 0 case is now moot, since PATCH v6 (which is the latest version that I can find) is checking for "len >= 1" before accessing name[0]: +static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) +{ + if (len >= 1 && unlikely(name[0] == '.')) { + if (len == 1 || (len == 2 && name[1] == '.')) + return true; + } + + return false; +} This seems a tiny bit sub-optimal, as (len >= 1) is true for almost every filename, so it doesn't allow failing the condition quickly. Checking for exactly (len == 1) and (len == 2) allows failing this condition for most of the files immediately, which makes "unlikely()" actually useful, and allows simplifying the inside condition. static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) { if (unlikely((len == 1 || len == 2) && name[0] == '.')) { if (len == 1 || name[1] == '.') return true; } return false; } That said, this is at best micro-optimization so it isn't obvious this is much of an improvement or not. Cheers, Andreas > On Mon, Dec 30, 2019 at 06:13:03AM -0800, Matthew Wilcox wrote: >> >> Didn't see a response from you on this. Can you confirm the three >> cases in ext4 mentioned below should be converted to return -EUNCLEAN? >> >> ----- Forwarded message from Matthew Wilcox <willy@infradead.org> ----- >> >> Date: Thu, 12 Dec 2019 10:13:02 -0800 >> From: Matthew Wilcox <willy@infradead.org> >> To: Eric Biggers <ebiggers@kernel.org> >> Cc: Tiezhu Yang <yangtiezhu@loongson.cn>, Alexander Viro >> <viro@zeniv.linux.org.uk>, "Theodore Y. Ts'o" <tytso@mit.edu>, Jaegeuk >> Kim <jaegeuk@kernel.org>, Chao Yu <yuchao0@huawei.com>, Tyler Hicks >> <tyhicks@canonical.com>, linux-fsdevel@vger.kernel.org, >> ecryptfs@vger.kernel.org, linux-fscrypt@vger.kernel.org, >> linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v4] fs: introduce is_dot_or_dotdot helper for cleanup >> User-Agent: Mutt/1.12.1 (2019-06-15) >> >> 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. >> >> ----- End forwarded message ----- > > ----- End forwarded message ----- Cheers, Andreas [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 873 bytes --]
On Tue, Jan 28, 2020 at 06:23:18PM -0700, Andreas Dilger wrote: > On Jan 28, 2020, at 3:11 PM, Matthew Wilcox <willy@infradead.org> wrote: > > I've tried to get Ted's opinion on this a few times with radio silence. > > Or email is broken. Anyone else care to offer an opinion? > > Maybe I'm missing something, but I think the discussion of the len == 0 > case is now moot, since PATCH v6 (which is the latest version that I can > find) is checking for "len >= 1" before accessing name[0]: Regardless of _this_ patch, the question is "Should ext4 be checking for filenames of zero length and reporting -EUCLEAN if it finds any?" I believe the answer is yes, since it's clearly a corrupted filesystem, but I may be missing something. Thanks for your reply. > >> 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.