| 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
| ||
|
Date: Wed, 22 Nov 2017 13:02:09 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Jan Kara <jack@...e.cz>, Dan Williams <dan.j.williams@...el.com>
Cc: Ross Zwisler <ross.zwisler@...ux.intel.com>,
Christoph Hellwig <hch@...radead.org>,
linux-fsdevel@...r.kernel.org, linux-nvdimm@...ts.01.org,
linux-mm@...ck.org, linux-api@...r.kernel.org,
linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org,
"Darrick J . Wong" <darrick.wong@...cle.com>,
Arnd Bergmann <arnd@...db.de>,
Andy Lutomirski <luto@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Michal Hocko <mhocko@...nel.org>,
Matthew Wilcox <willy@...radead.org>,
Kees Cook <keescook@...omium.org>,
Florian Weimer <fweimer@...hat.com>,
John Hubbard <jhubbard@...dia.com>,
"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: [PATCH 01/18] mm: introduce MAP_SHARED_VALIDATE, a mechanism to
safely define new mmap flags
On 11/01/2017 04:36 PM, Jan Kara 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.
So I'm trying to make sense of this together with Michal's attempt for
MAP_FIXED_SAFE [1] where he has to introduce a completely new flag
instead of flag modifier exactly for the reason of not validating
unknown flags. And my conclusion is that because MAP_SHARED_VALIDATE
implies MAP_SHARED and excludes MAP_PRIVATE, MAP_FIXED_SAFE as a
modifier cannot build on top of this. Wouldn't thus it be really better
long-term to introduce mmap3 at this point? ...
Vlastimil
[1] https://marc.info/?l=linux-arch&m=151083448322336&w=2
> 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>
> Reviewed-by: Ross Zwisler <ross.zwisler@...ux.intel.com>
> 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 | 15 +++++++++++
> tools/include/uapi/asm-generic/mman-common.h | 1 +
> 9 files changed, 61 insertions(+)
>
> diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
> index 3b26cc62dadb..f6d118aaedb9 100644
> --- a/arch/alpha/include/uapi/asm/mman.h
> +++ b/arch/alpha/include/uapi/asm/mman.h
> @@ -11,6 +11,7 @@
>
> #define MAP_SHARED 0x01 /* Share changes */
> #define MAP_PRIVATE 0x02 /* Changes are private */
> +#define MAP_SHARED_VALIDATE 0x03 /* share + validate extension flags */
> #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 */
> diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h
> index da3216007fe0..93268e4cd3c7 100644
> --- a/arch/mips/include/uapi/asm/mman.h
> +++ b/arch/mips/include/uapi/asm/mman.h
> @@ -28,6 +28,7 @@
> */
> #define MAP_SHARED 0x001 /* Share changes */
> #define MAP_PRIVATE 0x002 /* Changes are private */
> +#define MAP_SHARED_VALIDATE 0x003 /* share + validate extension flags */
> #define MAP_TYPE 0x00f /* Mask for type of mapping */
> #define MAP_FIXED 0x010 /* Interpret addr exactly */
>
> diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h
> index 775b5d5e41a1..bca652aa1677 100644
> --- a/arch/parisc/include/uapi/asm/mman.h
> +++ b/arch/parisc/include/uapi/asm/mman.h
> @@ -11,6 +11,7 @@
>
> #define MAP_SHARED 0x01 /* Share changes */
> #define MAP_PRIVATE 0x02 /* Changes are private */
> +#define MAP_SHARED_VALIDATE 0x03 /* share + validate extension flags */
> #define MAP_TYPE 0x03 /* Mask for type of mapping */
> #define MAP_FIXED 0x04 /* Interpret addr exactly */
> #define MAP_ANONYMOUS 0x10 /* don't use a file */
> diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h
> index b15b278aa314..9ab426374714 100644
> --- a/arch/xtensa/include/uapi/asm/mman.h
> +++ b/arch/xtensa/include/uapi/asm/mman.h
> @@ -35,6 +35,7 @@
> */
> #define MAP_SHARED 0x001 /* Share changes */
> #define MAP_PRIVATE 0x002 /* Changes are private */
> +#define MAP_SHARED_VALIDATE 0x003 /* share + validate extension flags */
> #define MAP_TYPE 0x00f /* Mask for type of mapping */
> #define MAP_FIXED 0x010 /* Interpret addr exactly */
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 13dab191a23e..57added3201d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1701,6 +1701,7 @@ struct file_operations {
> long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
> long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
> int (*mmap) (struct file *, struct vm_area_struct *);
> + unsigned long mmap_supported_flags;
> int (*open) (struct inode *, struct file *);
> int (*flush) (struct file *, fl_owner_t id);
> int (*release) (struct inode *, struct file *);
> diff --git a/include/linux/mman.h b/include/linux/mman.h
> index c8367041fafd..94b63b4d71ff 100644
> --- a/include/linux/mman.h
> +++ b/include/linux/mman.h
> @@ -7,6 +7,45 @@
> #include <linux/atomic.h>
> #include <uapi/linux/mman.h>
>
> +/*
> + * Arrange for legacy / undefined architecture specific flags to be
> + * ignored by default in LEGACY_MAP_MASK.
> + */
> +#ifndef MAP_32BIT
> +#define MAP_32BIT 0
> +#endif
> +#ifndef MAP_HUGE_2MB
> +#define MAP_HUGE_2MB 0
> +#endif
> +#ifndef MAP_HUGE_1GB
> +#define MAP_HUGE_1GB 0
> +#endif
> +#ifndef MAP_UNINITIALIZED
> +#define MAP_UNINITIALIZED 0
> +#endif
> +
> +/*
> + * The historical set of flags that all mmap implementations implicitly
> + * support when a ->mmap_validate() op is not provided in file_operations.
> + */
> +#define LEGACY_MAP_MASK (MAP_SHARED \
> + | MAP_PRIVATE \
> + | MAP_FIXED \
> + | MAP_ANONYMOUS \
> + | MAP_DENYWRITE \
> + | MAP_EXECUTABLE \
> + | MAP_UNINITIALIZED \
> + | MAP_GROWSDOWN \
> + | MAP_LOCKED \
> + | MAP_NORESERVE \
> + | MAP_POPULATE \
> + | MAP_NONBLOCK \
> + | MAP_STACK \
> + | MAP_HUGETLB \
> + | MAP_32BIT \
> + | MAP_HUGE_2MB \
> + | MAP_HUGE_1GB)
> +
> extern int sysctl_overcommit_memory;
> extern int sysctl_overcommit_ratio;
> extern unsigned long sysctl_overcommit_kbytes;
> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> index 203268f9231e..8ce7f5a0800f 100644
> --- a/include/uapi/asm-generic/mman-common.h
> +++ b/include/uapi/asm-generic/mman-common.h
> @@ -16,6 +16,7 @@
>
> #define MAP_SHARED 0x01 /* Share changes */
> #define MAP_PRIVATE 0x02 /* Changes are private */
> +#define MAP_SHARED_VALIDATE 0x03 /* share + validate extension flags */
> #define MAP_TYPE 0x0f /* Mask for type of mapping */
> #define MAP_FIXED 0x10 /* Interpret addr exactly */
> #define MAP_ANONYMOUS 0x20 /* don't use a file */
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 680506faceae..924839fac0e6 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1387,9 +1387,24 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>
> if (file) {
> struct inode *inode = file_inode(file);
> + unsigned long flags_mask;
> +
> + flags_mask = LEGACY_MAP_MASK | file->f_op->mmap_supported_flags;
>
> switch (flags & MAP_TYPE) {
> case MAP_SHARED:
> + /*
> + * Force use of MAP_SHARED_VALIDATE with non-legacy
> + * flags. E.g. MAP_SYNC is dangerous to use with
> + * MAP_SHARED as you don't know which consistency model
> + * you will get. We silently ignore unsupported flags
> + * with MAP_SHARED to preserve backward compatibility.
> + */
> + flags &= LEGACY_MAP_MASK;
> + /* fall through */
> + case MAP_SHARED_VALIDATE:
> + if (flags & ~flags_mask)
> + return -EOPNOTSUPP;
> if ((prot&PROT_WRITE) && !(file->f_mode&FMODE_WRITE))
> return -EACCES;
>
> diff --git a/tools/include/uapi/asm-generic/mman-common.h b/tools/include/uapi/asm-generic/mman-common.h
> index 203268f9231e..8ce7f5a0800f 100644
> --- a/tools/include/uapi/asm-generic/mman-common.h
> +++ b/tools/include/uapi/asm-generic/mman-common.h
> @@ -16,6 +16,7 @@
>
> #define MAP_SHARED 0x01 /* Share changes */
> #define MAP_PRIVATE 0x02 /* Changes are private */
> +#define MAP_SHARED_VALIDATE 0x03 /* share + validate extension flags */
> #define MAP_TYPE 0x0f /* Mask for type of mapping */
> #define MAP_FIXED 0x10 /* Interpret addr exactly */
> #define MAP_ANONYMOUS 0x20 /* don't use a file */
>
Powered by blists - more mailing lists