linux-ext4 - Re: [PATCH 01/17] mm: introduce MAP_SHARED_VALIDATE, a mechanism to safely define new mmap flags

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPcyv4jpHVUamGR=JuK4eUUfFDCWzVmiCMr7K-sTJ2U1TX8uzQ@mail.gmail.com>
Date:   Thu, 19 Oct 2017 09:48:58 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Jan Kara <jack@...e.cz>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        "linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        linux-ext4 <linux-ext4@...r.kernel.org>,
        linux-xfs@...r.kernel.org, Christoph Hellwig <hch@...radead.org>,
        Linux API <linux-api@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Andy Lutomirski <luto@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 01/17] mm: introduce MAP_SHARED_VALIDATE, a mechanism to
 safely define new mmap flags

On Thu, Oct 19, 2017 at 5:58 AM, Jan Kara <jack@...e.cz> wrote:
>
> From: Dan Williams <dan.j.williams@...el.com>
>
> The mmap(2) syscall suffers from the ABI anti-pattern of not validating
> unknown flags. However, proposals like MAP_SYNC need a mechanism to
> define new behavior that is known to fail on older kernels without the
> support. Define a new MAP_SHARED_VALIDATE flag pattern that is
> guaranteed to fail on all legacy mmap implementations.
>
> It is worth noting that the original proposal was for a standalone
> MAP_VALIDATE flag. However, when that  could not be supported by all
> archs Linus observed:
>
>     I see why you *think* you want a bitmap. You think you want
>     a bitmap because you want to make MAP_VALIDATE be part of MAP_SYNC
>     etc, so that people can do
>
>     ret = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED
>                     | MAP_SYNC, fd, 0);
>
>     and "know" that MAP_SYNC actually takes.
>
>     And I'm saying that whole wish is bogus. You're fundamentally
>     depending on special semantics, just make it explicit. It's already
>     not portable, so don't try to make it so.
>
>     Rename that MAP_VALIDATE as MAP_SHARED_VALIDATE, make it have a value
>     of 0x3, and make people do
>
>     ret = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED_VALIDATE
>                     | MAP_SYNC, fd, 0);
>
>     and then the kernel side is easier too (none of that random garbage
>     playing games with looking at the "MAP_VALIDATE bit", but just another
>     case statement in that map type thing.
>
>     Boom. Done.
>
> Similar to ->fallocate() we also want the ability to validate the
> support for new flags on a per ->mmap() 'struct file_operations'
> instance basis.  Towards that end arrange for flags to be generically
> validated against a mmap_supported_flags exported by 'struct
> file_operations'. By default all existing flags are implicitly
> supported, but new flags require MAP_SHARED_VALIDATE and
> per-instance-opt-in.
>
> Cc: Jan Kara <jack@...e.cz>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Andy Lutomirski <luto@...nel.org>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Suggested-by: Christoph Hellwig <hch@....de>
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> Signed-off-by: Jan Kara <jack@...e.cz>
> ---
>  arch/alpha/include/uapi/asm/mman.h           |  1 +
>  arch/mips/include/uapi/asm/mman.h            |  1 +
>  arch/parisc/include/uapi/asm/mman.h          |  1 +
>  arch/xtensa/include/uapi/asm/mman.h          |  1 +
>  include/linux/fs.h                           |  1 +
>  include/linux/mman.h                         | 39 ++++++++++++++++++++++++++++
>  include/uapi/asm-generic/mman-common.h       |  1 +
>  mm/mmap.c                                    | 21 +++++++++++++++
>  tools/include/uapi/asm-generic/mman-common.h |  1 +
>  9 files changed, 67 insertions(+)
>
> diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
> index 3b26cc62dadb..92823f24890b 100644
> --- a/arch/alpha/include/uapi/asm/mman.h
> +++ b/arch/alpha/include/uapi/asm/mman.h
> @@ -14,6 +14,7 @@
>  #define MAP_TYPE       0x0f            /* Mask for type of mapping (OSF/1 is _wrong_) */
>  #define MAP_FIXED      0x100           /* Interpret addr exactly */
>  #define MAP_ANONYMOUS  0x10            /* don't use a file */
> +#define MAP_SHARED_VALIDATE 0x3                /* share + validate extension flags */

Looks good, only comment is your original feedback to move this
definition next to MAP_SHARED a few lines up in the all places where
we define it.