linux-ext4 - Re: buggy readdir with inline dirs

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]

Date:	Sat, 23 Mar 2013 10:28:05 -0700
From:	Andreas Dilger <adilger@...ger.ca>
To:	Tao Ma <tm@....ma>
Cc:	Zach Brown <zab@...hat.com>,
	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
	Eric Sandeen <sandeen@...hat.com>
Subject: Re: buggy readdir with inline dirs

On 2013-03-23, at 6:24, Tao Ma <tm@....ma> wrote:
> On 03/23/2013 02:26 AM, Zach Brown wrote:
>> I don't remember quite how, but I found myself flipping through the
>> inline dir code that's in mainline now.  It looked pretty fishy so Eric
>> and I played around with it.  It's very buggy in its current form.
> Sorry about any inconvenience brought to you.
>> 
>> ext4_read_inline_dir() doesn't seem to undertand the filldir arguments.
>> It suggests that offset 0 is the next offset after both the "." and ".."
>> entries.  It needs to have specific offsets for "." and ".." and return them
>> accordingly.  It looks like fixing this will trickle down into the
>> revalidation loop.
> yes, it is my fault, I guess at the very first beginning,  I just can't
> figured out how to return a proper 'offset' to the user to indicate
> '..'. Now we don't save anything about '.', so offset 0 is OK for it,
> but maybe we should return some offset like '2' to the user about it.
> Anyway it should be fixed.

FYI, ZFS (which generates "." and ".." entries on-the-fly also) uses "0" for start of readdir, "1" for ".", and "2" for "..".

Cheers, Andreas 

>> It doesn't understand that it's possible to only return a single "."
>> entry in getdents and have a subsequent call have f_pos pointing at the
>> fake ".." entry.  With the current code if your getdents buffer only has
>> room for "." it just spins returning that entry leaving f_pos at 0.
> Sorry.
>> 
>> Those are all relatively simple bugs that just need to be fixed.
>> 
>> But the big bug is that it changes the d_off values for entries as it
>> converts from byte offsets in the inline dir xattr to hashed offsets in
>> indexed dir leaves.  A concurrent readdir could be unlucky enough to get
>> a bunch of duplicate entries as it reads past the nice low inline byte
>> offsets into the huge hashed offsets.
>> 
>> I'm not sure how to easily fix that.  It feels like it'd want to
>> maintain the dir entries in the xattr blob with the offsets that they'll
>> have once converted to full dir blocks.  So instead of being a magical
>> readdir path maybe it wants to be in the path of looking up dir blocks
>> so existing unindexed and indexed code would operate on the data in the
>> xattr blob as though it were a block?
>> 
>> Dunno, just wanted to share what we found.  Are these all known problems
>> in prototype code that isn't intended to be used?
> I will check what xfs does in this case as Dave mentioned in another
> reply and come back with a fix about it.
> 
> Thanks,
> Tao
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html