linux-kernel - Re: [PATCH v2 2/2] fat: always use dir_emit

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zn1gQeWToPNkp9nt@quatroqueijos.cascardo.eti.br>
Date: Thu, 27 Jun 2024 09:51:13 -0300
From: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
To: OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Gwendal Grignou <gwendal@...omium.org>, dlunev@...omium.org
Subject: Re: [PATCH v2 2/2] fat: always use dir_emit_dots and ignore . and ..
 entries

On Thu, Jun 27, 2024 at 05:10:44AM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@...lia.com> writes:
> 
> >> Unacceptable to change the correct behavior to broken format. And
> >> unlikely break the userspace, however this still has the user visible
> >> change of seek pos.
> >> 
> >> Thanks.
> >> 
> >
> > I agree that if this breaks userspace with a good filesystem or regresses
> > in a way that real applications would break, that this needs to be redone.
> >
> > However, I spent a few hours doing some extra testing (I had already run
> > some xfstests that include directory testing) and I failed to find any
> > issues with this fix.
> >
> > If this would break, it would have broken the root directory. In the case
> > of a directory including the . and .. entries, the d_off for the .. entry
> > will be set for the first non-dot-or-dotdot entry. For ., it will be set as
> > 1, which, if used by telldir (or llseek), will emit the .. entry, as
> > expected.
> >
> > For the case where both . and .. are absent, the first real entry will have
> > d_off as 2, and it will just work.
> >
> > So everything seems to work as expected. Do you see any user visible change
> > that would break any applications?
> 
> First of all, I'm not thinking this is the fix, I'm thinking this as the
> workaround of broken formatter (because the windows's fsck also think it
> as broken). So very low priority to support.
> 
> As said, I also think low chance to break the userspace. However it
> changes real offset to pseudo offset. So if userspace saved it to
> persistent space, breaks userspace. Unlikely, but I think there is no
> value to change the behavior for workaround.
> 
> Thanks.
> -- 
> OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>

I looked at that perspective, but still wanted to allow users to use such
filesystems, even if they needed to fsck it first.

But there is the issue that when such filesystems are mounted, they are
further corrupted, preventing such fsck from correctly fixing and allowing
access to the data.

So I started doing some investigation and that lead me to the following
code from fs/fat/inode.c:

static void fat_evict_inode(struct inode *inode)
{
	truncate_inode_pages_final(&inode->i_data);
	if (!inode->i_nlink) {
		inode->i_size = 0;
		fat_truncate_blocks(inode, 0);
	} else
		fat_free_eofblocks(inode);
[...]

That is, since the directory has no links, once it is evicted (which
happens right after reading the number of subdirectories and failing
verification), it is truncated. That means all clusters are marked as FREE.
Then, later, if trying to fsck or mount this filesystem again, the
directory entry is removed or further errors show up (as an EOF is
expected, not a FREE cluster).

And that is caused by attributing a number of 0 links. I looked it up on
how other filesystems handle this situation and I found out that exfat adds
2 to the number of subdirectories, just as I am suggesting. When
enumerating the directories (at its readdir), it also relies on
dir_emit_dots for all cases.

As for programs persisting the offset, the manpage for telldir has on its
NOTES section:

"""
Application programs should treat this strictly as an opaque value, making
no assumptions about its contents.
"""

I know this doesn't refer to persisting or not that opaque value, but any
other changes to the directory would change the offset of its current
subdirectories and given those values are opaque, no assumptions should be
made. And unless we find such programs in the wild, the same argunent could
be made that there may be programs that expect . and .. to be at offset 0
and 1, like every filesystem that uses dir_emit_dots does.

I understand the cautiousness to prevent regressions, but I did the work
here to test and understand the changes that are being proposed. I even
looked into another way of preventing the further corruption, but that
convinced me even more that the right fix is to assign a minimum number of
links to directories and I found precedence to this.

Thanks.
Cascardo.