Messages in this thread | | | Subject | Re: [PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps | From | Robert Foss <> | Date | Mon, 15 Aug 2016 16:14:41 -0400 |
| |
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@collabora.com >> 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.
| |