linux-kernel - Re: [PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <06a8093b-f037-85c0-ca59-43cc75ea2b0d@collabora.com>
Date:	Mon, 15 Aug 2016 16:14:41 -0400
From:	Robert Foss <robert.foss@...labora.com>
To:	Jann Horn <jann@...jh.net>
Cc:	corbet@....net, akpm@...ux-foundation.org, vbabka@...e.cz,
	koct9i@...il.com, mhocko@...e.com, hughd@...gle.com,
	n-horiguchi@...jp.nec.com, minchan@...nel.org,
	john.stultz@...aro.org, ross.zwisler@...ux.intel.com,
	jmarchan@...hat.com, hannes@...xchg.org, keescook@...omium.org,
	viro@...iv.linux.org.uk, gorcunov@...nvz.org,
	plaguedbypenguins@...il.com, rientjes@...gle.com,
	eric.engestrom@...tec.com, jdanis@...gle.com, calvinowens@...com,
	adobriyan@...il.com, sonnyrao@...omium.org,
	kirill.shutemov@...ux.intel.com, ldufour@...ux.vnet.ibm.com,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	Ben Zhang <benzh@...omium.org>,
	Bryan Freed <bfreed@...omium.org>,
	Filipe Brandenburger <filbranden@...omium.org>,
	Mateusz Guzik <mguzik@...hat.com>
Subject: Re: [PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps



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@...labora.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.