lkml.org 
[lkml]   [2016]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps
From
Date


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?
>
>
>> +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.

\
 
 \ /
  Last update: 2016-09-17 09:56    [W:0.072 / U:0.308 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site