From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752985AbcHON5u (ORCPT ); Mon, 15 Aug 2016 09:57:50 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:50634 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752824AbcHON5p (ORCPT ); Mon, 15 Aug 2016 09:57:45 -0400 Subject: Re: [PACTH v2 1/3] mm, proc: Implement /proc//totmaps To: Jann Horn References: <1471039462-16771-1-git-send-email-robert.foss@collabora.com> <1471039462-16771-2-git-send-email-robert.foss@collabora.com> <20160813143944.GA22441@pc.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 , Bryan Freed , Filipe Brandenburger , Mateusz Guzik From: Robert Foss Message-ID: Date: Mon, 15 Aug 2016 09:57:35 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160813143944.GA22441@pc.thejh.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.