[2/2] jffs2: Provide jffs2_sync files to track gc POLL progress

Message ID f1c6c3cc0c8850e143594afd57b50d30cd8bb5f8.1532043059.git.theuns.verwoerd@alliedtelesis.co.nz
State New
Delegated to: David Woodhouse
Headers show
Series
  • Secure deletion under JFFS2
Related show

Commit Message

Theuns Verwoerd July 19, 2018, 11:50 p.m.
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(+)

Comments

Al Viro July 20, 2018, 12:04 a.m. | #1
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.
Theuns Verwoerd July 20, 2018, 12:54 a.m. | #2
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
Richard Weinberger July 22, 2018, 1:17 p.m. | #3
[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?
Al Viro July 22, 2018, 1:49 p.m. | #4
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.
Theuns Verwoerd July 22, 2018, 10:22 p.m. | #5
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

Patch

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();