| Message ID | f1c6c3cc0c8850e143594afd57b50d30cd8bb5f8.1532043059.git.theuns.verwoerd@alliedtelesis.co.nz |
|---|---|
| State | Not Applicable |
| Delegated to: | David Woodhouse |
| Headers | show |
| Series | Secure deletion under JFFS2 | expand |
On Fri, Jul 20, 2018 at 11:50:12AM +1200, Theuns Verwoerd wrote: > +ssize_t jffs2_sync_file_read(struct file *f, > + char __user *b, size_t len, loff_t *ofs) > +{ > + struct jffs2_sb_info *c = file_inode(f)->i_private; > + > + while (c->tidemark) > + schedule(); > + > + return 0; > +} Brilliant. So when that gets called with c->tidemark being true and need_resched() - false, we shall... Bonus question: what happens if that is called after that jffs2_sb_info gets freed? -- It Doesn't Need To Make Sense - It's For Security Purposes.
On 07/20/2018 12:04 PM, Al Viro wrote: > On Fri, Jul 20, 2018 at 11:50:12AM +1200, Theuns Verwoerd wrote: > >> +ssize_t jffs2_sync_file_read(struct file *f, >> + char __user *b, size_t len, loff_t *ofs) >> +{ >> + struct jffs2_sb_info *c = file_inode(f)->i_private; >> + >> + while (c->tidemark) >> + schedule(); >> + >> + return 0; >> +} > Brilliant. So when that gets called with c->tidemark being true and > need_resched() - false, we shall... By my reading, schedule() will happily force a reschedule (once) when need_resched() is false. Every time the process is granted a time slice, it'll check the condition and surrender if not ready. What do you believe will happen? > Bonus question: what happens if that is called after that jffs2_sb_info > gets freed? That is probably a valid critique - if jffs2_kill_sb() gets called the cleanup in exit_jffs2_fs() is not sufficient. Regards, Theuns KRN
[fixing CC list] On Fri, Jul 20, 2018 at 2:54 AM, Theuns Verwoerd <Theuns.Verwoerd@alliedtelesis.co.nz> wrote: > > On 07/20/2018 12:04 PM, Al Viro wrote: >> On Fri, Jul 20, 2018 at 11:50:12AM +1200, Theuns Verwoerd wrote: >> >>> +ssize_t jffs2_sync_file_read(struct file *f, >>> + char __user *b, size_t len, loff_t *ofs) >>> +{ >>> + struct jffs2_sb_info *c = file_inode(f)->i_private; >>> + >>> + while (c->tidemark) >>> + schedule(); >>> + >>> + return 0; >>> +} >> Brilliant. So when that gets called with c->tidemark being true and >> need_resched() - false, we shall... > By my reading, schedule() will happily force a reschedule (once) when > need_resched() is false. Every time the process is granted a time > slice, it'll check the condition and surrender if not ready. > > What do you believe will happen? >> Bonus question: what happens if that is called after that jffs2_sb_info >> gets freed? > That is probably a valid critique - if jffs2_kill_sb() gets called the > cleanup in exit_jffs2_fs() is not sufficient. Beside of that, IMHO this debugfs file is a gross hack. Did you look into the possibility to add the GC phase into the unlink code?
On Sun, Jul 22, 2018 at 03:17:07PM +0200, Richard Weinberger wrote: > On Fri, Jul 20, 2018 at 2:54 AM, Theuns Verwoerd > <Theuns.Verwoerd@alliedtelesis.co.nz> wrote: > > > > On 07/20/2018 12:04 PM, Al Viro wrote: > >> On Fri, Jul 20, 2018 at 11:50:12AM +1200, Theuns Verwoerd wrote: > >> > >>> +ssize_t jffs2_sync_file_read(struct file *f, > >>> + char __user *b, size_t len, loff_t *ofs) > >>> +{ > >>> + struct jffs2_sb_info *c = file_inode(f)->i_private; > >>> + > >>> + while (c->tidemark) > >>> + schedule(); > >>> + > >>> + return 0; > >>> +} > >> Brilliant. So when that gets called with c->tidemark being true and > >> need_resched() - false, we shall... > > By my reading, schedule() will happily force a reschedule (once) when > > need_resched() is false. Every time the process is granted a time > > slice, it'll check the condition and surrender if not ready. Except that the task remains runnable (and not using its timeslice up). It won't sleep; there's nothing to drive it into TASK_NOT_RUNNING state. Try it and watch what's going on when you trigger that loop. That's really not a good way to wait for something to happen.
On 07/23/2018 01:17 AM, Richard Weinberger wrote: > Beside of that, IMHO this debugfs file is a gross hack. > Did you look into the possibility to add the GC phase into the unlink code? I'm not terribly keen on it either, but it's the least bad option I could think of. The main issue is that this kind of forced GC can be very slow (typically 2min on my test platform), and is not necessary for the vast majority of unlink cases - we only need to securely delete keys, other files can be cleaned up eventually. Unlink calls have no way of identifying such sensitive files, however, nor is there a way of demarcating a series of back-to-back secure deletions that can be collected en masse. Parallel to the problem of triggering the forced GC, is the issue of knowing when it has completed: blocking on file read seemed to be the most digestible to userspace way of doing that. Alternatives I've considered include overwrite-before-unlink (the block history gets complicated, plus the problem of actually doing such an overwrite), automatic GC on unlink (see above), an alternative system call for secure unlink (that messes up the entire FS call chain), and encrypting the sensitive files (key to access keys must also be persistent, which gets us back to the same problem). In my original version I'd used a /proc file, but debugfs has a more forgiving social contract. Regards, Theuns KRN
diff --git a/fs/jffs2/Kconfig b/fs/jffs2/Kconfig index ad850c5bf2ca..191272729f06 100644 --- a/fs/jffs2/Kconfig +++ b/fs/jffs2/Kconfig @@ -96,6 +96,14 @@ config JFFS2_FS_SECURITY If you are not using a security module that requires using extended attributes for file security labels, say N. +config JFFS2_FS_SYNC + bool "JFFS2 jffs2_sync file support" + depends on JFFS2_FS + help + This enables creation of jffs2_sync files for tracking + POLL forced garbage collection. It is used as part of FIPS + secure deletion support. + config JFFS2_COMPRESSION_OPTIONS bool "Advanced compression options for JFFS2" depends on JFFS2_FS diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c index 87bdf0f4cba1..264f68ab2e91 100644 --- a/fs/jffs2/super.c +++ b/fs/jffs2/super.c @@ -27,6 +27,7 @@ #include <linux/namei.h> #include <linux/seq_file.h> #include <linux/exportfs.h> +#include <linux/debugfs.h> #include "compr.h" #include "nodelist.h" @@ -34,6 +35,26 @@ static void jffs2_put_super(struct super_block *); static struct kmem_cache *jffs2_inode_cachep; +#ifdef CONFIG_JFFS2_FS_SYNC +#define JFFS2_FS_SYNC_NAME "jffs2_sync" +static struct dentry *jffs2_debugfs_dir; + +ssize_t jffs2_sync_file_read(struct file *f, + char __user *b, size_t len, loff_t *ofs) +{ + struct jffs2_sb_info *c = file_inode(f)->i_private; + + while (c->tidemark) + schedule(); + + return 0; +} +const struct file_operations jffs2_sync_ops = { + .owner = THIS_MODULE, + .read = jffs2_sync_file_read, +}; +#endif + static struct inode *jffs2_alloc_inode(struct super_block *sb) { struct jffs2_inode_info *f; @@ -307,6 +328,17 @@ static int jffs2_fill_super(struct super_block *sb, void *data, int silent) sb->s_flags |= SB_POSIXACL; #endif ret = jffs2_do_fill_super(sb, data, silent); + +#ifdef CONFIG_JFFS2_FS_SYNC + if (jffs2_debugfs_dir) { + char fname[128]; + + snprintf(fname, sizeof(fname), "%s_%s", + JFFS2_FS_SYNC_NAME, sb->s_mtd->name); + debugfs_create_file(fname, 0444, + jffs2_debugfs_dir, c, &jffs2_sync_ops); + } +#endif return ret; } @@ -405,6 +437,11 @@ static int __init init_jffs2_fs(void) pr_err("error: Failed to register filesystem\n"); goto out_slab; } + +#ifdef CONFIG_JFFS2_FS_SYNC + jffs2_debugfs_dir = debugfs_create_dir("jffs2", NULL); +#endif + return 0; out_slab: @@ -419,6 +456,9 @@ static int __init init_jffs2_fs(void) static void __exit exit_jffs2_fs(void) { unregister_filesystem(&jffs2_fs_type); +#ifdef CONFIG_JFFS2_FS_SYNC + debugfs_remove_recursive(jffs2_debugfs_dir); +#endif jffs2_destroy_slab_caches(); jffs2_compressors_exit();
When executing secure deletion under JFFS2 via the -POLL forced cleanup option, userspace may need to know when the process has safely completed. Provide debugfs files per mount that, when read, blocks while cleanup is in progress: once the read completes it is safe to assume that all obsoleted sensitive information has been erased. If CONFIG_JFFS2_FS_SYNC is configured, create /sys/kernel/debug/jffs2_sync_<fs name> files that block on read while forced gc is in progress. Signed-off-by: Theuns Verwoerd <theuns.verwoerd@alliedtelesis.co.nz> --- fs/jffs2/Kconfig | 8 ++++++++ fs/jffs2/super.c | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)