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 S1752648AbcHMOkL (ORCPT <rfc822;w@1wt.eu>);
	Sat, 13 Aug 2016 10:40:11 -0400
Received: from thejh.net ([37.221.195.125]:50847 "EHLO thejh.net"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752398AbcHMOkJ (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Sat, 13 Aug 2016 10:40:09 -0400
Date: Sat, 13 Aug 2016 16:39:44 +0200
From: Jann Horn <jann@thejh.net>
To: robert.foss@collabora.com
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
Message-ID: <20160813143944.GA22441@pc.thejh.net>
References: <1471039462-16771-1-git-send-email-robert.foss@collabora.com>
 <1471039462-16771-2-git-send-email-robert.foss@collabora.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature"; boundary="EeQfGwPcQSOJBaQU"
Content-Disposition: inline
In-Reply-To: <1471039462-16771-2-git-send-email-robert.foss@collabora.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


--EeQfGwPcQSOJBaQU
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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?


> 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, stru=
ct file *file)
>  	struct seq_file *seq =3D file->private_data;
>  	struct proc_maps_private *priv =3D seq->private;
> =20
> +	if (!priv)
> +		return 0;
> +

You might want to get rid of this, see below.


> +static int totmaps_open(struct inode *inode, struct file *file)
> +{
> +	struct proc_maps_private *priv =3D NULL;
> +	struct seq_file *seq;
> +	int ret;
> +
> +	ret =3D 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 =3D get_proc_task(inode);
> +	if (!priv->task) {
> +		ret =3D -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 =3D {
[...]
> +	.release	=3D 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).

--EeQfGwPcQSOJBaQU
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJXrzEwAAoJED4KNFJOeCOoHv8P/0/zNQJ7UNjZo970j9N02pAR
hVtDeLzRgYMzattcAh2I9fOQNlL+7THuH8l0hhrzNPsxVhpEF+8q+RlPwH6egW+e
zEy/DHiEMWINNHABITHLFyk0qSYh43n7p66epsmW3W4MibkqvBPbR7muKb9dDAyR
Qi12tn2sXS6gNp8I2HH/1twzxpofkZ30NITQbKM+yyomxse5pDqGcwurAOF2q23Z
+fsYN+n8nsPd2zSbgVSb88tru6m7XNaxIUjvE4lY/DxgPCJ7k87oSlnr4MF31WKL
e77LFS4M8NB8WrmeZzvM4nhKbUr8s2WqlQvnxmy2uUiVc9sEnP4xPumNzN56zhwz
4kxYnSFgcXg1yvgXs0GYEwgHIWKn13bwvNsg4s9gX18GtBrQy6eiQ7DIQKR4boUX
oZZYO0z9uZiedvULPb5RmF6bLGl0ZEzi5hJ+8txfxAaAsJpd5I9WXBYKOiJrMKSp
zJpBRkg7XV3BK6XZjhCobsego8u1Z/Htfl4ONgpXzEUzWQo1ywH3D/KYUaULLVYF
vRG9W5lpi0VsCUZ1nhu+iXNWhY4SYAM016GqSCJdz39BQ7hhlG+I3LqM2SKx2r9a
Cs9kTf9lBnvfMn82bLux8ob6z6xX6wIns4gLg/OVpyid3mnByMenYhC0A8L6yjbr
Q2cs3h8oTsN1k+01By8P
=bPHI
-----END PGP SIGNATURE-----

--EeQfGwPcQSOJBaQU--