On Tue, Jan 28, 2020 at 06:23:18PM -0700, Andreas Dilger wrote:
> On Jan 28, 2020, at 3:11 PM, Matthew Wilcox <willy@xxxxxxxxxxxxx> 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.
