From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965491AbdKQIh2 (ORCPT ); Fri, 17 Nov 2017 03:37:28 -0500 Received: from hqemgate15.nvidia.com ([216.228.121.64]:7944 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751322AbdKQIhT (ORCPT ); Fri, 17 Nov 2017 03:37:19 -0500 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Fri, 17 Nov 2017 00:37:19 -0800 Subject: Re: [RFC PATCH 1/2] mm: introduce MAP_FIXED_SAFE To: Michal Hocko , CC: Khalid Aziz , Michael Ellerman , Andrew Morton , Russell King - ARM Linux , Andrea Arcangeli , , LKML , , Michal Hocko References: <20171116101900.13621-1-mhocko@kernel.org> <20171116101900.13621-2-mhocko@kernel.org> X-Nvconfidentiality: public From: John Hubbard Message-ID: Date: Fri, 17 Nov 2017 00:37:18 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171116101900.13621-2-mhocko@kernel.org> X-Originating-IP: [10.2.170.201] X-ClientProxiedBy: HQMAIL108.nvidia.com (172.18.146.13) To HQMAIL107.nvidia.com (172.20.187.13) Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/16/2017 02:18 AM, Michal Hocko wrote: > From: Michal Hocko > > MAP_FIXED is used quite often to enforce mapping at the particular > range. The main problem of this flag is, however, that it is inherently > dangerous because it unmaps existing mappings covered by the requested > range. This can cause silent memory corruptions. Some of them even with > serious security implications. While the current semantic might be > really desiderable in many cases there are others which would want to > enforce the given range but rather see a failure than a silent memory > corruption on a clashing range. Please note that there is no guarantee > that a given range is obeyed by the mmap even when it is free - e.g. > arch specific code is allowed to apply an alignment. > > Introduce a new MAP_FIXED_SAFE flag for mmap to achieve this behavior. > It has the same semantic as MAP_FIXED wrt. the given address request > with a single exception that it fails with ENOMEM if the requested > address is already covered by an existing mapping. We still do rely on > get_unmaped_area to handle all the arch specific MAP_FIXED treatment and > check for a conflicting vma after it returns. > > [set MAP_FIXED before round_hint_to_min as per Khalid Aziz] > Signed-off-by: Michal Hocko > --- > arch/alpha/include/uapi/asm/mman.h | 2 ++ > arch/mips/include/uapi/asm/mman.h | 2 ++ > arch/parisc/include/uapi/asm/mman.h | 2 ++ > arch/powerpc/include/uapi/asm/mman.h | 1 + > arch/sparc/include/uapi/asm/mman.h | 1 + > arch/tile/include/uapi/asm/mman.h | 1 + > arch/xtensa/include/uapi/asm/mman.h | 2 ++ > include/uapi/asm-generic/mman.h | 1 + > mm/mmap.c | 11 +++++++++++ > 9 files changed, 23 insertions(+) > > diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h > index 3b26cc62dadb..0e5724e4b4ad 100644 > --- a/arch/alpha/include/uapi/asm/mman.h > +++ b/arch/alpha/include/uapi/asm/mman.h > @@ -31,6 +31,8 @@ > #define MAP_STACK 0x80000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x100000 /* create a huge page mapping */ > > +#define MAP_FIXED_SAFE 0x200000 /* MAP_FIXED which doesn't unmap underlying mapping */ > + > #define MS_ASYNC 1 /* sync memory asynchronously */ > #define MS_SYNC 2 /* synchronous memory sync */ > #define MS_INVALIDATE 4 /* invalidate the caches */ > diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h > index da3216007fe0..fc5e61ef9fd4 100644 > --- a/arch/mips/include/uapi/asm/mman.h > +++ b/arch/mips/include/uapi/asm/mman.h > @@ -49,6 +49,8 @@ > #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ > > +#define MAP_FIXED_SAFE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */ > + > /* > * Flags for msync > */ > diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h > index cc9ba1d34779..c926487472fa 100644 > --- a/arch/parisc/include/uapi/asm/mman.h > +++ b/arch/parisc/include/uapi/asm/mman.h > @@ -25,6 +25,8 @@ > #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ > > +#define MAP_FIXED_SAFE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */ > + > #define MS_SYNC 1 /* synchronous memory sync */ > #define MS_ASYNC 2 /* sync memory asynchronously */ > #define MS_INVALIDATE 4 /* invalidate the caches */ > diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h > index 03c06ba7464f..d97342ca25b1 100644 > --- a/arch/powerpc/include/uapi/asm/mman.h > +++ b/arch/powerpc/include/uapi/asm/mman.h > @@ -28,5 +28,6 @@ > #define MAP_NONBLOCK 0x10000 /* do not block on IO */ > #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x800000 /* MAP_FIXED which doesn't unmap underlying mapping */ Hi Michal, 1. The powerpc change, above, has one too many zeroes. It should be 0x80000, not 0x800000. 2. For the one-line comments, if you phrase them like this: /* Like MAP_FIXED, except that it doesn't unmap pre-existing mappings */ ...I think that would be a little clearer. more below: > #endif /* _UAPI_ASM_POWERPC_MMAN_H */ > diff --git a/arch/sparc/include/uapi/asm/mman.h b/arch/sparc/include/uapi/asm/mman.h > index 9765896ecb2c..7b00477a7f9a 100644 > --- a/arch/sparc/include/uapi/asm/mman.h > +++ b/arch/sparc/include/uapi/asm/mman.h > @@ -23,6 +23,7 @@ > #define MAP_NONBLOCK 0x10000 /* do not block on IO */ > #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x80000 /* MAP_FIXED which doesn't unmap underlying mapping */ > > > #endif /* _UAPI__SPARC_MMAN_H__ */ > diff --git a/arch/tile/include/uapi/asm/mman.h b/arch/tile/include/uapi/asm/mman.h > index 63ee13faf17d..d5d58d2dc95e 100644 > --- a/arch/tile/include/uapi/asm/mman.h > +++ b/arch/tile/include/uapi/asm/mman.h > @@ -29,6 +29,7 @@ > #define MAP_DENYWRITE 0x0800 /* ETXTBSY */ > #define MAP_EXECUTABLE 0x1000 /* mark it as an executable */ > #define MAP_HUGETLB 0x4000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x8000 /* MAP_FIXED which doesn't unmap underlying mapping */ > > > /* > diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h > index b15b278aa314..d665bd8b7cbd 100644 > --- a/arch/xtensa/include/uapi/asm/mman.h > +++ b/arch/xtensa/include/uapi/asm/mman.h > @@ -55,6 +55,7 @@ > #define MAP_NONBLOCK 0x20000 /* do not block on IO */ > #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */ > #ifdef CONFIG_MMAP_ALLOW_UNINITIALIZED > # define MAP_UNINITIALIZED 0x4000000 /* For anonymous mmap, memory could be > * uninitialized */ > @@ -62,6 +63,7 @@ > # define MAP_UNINITIALIZED 0x0 /* Don't support this flag */ > #endif > > + > /* > * Flags for msync > */ > diff --git a/include/uapi/asm-generic/mman.h b/include/uapi/asm-generic/mman.h > index 7162cd4cca73..64c46047fbd3 100644 > --- a/include/uapi/asm-generic/mman.h > +++ b/include/uapi/asm-generic/mman.h > @@ -12,6 +12,7 @@ > #define MAP_NONBLOCK 0x10000 /* do not block on IO */ > #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x80000 /* MAP_FIXED which doesn't unmap underlying mapping */ > > /* Bits [26:31] are reserved, see mman-common.h for MAP_HUGETLB usage */ > > diff --git a/mm/mmap.c b/mm/mmap.c > index 680506faceae..89af0b5839a5 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1342,6 +1342,10 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > if (!(file && path_noexec(&file->f_path))) > prot |= PROT_EXEC; > > + /* force arch specific MAP_FIXED handling in get_unmapped_area */ > + if (flags & MAP_FIXED_SAFE) > + flags |= MAP_FIXED; Hooking in at this point is a nice way to solve the problem. :) For the naming and implementation, I see a couple of things that might improve it slightly: a) Change MAP_FIXED_SAFE to MAP_NO_CLOBBER (as per Kees' idea), but keep the new flag independent, by omitting the above two lines. Instead of forcing MAP_FIXED as a result of the new flag, you could simply fail to take any action on MAP_NO_CLOBBER *unless* MAP_FIXED is set. This is a bit easier to explain and reason about, as compared to a flag that auto-sets another flag. I like this approach best. or b) Change MAP_FIXED_SAFE to MAP_FIXED_NO_CLOBBER (also a variation on Kees' name idea, but a little longer, a bit uglier, and clearer), and leave the implementation the same. thanks, John Hubbard > + > if (!(flags & MAP_FIXED)) > addr = round_hint_to_min(addr); > > @@ -1365,6 +1369,13 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > if (offset_in_page(addr)) > return addr; > > + if (flags & MAP_FIXED_SAFE) { > + struct vm_area_struct *vma = find_vma(mm, addr); > + > + if (vma && vma->vm_start <= addr) > + return -ENOMEM; > + } > + > if (prot == PROT_EXEC) { > pkey = execute_only_pkey(mm); > if (pkey < 0) > From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Hubbard Subject: Re: [RFC PATCH 1/2] mm: introduce MAP_FIXED_SAFE Date: Fri, 17 Nov 2017 00:37:18 -0800 Message-ID: References: <20171116101900.13621-1-mhocko@kernel.org> <20171116101900.13621-2-mhocko@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20171116101900.13621-2-mhocko@kernel.org> Content-Language: en-US Sender: linux-arch-owner@vger.kernel.org To: Michal Hocko , linux-api@vger.kernel.org Cc: Khalid Aziz , Michael Ellerman , Andrew Morton , Russell King - ARM Linux , Andrea Arcangeli , linux-mm@kvack.org, LKML , linux-arch@vger.kernel.org, Michal Hocko List-Id: linux-api@vger.kernel.org On 11/16/2017 02:18 AM, Michal Hocko wrote: > From: Michal Hocko > > MAP_FIXED is used quite often to enforce mapping at the particular > range. The main problem of this flag is, however, that it is inherently > dangerous because it unmaps existing mappings covered by the requested > range. This can cause silent memory corruptions. Some of them even with > serious security implications. While the current semantic might be > really desiderable in many cases there are others which would want to > enforce the given range but rather see a failure than a silent memory > corruption on a clashing range. Please note that there is no guarantee > that a given range is obeyed by the mmap even when it is free - e.g. > arch specific code is allowed to apply an alignment. > > Introduce a new MAP_FIXED_SAFE flag for mmap to achieve this behavior. > It has the same semantic as MAP_FIXED wrt. the given address request > with a single exception that it fails with ENOMEM if the requested > address is already covered by an existing mapping. We still do rely on > get_unmaped_area to handle all the arch specific MAP_FIXED treatment and > check for a conflicting vma after it returns. > > [set MAP_FIXED before round_hint_to_min as per Khalid Aziz] > Signed-off-by: Michal Hocko > --- > arch/alpha/include/uapi/asm/mman.h | 2 ++ > arch/mips/include/uapi/asm/mman.h | 2 ++ > arch/parisc/include/uapi/asm/mman.h | 2 ++ > arch/powerpc/include/uapi/asm/mman.h | 1 + > arch/sparc/include/uapi/asm/mman.h | 1 + > arch/tile/include/uapi/asm/mman.h | 1 + > arch/xtensa/include/uapi/asm/mman.h | 2 ++ > include/uapi/asm-generic/mman.h | 1 + > mm/mmap.c | 11 +++++++++++ > 9 files changed, 23 insertions(+) > > diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h > index 3b26cc62dadb..0e5724e4b4ad 100644 > --- a/arch/alpha/include/uapi/asm/mman.h > +++ b/arch/alpha/include/uapi/asm/mman.h > @@ -31,6 +31,8 @@ > #define MAP_STACK 0x80000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x100000 /* create a huge page mapping */ > > +#define MAP_FIXED_SAFE 0x200000 /* MAP_FIXED which doesn't unmap underlying mapping */ > + > #define MS_ASYNC 1 /* sync memory asynchronously */ > #define MS_SYNC 2 /* synchronous memory sync */ > #define MS_INVALIDATE 4 /* invalidate the caches */ > diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h > index da3216007fe0..fc5e61ef9fd4 100644 > --- a/arch/mips/include/uapi/asm/mman.h > +++ b/arch/mips/include/uapi/asm/mman.h > @@ -49,6 +49,8 @@ > #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ > > +#define MAP_FIXED_SAFE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */ > + > /* > * Flags for msync > */ > diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h > index cc9ba1d34779..c926487472fa 100644 > --- a/arch/parisc/include/uapi/asm/mman.h > +++ b/arch/parisc/include/uapi/asm/mman.h > @@ -25,6 +25,8 @@ > #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ > > +#define MAP_FIXED_SAFE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */ > + > #define MS_SYNC 1 /* synchronous memory sync */ > #define MS_ASYNC 2 /* sync memory asynchronously */ > #define MS_INVALIDATE 4 /* invalidate the caches */ > diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h > index 03c06ba7464f..d97342ca25b1 100644 > --- a/arch/powerpc/include/uapi/asm/mman.h > +++ b/arch/powerpc/include/uapi/asm/mman.h > @@ -28,5 +28,6 @@ > #define MAP_NONBLOCK 0x10000 /* do not block on IO */ > #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x800000 /* MAP_FIXED which doesn't unmap underlying mapping */ Hi Michal, 1. The powerpc change, above, has one too many zeroes. It should be 0x80000, not 0x800000. 2. For the one-line comments, if you phrase them like this: /* Like MAP_FIXED, except that it doesn't unmap pre-existing mappings */ ...I think that would be a little clearer. more below: > #endif /* _UAPI_ASM_POWERPC_MMAN_H */ > diff --git a/arch/sparc/include/uapi/asm/mman.h b/arch/sparc/include/uapi/asm/mman.h > index 9765896ecb2c..7b00477a7f9a 100644 > --- a/arch/sparc/include/uapi/asm/mman.h > +++ b/arch/sparc/include/uapi/asm/mman.h > @@ -23,6 +23,7 @@ > #define MAP_NONBLOCK 0x10000 /* do not block on IO */ > #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x80000 /* MAP_FIXED which doesn't unmap underlying mapping */ > > > #endif /* _UAPI__SPARC_MMAN_H__ */ > diff --git a/arch/tile/include/uapi/asm/mman.h b/arch/tile/include/uapi/asm/mman.h > index 63ee13faf17d..d5d58d2dc95e 100644 > --- a/arch/tile/include/uapi/asm/mman.h > +++ b/arch/tile/include/uapi/asm/mman.h > @@ -29,6 +29,7 @@ > #define MAP_DENYWRITE 0x0800 /* ETXTBSY */ > #define MAP_EXECUTABLE 0x1000 /* mark it as an executable */ > #define MAP_HUGETLB 0x4000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x8000 /* MAP_FIXED which doesn't unmap underlying mapping */ > > > /* > diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h > index b15b278aa314..d665bd8b7cbd 100644 > --- a/arch/xtensa/include/uapi/asm/mman.h > +++ b/arch/xtensa/include/uapi/asm/mman.h > @@ -55,6 +55,7 @@ > #define MAP_NONBLOCK 0x20000 /* do not block on IO */ > #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */ > #ifdef CONFIG_MMAP_ALLOW_UNINITIALIZED > # define MAP_UNINITIALIZED 0x4000000 /* For anonymous mmap, memory could be > * uninitialized */ > @@ -62,6 +63,7 @@ > # define MAP_UNINITIALIZED 0x0 /* Don't support this flag */ > #endif > > + > /* > * Flags for msync > */ > diff --git a/include/uapi/asm-generic/mman.h b/include/uapi/asm-generic/mman.h > index 7162cd4cca73..64c46047fbd3 100644 > --- a/include/uapi/asm-generic/mman.h > +++ b/include/uapi/asm-generic/mman.h > @@ -12,6 +12,7 @@ > #define MAP_NONBLOCK 0x10000 /* do not block on IO */ > #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x80000 /* MAP_FIXED which doesn't unmap underlying mapping */ > > /* Bits [26:31] are reserved, see mman-common.h for MAP_HUGETLB usage */ > > diff --git a/mm/mmap.c b/mm/mmap.c > index 680506faceae..89af0b5839a5 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1342,6 +1342,10 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > if (!(file && path_noexec(&file->f_path))) > prot |= PROT_EXEC; > > + /* force arch specific MAP_FIXED handling in get_unmapped_area */ > + if (flags & MAP_FIXED_SAFE) > + flags |= MAP_FIXED; Hooking in at this point is a nice way to solve the problem. :) For the naming and implementation, I see a couple of things that might improve it slightly: a) Change MAP_FIXED_SAFE to MAP_NO_CLOBBER (as per Kees' idea), but keep the new flag independent, by omitting the above two lines. Instead of forcing MAP_FIXED as a result of the new flag, you could simply fail to take any action on MAP_NO_CLOBBER *unless* MAP_FIXED is set. This is a bit easier to explain and reason about, as compared to a flag that auto-sets another flag. I like this approach best. or b) Change MAP_FIXED_SAFE to MAP_FIXED_NO_CLOBBER (also a variation on Kees' name idea, but a little longer, a bit uglier, and clearer), and leave the implementation the same. thanks, John Hubbard > + > if (!(flags & MAP_FIXED)) > addr = round_hint_to_min(addr); > > @@ -1365,6 +1369,13 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > if (offset_in_page(addr)) > return addr; > > + if (flags & MAP_FIXED_SAFE) { > + struct vm_area_struct *vma = find_vma(mm, addr); > + > + if (vma && vma->vm_start <= addr) > + return -ENOMEM; > + } > + > if (prot == PROT_EXEC) { > pkey = execute_only_pkey(mm); > if (pkey < 0) > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f199.google.com (mail-pf0-f199.google.com [209.85.192.199]) by kanga.kvack.org (Postfix) with ESMTP id 2C8626B0038 for ; Fri, 17 Nov 2017 03:37:21 -0500 (EST) Received: by mail-pf0-f199.google.com with SMTP id f85so1813043pfe.7 for ; Fri, 17 Nov 2017 00:37:21 -0800 (PST) Received: from hqemgate15.nvidia.com (hqemgate15.nvidia.com. [216.228.121.64]) by mx.google.com with ESMTPS id d11si2399656plr.754.2017.11.17.00.37.19 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 17 Nov 2017 00:37:19 -0800 (PST) Subject: Re: [RFC PATCH 1/2] mm: introduce MAP_FIXED_SAFE References: <20171116101900.13621-1-mhocko@kernel.org> <20171116101900.13621-2-mhocko@kernel.org> From: John Hubbard Message-ID: Date: Fri, 17 Nov 2017 00:37:18 -0800 MIME-Version: 1.0 In-Reply-To: <20171116101900.13621-2-mhocko@kernel.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko , linux-api@vger.kernel.org Cc: Khalid Aziz , Michael Ellerman , Andrew Morton , Russell King - ARM Linux , Andrea Arcangeli , linux-mm@kvack.org, LKML , linux-arch@vger.kernel.org, Michal Hocko On 11/16/2017 02:18 AM, Michal Hocko wrote: > From: Michal Hocko > > MAP_FIXED is used quite often to enforce mapping at the particular > range. The main problem of this flag is, however, that it is inherently > dangerous because it unmaps existing mappings covered by the requested > range. This can cause silent memory corruptions. Some of them even with > serious security implications. While the current semantic might be > really desiderable in many cases there are others which would want to > enforce the given range but rather see a failure than a silent memory > corruption on a clashing range. Please note that there is no guarantee > that a given range is obeyed by the mmap even when it is free - e.g. > arch specific code is allowed to apply an alignment. > > Introduce a new MAP_FIXED_SAFE flag for mmap to achieve this behavior. > It has the same semantic as MAP_FIXED wrt. the given address request > with a single exception that it fails with ENOMEM if the requested > address is already covered by an existing mapping. We still do rely on > get_unmaped_area to handle all the arch specific MAP_FIXED treatment and > check for a conflicting vma after it returns. > > [set MAP_FIXED before round_hint_to_min as per Khalid Aziz] > Signed-off-by: Michal Hocko > --- > arch/alpha/include/uapi/asm/mman.h | 2 ++ > arch/mips/include/uapi/asm/mman.h | 2 ++ > arch/parisc/include/uapi/asm/mman.h | 2 ++ > arch/powerpc/include/uapi/asm/mman.h | 1 + > arch/sparc/include/uapi/asm/mman.h | 1 + > arch/tile/include/uapi/asm/mman.h | 1 + > arch/xtensa/include/uapi/asm/mman.h | 2 ++ > include/uapi/asm-generic/mman.h | 1 + > mm/mmap.c | 11 +++++++++++ > 9 files changed, 23 insertions(+) > > diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h > index 3b26cc62dadb..0e5724e4b4ad 100644 > --- a/arch/alpha/include/uapi/asm/mman.h > +++ b/arch/alpha/include/uapi/asm/mman.h > @@ -31,6 +31,8 @@ > #define MAP_STACK 0x80000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x100000 /* create a huge page mapping */ > > +#define MAP_FIXED_SAFE 0x200000 /* MAP_FIXED which doesn't unmap underlying mapping */ > + > #define MS_ASYNC 1 /* sync memory asynchronously */ > #define MS_SYNC 2 /* synchronous memory sync */ > #define MS_INVALIDATE 4 /* invalidate the caches */ > diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h > index da3216007fe0..fc5e61ef9fd4 100644 > --- a/arch/mips/include/uapi/asm/mman.h > +++ b/arch/mips/include/uapi/asm/mman.h > @@ -49,6 +49,8 @@ > #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ > > +#define MAP_FIXED_SAFE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */ > + > /* > * Flags for msync > */ > diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h > index cc9ba1d34779..c926487472fa 100644 > --- a/arch/parisc/include/uapi/asm/mman.h > +++ b/arch/parisc/include/uapi/asm/mman.h > @@ -25,6 +25,8 @@ > #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ > > +#define MAP_FIXED_SAFE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */ > + > #define MS_SYNC 1 /* synchronous memory sync */ > #define MS_ASYNC 2 /* sync memory asynchronously */ > #define MS_INVALIDATE 4 /* invalidate the caches */ > diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h > index 03c06ba7464f..d97342ca25b1 100644 > --- a/arch/powerpc/include/uapi/asm/mman.h > +++ b/arch/powerpc/include/uapi/asm/mman.h > @@ -28,5 +28,6 @@ > #define MAP_NONBLOCK 0x10000 /* do not block on IO */ > #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x800000 /* MAP_FIXED which doesn't unmap underlying mapping */ Hi Michal, 1. The powerpc change, above, has one too many zeroes. It should be 0x80000, not 0x800000. 2. For the one-line comments, if you phrase them like this: /* Like MAP_FIXED, except that it doesn't unmap pre-existing mappings */ ...I think that would be a little clearer. more below: > #endif /* _UAPI_ASM_POWERPC_MMAN_H */ > diff --git a/arch/sparc/include/uapi/asm/mman.h b/arch/sparc/include/uapi/asm/mman.h > index 9765896ecb2c..7b00477a7f9a 100644 > --- a/arch/sparc/include/uapi/asm/mman.h > +++ b/arch/sparc/include/uapi/asm/mman.h > @@ -23,6 +23,7 @@ > #define MAP_NONBLOCK 0x10000 /* do not block on IO */ > #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x80000 /* MAP_FIXED which doesn't unmap underlying mapping */ > > > #endif /* _UAPI__SPARC_MMAN_H__ */ > diff --git a/arch/tile/include/uapi/asm/mman.h b/arch/tile/include/uapi/asm/mman.h > index 63ee13faf17d..d5d58d2dc95e 100644 > --- a/arch/tile/include/uapi/asm/mman.h > +++ b/arch/tile/include/uapi/asm/mman.h > @@ -29,6 +29,7 @@ > #define MAP_DENYWRITE 0x0800 /* ETXTBSY */ > #define MAP_EXECUTABLE 0x1000 /* mark it as an executable */ > #define MAP_HUGETLB 0x4000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x8000 /* MAP_FIXED which doesn't unmap underlying mapping */ > > > /* > diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h > index b15b278aa314..d665bd8b7cbd 100644 > --- a/arch/xtensa/include/uapi/asm/mman.h > +++ b/arch/xtensa/include/uapi/asm/mman.h > @@ -55,6 +55,7 @@ > #define MAP_NONBLOCK 0x20000 /* do not block on IO */ > #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */ > #ifdef CONFIG_MMAP_ALLOW_UNINITIALIZED > # define MAP_UNINITIALIZED 0x4000000 /* For anonymous mmap, memory could be > * uninitialized */ > @@ -62,6 +63,7 @@ > # define MAP_UNINITIALIZED 0x0 /* Don't support this flag */ > #endif > > + > /* > * Flags for msync > */ > diff --git a/include/uapi/asm-generic/mman.h b/include/uapi/asm-generic/mman.h > index 7162cd4cca73..64c46047fbd3 100644 > --- a/include/uapi/asm-generic/mman.h > +++ b/include/uapi/asm-generic/mman.h > @@ -12,6 +12,7 @@ > #define MAP_NONBLOCK 0x10000 /* do not block on IO */ > #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ > #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ > +#define MAP_FIXED_SAFE 0x80000 /* MAP_FIXED which doesn't unmap underlying mapping */ > > /* Bits [26:31] are reserved, see mman-common.h for MAP_HUGETLB usage */ > > diff --git a/mm/mmap.c b/mm/mmap.c > index 680506faceae..89af0b5839a5 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1342,6 +1342,10 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > if (!(file && path_noexec(&file->f_path))) > prot |= PROT_EXEC; > > + /* force arch specific MAP_FIXED handling in get_unmapped_area */ > + if (flags & MAP_FIXED_SAFE) > + flags |= MAP_FIXED; Hooking in at this point is a nice way to solve the problem. :) For the naming and implementation, I see a couple of things that might improve it slightly: a) Change MAP_FIXED_SAFE to MAP_NO_CLOBBER (as per Kees' idea), but keep the new flag independent, by omitting the above two lines. Instead of forcing MAP_FIXED as a result of the new flag, you could simply fail to take any action on MAP_NO_CLOBBER *unless* MAP_FIXED is set. This is a bit easier to explain and reason about, as compared to a flag that auto-sets another flag. I like this approach best. or b) Change MAP_FIXED_SAFE to MAP_FIXED_NO_CLOBBER (also a variation on Kees' name idea, but a little longer, a bit uglier, and clearer), and leave the implementation the same. thanks, John Hubbard > + > if (!(flags & MAP_FIXED)) > addr = round_hint_to_min(addr); > > @@ -1365,6 +1369,13 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > if (offset_in_page(addr)) > return addr; > > + if (flags & MAP_FIXED_SAFE) { > + struct vm_area_struct *vma = find_vma(mm, addr); > + > + if (vma && vma->vm_start <= addr) > + return -ENOMEM; > + } > + > if (prot == PROT_EXEC) { > pkey = execute_only_pkey(mm); > if (pkey < 0) > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org