* [PATCH] ecryptfs: disallow ecryptfs as underlying filesystem
@ 2010-04-24 2:41 Eric Sandeen
2010-04-24 8:54 ` Pekka Enberg
0 siblings, 1 reply; 4+ messages in thread
From: Eric Sandeen @ 2010-04-24 2:41 UTC (permalink / raw)
To: kernel list, Tyler Hicks
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.
Based on a patch originally, I believe, from Mike Halcrow.
Signed-off-by: Eric Sandeen <sandeen@redhat.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;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] ecryptfs: disallow ecryptfs as underlying filesystem
2010-04-24 2:41 [PATCH] ecryptfs: disallow ecryptfs as underlying filesystem Eric Sandeen
@ 2010-04-24 8:54 ` Pekka Enberg
2010-04-24 14:39 ` Eric Sandeen
0 siblings, 1 reply; 4+ messages in thread
From: Pekka Enberg @ 2010-04-24 8:54 UTC (permalink / raw)
To: Eric Sandeen; +Cc: kernel list, Tyler Hicks, Al Viro, Christoph Hellwig
On Sat, Apr 24, 2010 at 5:41 AM, Eric Sandeen <sandeen@redhat.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.
> Based on a patch originally, I believe, from Mike Halcrow.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ecryptfs: disallow ecryptfs as underlying filesystem
2010-04-24 8:54 ` Pekka Enberg
@ 2010-04-24 14:39 ` Eric Sandeen
2010-04-27 15:56 ` Tyler Hicks
0 siblings, 1 reply; 4+ messages in thread
From: Eric Sandeen @ 2010-04-24 14:39 UTC (permalink / raw)
To: Pekka Enberg; +Cc: kernel list, Tyler Hicks, Al Viro, Christoph Hellwig
Pekka Enberg wrote:
> On Sat, Apr 24, 2010 at 5:41 AM, Eric Sandeen <sandeen@redhat.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@redhat.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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ecryptfs: disallow ecryptfs as underlying filesystem
2010-04-24 14:39 ` Eric Sandeen
@ 2010-04-27 15:56 ` Tyler Hicks
0 siblings, 0 replies; 4+ messages in thread
From: Tyler Hicks @ 2010-04-27 15:56 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Pekka Enberg, kernel list, Al Viro, Christoph Hellwig
On 04/24/2010 09:39 AM, Eric Sandeen wrote:
> Pekka Enberg wrote:
>> On Sat, Apr 24, 2010 at 5:41 AM, Eric Sandeen <sandeen@redhat.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
>
The xattr point is a good one. Also, two rounds of file name encryption
would result in a lot of ENAMETOOLONG errors. You're right about
eCryptfs on eCryptfs not being a design goal at this point. Thanks for
the patch!
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/ecryptfs/ecryptfs-2.6.git#next
Tyler
>>> Based on a patch originally, I believe, from Mike Halcrow.
>>>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.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@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-04-27 15:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-04-24 2:41 [PATCH] ecryptfs: disallow ecryptfs as underlying filesystem Eric Sandeen
2010-04-24 8:54 ` Pekka Enberg
2010-04-24 14:39 ` Eric Sandeen
2010-04-27 15:56 ` Tyler Hicks
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox