Re: Intentionally corrupted vfat fs causing BUG

[Richard Weinberger <richard@nod.at>]

Am 13.10.2014 um 10:59 schrieb OGAWA Hirofumi:
> Richard Weinberger <richard@nod.at> writes:
> 
>> Am 13.10.2014 um 10:35 schrieb OGAWA Hirofumi:
>>> Richard Weinberger <richard@nod.at> writes:
>>>
>>>>> I'm still not sure whether this is right direction or not though,
>>>>> because mount operation is root only and untrusted image should run fsck
>>>>> before. But, also, Oops is clearly unexpected. Hmmm...
>>>>
>>>> This limitation is not true anymore. Plug in a USB stick into a recent
>>>> Linux desktop, it will automatically mount it...  Also think of user
>>>> namespaces and FUSE.
>>>
>>> Not really (well, true, some sort though). It is still controlled by root
>>> or capability, right?  I.e. still controlled by admin of system.
>>
>> Fact is, I can plugin a USB stick to my buddies Laptop and make it trigger a BUG_ON. :)
>>
>>> I read user namespaces last time, it doesn't allow to mount the block
>>> device by namespace's root.
>>>
>>> FUSE is allowed to mount by true user (I.e. admin can't disallow it)? I
>>> still didn't check it fully.
>>
>> The question is how long these limits will stay...
>> User namespaces uncovered already a pile of issues wrt. to mounting.
> 
> Well, anyway, I don't object like that simple patch.
> 
> My worry is, I feel we need something like online-fsck finally if we
> tackled fully to avoid issues (I still didn't analyze about this issue
> seriously and fully), and measurable overheads.
> 
> And I myself have interest to online/runtime-fsck (i.e. detect and fix)
> though, I don't have interest to make it generic operations, and I would
> not have interest to tackle for all FSes...
> 

What about this one?

diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index 6df8d3d..60a28b7 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -736,7 +736,8 @@ static struct dentry *vfat_lookup(struct inode *dir, struct dentry *dentry,
 	}

 	alias = d_find_alias(inode);
-	if (alias && !vfat_d_anon_disconn(alias)) {
+	if (alias && !vfat_d_anon_disconn(alias) &&
+	    alias->d_parent == dentry->d_parent) {
 		/*
 		 * This inode has non anonymous-DCACHE_DISCONNECTED
 		 * dentry. This means, the user did ->lookup() by an

VFAT suffers from the issue because it is using dentry aliases to have fast lookups
for short and long names. Without aliases the VFS will catch the loop just fine as on ext2 (I've
tested it).
Let's change vfat_lookup() to verify that both the alias and dentry have the same parent otherwise
we're facing a loop.

Thanks,
//richard