On 2016-08-15 09:57 AM, Robert Foss wrote:
On 2016-08-13 10:39 AM, Jann Horn wrote:
On Fri, Aug 12, 2016 at 06:04:20PM -0400, robert.foss@xxxxxxxxxxxxx
wrote:
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index aa27810..c55e1fe 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -281,6 +281,7 @@ struct proc_maps_private {
struct mm_struct *mm;
#ifdef CONFIG_MMU
struct vm_area_struct *tail_vma;
+ struct mem_size_stats *mss;
This is unused now, right?
Fixing it in v3.
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 4648c7f..b7612e9 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -246,6 +246,9 @@ static int proc_map_release(struct inode *inode,
struct file *file)
struct seq_file *seq = file->private_data;
struct proc_maps_private *priv = seq->private;
+ if (!priv)
+ return 0;
+
You might want to get rid of this, see below.
Fixing it in v3.
+static int totmaps_open(struct inode *inode, struct file *file)
+{
+ struct proc_maps_private *priv = NULL;
+ struct seq_file *seq;
+ int ret;
+
+ ret = do_maps_open(inode, file, &proc_totmaps_op);
+ if (ret)
+ goto error;
[...]
+error:
+ proc_map_release(inode, file);
+ return ret;
I don't think this is correct. Have a look at the other callers of
do_maps_open() - none of them do any cleanup steps on error, they
just return. I think the "goto error" here should be a return
instead.
Have a look at the error cases that can cause do_maps_open() to
fail: do_maps_open() just calls proc_maps_open(). If the
__seq_open_private() call fails because of memory pressure,
file->private_data is still NULL, and your newly added NULL check
in proc_map_release() causes proc_map_release() to be a no-op
there. But if proc_maps_open() fails later on, things get nasty:
If, for example, proc_mem_open() fails because of a ptrace
permission denial, __seq_open_file -> seq_open has already set
file->private_data to a struct seq_file *, and then
proc_maps_open(), prior to passing on the error code, calls
seq_release_private -> seq_release, which frees that
struct seq_file * without NULLing the private_data pointer.
As far as I can tell, proc_map_release() would then run into
a use-after-free scenario.
+ priv->task = get_proc_task(inode);
+ if (!priv->task) {
+ ret = -ESRCH;
+ goto error;
+ }
You're not actually using ->task anywhere in the current version,
right? Can this be deleted?
Actually, priv->task is used by hold_task_mempolicy() in
totmaps_proc_show(), which as far as I understand it is needed due to
the "vma = mm->mmap" looping we do.
+const struct file_operations proc_totmaps_operations = {
[...]
+ .release = proc_map_release,
This won't release priv->task, causing a memory leak (exploitable
through a reference counter overflow of the task_struct usage
counter).
Thanks for the thorough walkthrough, it is much appreciated.
priv->task does not appear to be used any more, and can be removed.
When "priv->task = get_proc_task(inode)" is removed, totmaps_open()
starts to look just like the other XXX_open functions.
I'll send out v3 as soon as testing has been done.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
