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 S1753157AbbESSFC (ORCPT <rfc822;w@1wt.eu>);
	Tue, 19 May 2015 14:05:02 -0400
Received: from mail-la0-f41.google.com ([209.85.215.41]:35954 "EHLO
	mail-la0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751132AbbESSFA (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 19 May 2015 14:05:00 -0400
MIME-Version: 1.0
In-Reply-To: <1432005006-3428-1-git-send-email-calvinowens@fb.com>
References: <20150214204009.GA1763278@mail.thefacebook.com> <1432005006-3428-1-git-send-email-calvinowens@fb.com>
From: Andy Lutomirski <luto@amacapital.net>
Date: Tue, 19 May 2015 11:04:37 -0700
Message-ID: <CALCETrXoidupgs+DyzJ4ibS9jmvNOn4DerVvtDgck1xk_2eaFw@mail.gmail.com>
Subject: Re: [PATCH v5] procfs: Always expose /proc/<pid>/map_files/ and make
 it readable
To: Calvin Owens <calvinowens@fb.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Al Viro <viro@zeniv.linux.org.uk>, Miklos Szeredi <miklos@szeredi.hu>,
        Zefan Li <lizefan@huawei.com>, Oleg Nesterov <oleg@redhat.com>,
        Joe Perches <joe@perches.com>, David Howells <dhowells@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        kernel-team@fb.com, Kees Cook <keescook@chromium.org>,
        "Kirill A. Shutemov" <kirill@shutemov.name>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, May 18, 2015 at 8:10 PM, Calvin Owens <calvinowens@fb.com> wrote:
> Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and
> is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is
> very useful for enumerating the files mapped into a process when the
> more verbose information in /proc/<pid>/maps is not needed. It also
> allows access to file descriptors for files that have been deleted and
> closed but are still mmapped into a process, which can be very useful
> for introspection and debugging.
>
> This patch moves the folder out from behind CHECKPOINT_RESTORE, and

I'm fine with this.

> removes the CAP_SYS_ADMIN restrictions. With that change alone,
> following the links would have required PTRACE_MODE_READ like the
> links in /proc/<pid>/fd/*.

I'm still not at all convinced that this is safe.  Here are a few ways
that it could have unintended consequences:

1. Mmap a dma-buf and then open /proc/self/map_files/addr.  You get an
fd pointing at a different inode than you mapped.  (kdbus would have
the same problem if it were merged.)

2. Open a file with O_RDONLY, mmap it with PROT_READ, close the file,
then open /proc/self/map_files/addr with O_RDWR.  I don't see anything
preventing that from succeeding.

3. Open a file, mmap it, close the fd, chroot, drop privileges, open
/proc/self/map_files/addr, then call ftruncate.

So NAK as-is, I think.

Fixing #1 would involve changing the way mmap works, I think.  Fixing
#2 would require similar infrastructure to what we'd need to fix the
existing /proc/pid/fd mode holes.  I have no clue how to even approach
fixing #3.

What's the use case of this patch?

--Andy