linux-ext4 - Re: [PATCH v4] fs: introduce is_dot_or

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200129073630.GF6615@bombadil.infradead.org>
Date:   Tue, 28 Jan 2020 23:36:30 -0800
From:   Matthew Wilcox <willy@...radead.org>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>,
        "Theodore Y. Ts'o" <tytso@....edu>, Chao Yu <yuchao0@...wei.com>,
        linux-fscrypt@...r.kernel.org,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH v4] fs: introduce is_dot_or_dotdot helper for cleanup

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