ecryptfs: disallow ecryptfs as underlying filesystem
PATCH 1/1 ecryptfs: disallow ecryptfs as underlying filesystem
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.
---
fs/ecryptfs/main.c +23 -5
@@ -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; }
Quoted 10 lines · Eric Sandeen
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.
Quoted 69 lines
Based on a patch originally, I believe, from Mike Halcrow.
Signed-off-by Eric Sandeen <[email protected]>---
fs/ecryptfs/main.c +21
-5@@ -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 byget_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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 9 lines · Pekka Enberg
Quoted 8 lines · Eric Sandeen
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]...
Quoted 5 lines
Quoted 2 lines
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
Quoted 69 lines
Quoted 69 lines
Based on a patch originally, I believe, from Mike Halcrow.
Signed-off-by Eric Sandeen <[email protected]>---
fs/ecryptfs/main.c +23
-6@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 16 lines · Eric Sandeen
Quoted 9 lines · Pekka Enberg
Quoted 8 lines · Eric Sandeen
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]...
Quoted 5 lines
Quoted 2 lines
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.
Quoted 9 lines
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 togit://git.kernel.org/pub/scm/linux/kernel/git/ecryptfs/ecryptfs-2.6.git#next
Tyler
Quoted 70 lines
Quoted 69 lines
Quoted 69 lines
Based on a patch originally, I believe, from Mike Halcrow.
Signed-off-by Eric Sandeen <[email protected]>---
fs/ecryptfs/main.c +23
-6@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/