From: Robert Foss <robert.foss@collabora.com>
To: Jann Horn <jann@thejh.net>
Cc: corbet@lwn.net, akpm@linux-foundation.org, vbabka@suse.cz,
koct9i@gmail.com, mhocko@suse.com, hughd@google.com,
n-horiguchi@ah.jp.nec.com, minchan@kernel.org,
john.stultz@linaro.org, ross.zwisler@linux.intel.com,
jmarchan@redhat.com, hannes@cmpxchg.org, keescook@chromium.org,
viro@zeniv.linux.org.uk, gorcunov@openvz.org,
plaguedbypenguins@gmail.com, rientjes@google.com,
eric.engestrom@imgtec.com, jdanis@google.com, calvinowens@fb.com,
adobriyan@gmail.com, sonnyrao@chromium.org,
kirill.shutemov@linux.intel.com, ldufour@linux.vnet.ibm.com,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
Ben Zhang <benzh@chromium.org>, Bryan Freed <bfreed@chromium.org>,
Filipe Brandenburger <filbranden@chromium.org>,
Mateusz Guzik <mguzik@redhat.com>
Subject: Re: [PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps
Date: Mon, 15 Aug 2016 09:57:35 -0400 [thread overview]
Message-ID: <b08ea4cc-f6a1-0e11-526d-5c01984de9bc@collabora.com> (raw)
In-Reply-To: <20160813143944.GA22441@pc.thejh.net>
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.
next prev parent reply other threads:[~2016-08-15 13:57 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-12 22:04 [PACTH v2 0/3] Implement /proc/<pid>/totmaps robert.foss
2016-08-12 22:04 ` [PACTH v2 1/3] mm, proc: " robert.foss
2016-08-13 14:39 ` Jann Horn
2016-08-15 13:57 ` Robert Foss [this message]
2016-08-15 20:14 ` Robert Foss
2016-08-12 22:04 ` [PACTH v2 2/3] Documentation/filesystems: Fixed typo robert.foss
2016-08-12 22:04 ` [PACTH v2 3/3] Documentation/filesystems: Added /proc/PID/totmaps documentation robert.foss
2016-08-14 9:04 ` [PACTH v2 0/3] Implement /proc/<pid>/totmaps Michal Hocko
2016-08-15 13:00 ` Robert Foss
2016-08-15 13:42 ` Michal Hocko
2016-08-15 16:25 ` Robert Foss
2016-08-16 7:12 ` Michal Hocko
2016-08-16 16:46 ` Robert Foss
2016-08-17 8:22 ` Michal Hocko
2016-08-17 9:31 ` Jann Horn
2016-08-17 13:03 ` Michal Hocko
2016-08-17 16:48 ` Robert Foss
2016-08-17 18:57 ` Sonny Rao
2016-08-18 7:44 ` Michal Hocko
2016-08-18 17:47 ` Sonny Rao
2016-08-18 18:01 ` Michal Hocko
2016-08-18 21:05 ` Robert Foss
2016-08-19 6:27 ` Sonny Rao
2016-08-19 2:26 ` Minchan Kim
2016-08-19 6:47 ` Sonny Rao
2016-08-19 8:05 ` Michal Hocko
2016-08-19 18:20 ` Sonny Rao
2016-08-22 0:07 ` Minchan Kim
2016-08-22 7:40 ` Michal Hocko
2016-08-22 14:12 ` Minchan Kim
2016-08-22 14:37 ` Robert Foss
2016-08-22 16:45 ` Michal Hocko
2016-08-22 17:29 ` Michal Hocko
2016-08-22 17:47 ` Michal Hocko
2016-08-23 8:26 ` Michal Hocko
2016-08-23 14:33 ` utime accounting regression since 4.6 (was: Re: [PACTH v2 0/3] Implement /proc/<pid>/totmaps) Michal Hocko
2016-08-23 21:46 ` Rik van Riel
2016-08-24 16:56 ` Michal Hocko
2016-09-30 9:49 ` Michal Hocko
2016-09-30 12:35 ` Rik van Riel
2016-08-19 6:43 ` [PACTH v2 0/3] Implement /proc/<pid>/totmaps Sonny Rao
2016-08-19 7:59 ` Michal Hocko
2016-08-19 17:57 ` Sonny Rao
2016-08-22 7:54 ` Michal Hocko
2016-08-22 22:44 ` Sonny Rao
2016-08-24 10:14 ` Marcin Jabrzyk
2016-08-30 8:20 ` Michal Hocko
2016-08-29 14:37 ` Michal Hocko
2016-08-30 8:15 ` Michal Hocko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b08ea4cc-f6a1-0e11-526d-5c01984de9bc@collabora.com \
--to=robert.foss@collabora.com \
--cc=adobriyan@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=benzh@chromium.org \
--cc=bfreed@chromium.org \
--cc=calvinowens@fb.com \
--cc=corbet@lwn.net \
--cc=eric.engestrom@imgtec.com \
--cc=filbranden@chromium.org \
--cc=gorcunov@openvz.org \
--cc=hannes@cmpxchg.org \
--cc=hughd@google.com \
--cc=jann@thejh.net \
--cc=jdanis@google.com \
--cc=jmarchan@redhat.com \
--cc=john.stultz@linaro.org \
--cc=keescook@chromium.org \
--cc=kirill.shutemov@linux.intel.com \
--cc=koct9i@gmail.com \
--cc=ldufour@linux.vnet.ibm.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mguzik@redhat.com \
--cc=mhocko@suse.com \
--cc=minchan@kernel.org \
--cc=n-horiguchi@ah.jp.nec.com \
--cc=plaguedbypenguins@gmail.com \
--cc=rientjes@google.com \
--cc=ross.zwisler@linux.intel.com \
--cc=sonnyrao@chromium.org \
--cc=vbabka@suse.cz \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).