From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S932483AbcHIQaB (ORCPT <rfc822;w@1wt.eu>);
	Tue, 9 Aug 2016 12:30:01 -0400
Received: from mx1.redhat.com ([209.132.183.28]:36698 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S932413AbcHIQaA (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 9 Aug 2016 12:30:00 -0400
Date: Tue, 9 Aug 2016 18:29:47 +0200
From: Mateusz Guzik <mguzik@redhat.com>
To: robert.foss@collabora.com
Cc: akpm@linux-foundation.org, keescook@chromium.org, viro@zeniv.linux.org.uk,
        gorcunov@openvz.org, john.stultz@linaro.org,
        plaguedbypenguins@gmail.com, sonnyrao@chromium.org,
        adobriyan@gmail.com, jdanis@google.com, calvinowens@fb.com,
        jann@thejh.net, mhocko@suse.com, koct9i@gmail.com, vbabka@suse.cz,
        n-horiguchi@ah.jp.nec.com, kirill.shutemov@linux.intel.com,
        ldufour@linux.vnet.ibm.com, hannes@cmpxchg.org,
        linux-kernel@vger.kernel.org, Ben Zhang <benzh@chromium.org>,
        Bryan Freed <bfreed@chromium.org>,
        Filipe Brandenburger <filbranden@chromium.org>
Subject: Re: [PACTH v1] mm, proc: Implement /proc/<pid>/totmaps
Message-ID: <20160809162946.gznxgsgfzndinkay@mguzik>
References: <1470758743-17685-1-git-send-email-robert.foss@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1470758743-17685-1-git-send-email-robert.foss@collabora.com>
User-Agent: Mutt/1.6.0.1 (2016-04-01)
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 09 Aug 2016 16:29:59 +0000 (UTC)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Aug 09, 2016 at 12:05:43PM -0400, robert.foss@collabora.com wrote:
> From: Sonny Rao <sonnyrao@chromium.org>
> 
> This is based on earlier work by Thiago Goncales. It implements a new
> per process proc file which summarizes the contents of the smaps file
> but doesn't display any addresses.  It gives more detailed information
> than statm like the PSS (proprotional set size).  It differs from the
> original implementation in that it doesn't use the full blown set of
> seq operations, uses a different termination condition, and doesn't
> displayed "Locked" as that was broken on the original implemenation.
> 
> This new proc file provides information faster than parsing the potentially
> huge smaps file.

I have no idea about usefulness of this.

The patch is definitely buggy with respect to how it implements actual
access to mm.

> +static int totmaps_proc_show(struct seq_file *m, void *data)
> +{
> +	struct proc_maps_private *priv = m->private;
> +	struct mm_struct *mm;
> +	struct vm_area_struct *vma;
> +	struct mem_size_stats *mss_sum = priv->mss;
> +
> +	/* reference to priv->task already taken */
> +	/* but need to get the mm here because */
> +	/* task could be in the process of exiting */
> +	mm = get_task_mm(priv->task);
> +	if (!mm || IS_ERR(mm))
> +		return -EINVAL;
> +

That's not how it's done in smaps.

> +static int totmaps_open(struct inode *inode, struct file *file)
> +{
> +	struct proc_maps_private *priv;
> +	int ret = -ENOMEM;
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (priv) {
> +		priv->mss = kzalloc(sizeof(*priv->mss), GFP_KERNEL);
> +		if (!priv->mss)
> +			return -ENOMEM;

Cases below explicitly kfree(priv). I can't remember whether the close
routine gets called if this one fails. Either way, something is wrong
here.

> +
> +		/* we need to grab references to the task_struct */
> +		/* at open time, because there's a potential information */
> +		/* leak where the totmaps file is opened and held open */
> +		/* while the underlying pid to task mapping changes */
> +		/* underneath it */
> +		priv->task = get_pid_task(proc_pid(inode), PIDTYPE_PID);

This performs no permission checks that I would see. If you take a look
at smaps you will see the user ends up in proc_maps_open which performs
proc_mem_open(inode, PTRACE_MODE_READ) and gets a mm from there.


> +		if (!priv->task) {
> +			kfree(priv->mss);
> +			kfree(priv);
> +			return -ESRCH;
> +		}
> +
> +		ret = single_open(file, totmaps_proc_show, priv);
> +		if (ret) {
> +			put_task_struct(priv->task);
> +			kfree(priv->mss);
> +			kfree(priv);
> +		}
> +	}
> +	return ret;
> +}
> +

-- 
Mateusz Guzik