linux-kernel - Re: [PATCH] ecryptfs: disallow ecryptfs as underlying filesystem

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: <4BD30288.1040501@redhat.com>
Date:	Sat, 24 Apr 2010 09:39:04 -0500
From:	Eric Sandeen <sandeen@...hat.com>
To:	Pekka Enberg <penberg@...helsinki.fi>
CC:	kernel list <linux-kernel@...r.kernel.org>,
	Tyler Hicks <tyhicks@...ux.vnet.ibm.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	Christoph Hellwig <hch@....de>
Subject: Re: [PATCH] ecryptfs: disallow ecryptfs as underlying filesystem

Pekka Enberg wrote:
> On Sat, Apr 24, 2010 at 5:41 AM, Eric Sandeen <sandeen@...hat.com> wrote:
>> mounting stacked ecryptfs on ecryptfs has been shown to lead to bugs
>> in testing.  For crypto info in xattr, there is no mechanism for handling
>> this at all, and for normal file headers, we run into other trouble:
>>
>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
>> IP: [<ffffffffa015b0b3>] ecryptfs_d_revalidate+0x43/0xa0 [ecryptfs]
>> ...
>>
>> There doesn't seem to be any good usecase for this, so I'd suggest just
>> disallowing the configuration.
> 
> Maybe there's no good use case for it but it sure sounds like a good
> test case for shaking out bugs in filesystem stacking code.

I could revise the patch to allow a force-override option if you're interested
in doing that shaking.  :)

(for cryptinfo-in-xattr, though, there is simply no mechanism to support this
at all in ecryptfs, and I doubt it's a design goal, though will defer to tyhicks
on all this)

-Eric

>> Based on a patch originally, I believe, from Mike Halcrow.
>>
>> Signed-off-by: Eric Sandeen <sandeen@...hat.com>
>> ---
>>
>> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
>> index af1a8f0..7ada044 100644
>> --- a/fs/ecryptfs/main.c
>> +++ b/fs/ecryptfs/main.c
>> @@ -594,28 +594,46 @@ static int ecryptfs_get_sb(struct file_system_type *fs_type, int flags,
>>                        struct vfsmount *mnt)
>>  {
>>        int rc;
>> -       struct super_block *sb;
>> +       struct super_block *sb, *lower_sb;
>> +       struct nameidata nd;
>> +
>> +       rc = path_lookup(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &nd);
>> +       if (rc) {
>> +               printk(KERN_WARNING
>> +                       "path_lookup() failed on dev_name = [%s]\n", dev_name);
>> +               goto out;
>> +       }
>> +       lower_sb = nd.path.dentry->d_sb;
>> +       if (strcmp(lower_sb->s_type->name, "ecryptfs") == 0) {
>> +               rc = -EINVAL;
>> +               printk(KERN_ERR "Mount on filesystem of type "
>> +                       "eCryptfs explicitly disallowed due to "
>> +                       "known incompatibilities\n");
>> +               goto out_pathput;
>> +       }
>>
>>        rc = get_sb_nodev(fs_type, flags, raw_data, ecryptfs_fill_super, mnt);
>>        if (rc < 0) {
>>                printk(KERN_ERR "Getting sb failed; rc = [%d]\n", rc);
>> -               goto out;
>> +               goto out_pathput;
>>        }
>>        sb = mnt->mnt_sb;
>>        rc = ecryptfs_parse_options(sb, raw_data);
>>        if (rc) {
>>                printk(KERN_ERR "Error parsing options; rc = [%d]\n", rc);
>> -               goto out_abort;
>> +               goto out_dput;
>>        }
>>        rc = ecryptfs_read_super(sb, dev_name);
>>        if (rc) {
>>                printk(KERN_ERR "Reading sb failed; rc = [%d]\n", rc);
>> -               goto out_abort;
>> +               goto out_dput;
>>        }
>>        goto out;
>> -out_abort:
>> +out_dput:
>>        dput(sb->s_root); /* aka mnt->mnt_root, as set by get_sb_nodev() */
>>        deactivate_locked_super(sb);
>> +out_pathput:
>> +       path_put(&nd.path);
>>  out:
>>        return rc;
>>  }
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/