| Message ID | 20240312222843.2505560-11-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Cover a guard gap corner case | expand |
Le 12/03/2024 à 23:28, Rick Edgecombe a écrit : > When memory is being placed, mmap() will take care to respect the guard > gaps of certain types of memory (VM_SHADOWSTACK, VM_GROWSUP and > VM_GROWSDOWN). In order to ensure guard gaps between mappings, mmap() > needs to consider two things: > 1. That the new mapping isn’t placed in an any existing mappings guard > gaps. > 2. That the new mapping isn’t placed such that any existing mappings > are not in *its* guard gaps. > > The long standing behavior of mmap() is to ensure 1, but not take any care > around 2. So for example, if there is a PAGE_SIZE free area, and a > mmap() with a PAGE_SIZE size, and a type that has a guard gap is being > placed, mmap() may place the shadow stack in the PAGE_SIZE free area. Then > the mapping that is supposed to have a guard gap will not have a gap to > the adjacent VMA. > > Add x86 arch implementations of arch_get_unmapped_area_vmflags/_topdown() > so future changes can allow the guard gap of type of vma being placed to > be taken into account. This will be used for shadow stack memory. > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > v3: > - Commit log grammar > > v2: > - Remove unnecessary added extern > --- > arch/x86/include/asm/pgtable_64.h | 1 + > arch/x86/kernel/sys_x86_64.c | 25 ++++++++++++++++++++----- > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h > index 24af25b1551a..13dcaf436efd 100644 > --- a/arch/x86/include/asm/pgtable_64.h > +++ b/arch/x86/include/asm/pgtable_64.h > @@ -244,6 +244,7 @@ extern void cleanup_highmap(void); > > #define HAVE_ARCH_UNMAPPED_AREA > #define HAVE_ARCH_UNMAPPED_AREA_TOPDOWN > +#define HAVE_ARCH_UNMAPPED_AREA_VMFLAGS > > #define PAGE_AGP PAGE_KERNEL_NOCACHE > #define HAVE_PAGE_AGP 1 > diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c > index b3278e4f7e59..d6fbc4dd08ef 100644 > --- a/arch/x86/kernel/sys_x86_64.c > +++ b/arch/x86/kernel/sys_x86_64.c > @@ -120,8 +120,8 @@ static void find_start_end(unsigned long addr, unsigned long flags, > } > > unsigned long > -arch_get_unmapped_area(struct file *filp, unsigned long addr, > - unsigned long len, unsigned long pgoff, unsigned long flags) > +arch_get_unmapped_area_vmflags(struct file *filp, unsigned long addr, unsigned long len, > + unsigned long pgoff, unsigned long flags, vm_flags_t vm_flags) > { > struct mm_struct *mm = current->mm; > struct vm_area_struct *vma; > @@ -156,9 +156,9 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr, > } > > unsigned long > -arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0, > - const unsigned long len, const unsigned long pgoff, > - const unsigned long flags) > +arch_get_unmapped_area_topdown_vmflags(struct file *filp, unsigned long addr0, > + unsigned long len, unsigned long pgoff, > + unsigned long flags, vm_flags_t vm_flags) > { > struct vm_area_struct *vma; > struct mm_struct *mm = current->mm; > @@ -227,3 +227,18 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0, > */ > return arch_get_unmapped_area(filp, addr0, len, pgoff, flags); > } > + > +unsigned long > +arch_get_unmapped_area(struct file *filp, unsigned long addr, > + unsigned long len, unsigned long pgoff, unsigned long flags) > +{ > + return arch_get_unmapped_area_vmflags(filp, addr, len, pgoff, flags, 0); > +} > + > +unsigned long > +arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr, > + const unsigned long len, const unsigned long pgoff, > + const unsigned long flags) > +{ > + return arch_get_unmapped_area_topdown_vmflags(filp, addr, len, pgoff, flags, 0); > +} Wouldn't it be better to define those two as static inlines ?
On Wed, 2024-03-13 at 09:04 +0000, Christophe Leroy wrote: > > + > > +unsigned long > > +arch_get_unmapped_area(struct file *filp, unsigned long addr, > > + unsigned long len, unsigned long pgoff, unsigned > > long flags) > > +{ > > + return arch_get_unmapped_area_vmflags(filp, addr, len, > > pgoff, flags, 0); > > +} > > + > > +unsigned long > > +arch_get_unmapped_area_topdown(struct file *filp, const unsigned > > long addr, > > + const unsigned long len, const unsigned > > long pgoff, > > + const unsigned long flags) > > +{ > > + return arch_get_unmapped_area_topdown_vmflags(filp, addr, > > len, pgoff, flags, 0); > > +} > > Wouldn't it be better to define those two as static inlines ? Yes, I think so. It is generic functionality (though not needed until the next shadow stack feature), so doesn't need to be in arch/x86 either. Thanks for your comments on the series and the RB tags.
On Wed, 2024-03-13 at 09:00 -0700, Rick Edgecombe wrote: > > Wouldn't it be better to define those two as static inlines ? > > Yes, I think so. I gave this a try and it turned out not to fit into any header too well. I decided putting it into a generic header was not great since these are actually supposed to be arch implementations. Emphasizing that point, mmap.c actually defines these unless HAVE_ARCH_UNMAPPED_AREA is defined. So to make it work, mm.h would have to assume that if HAVE_ARCH_UNMAPPED_AREA and HAVE_ARCH_UNMAPPED_AREA_TOPDOWN are defined, the arch actually doesn't have an arch_unmmapped_area() and wants a static inline version of arch_get_unmapped_area(). It confuses the meaning of HAVE_ARCH_UNMAPPED_AREA a bit to mean the opposite in some cases. Adding a ARCH_WANTS_UNMAPPED_AREA_INLINE seemed excessive. As for putting them in an arch/x86 header, I tried asm/mmu.h, but arch_get_unmapped_area_topdown/_vmflags() had to be forward declared. But then also vm_flags_t couldn't be pulled in properly because of a circular dependency. A few others hit weirdness or breakages. So in the end, I decided to just leave it as a non-static inline in arch/x86. Unless there are any objections, I'm going to just let 0-day build test all the archs, and I'll post the series with the rest of the feedback in a few days.
diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h index 24af25b1551a..13dcaf436efd 100644 --- a/arch/x86/include/asm/pgtable_64.h +++ b/arch/x86/include/asm/pgtable_64.h @@ -244,6 +244,7 @@ extern void cleanup_highmap(void); #define HAVE_ARCH_UNMAPPED_AREA #define HAVE_ARCH_UNMAPPED_AREA_TOPDOWN +#define HAVE_ARCH_UNMAPPED_AREA_VMFLAGS #define PAGE_AGP PAGE_KERNEL_NOCACHE #define HAVE_PAGE_AGP 1 diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c index b3278e4f7e59..d6fbc4dd08ef 100644 --- a/arch/x86/kernel/sys_x86_64.c +++ b/arch/x86/kernel/sys_x86_64.c @@ -120,8 +120,8 @@ static void find_start_end(unsigned long addr, unsigned long flags, } unsigned long -arch_get_unmapped_area(struct file *filp, unsigned long addr, - unsigned long len, unsigned long pgoff, unsigned long flags) +arch_get_unmapped_area_vmflags(struct file *filp, unsigned long addr, unsigned long len, + unsigned long pgoff, unsigned long flags, vm_flags_t vm_flags) { struct mm_struct *mm = current->mm; struct vm_area_struct *vma; @@ -156,9 +156,9 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr, } unsigned long -arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0, - const unsigned long len, const unsigned long pgoff, - const unsigned long flags) +arch_get_unmapped_area_topdown_vmflags(struct file *filp, unsigned long addr0, + unsigned long len, unsigned long pgoff, + unsigned long flags, vm_flags_t vm_flags) { struct vm_area_struct *vma; struct mm_struct *mm = current->mm; @@ -227,3 +227,18 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0, */ return arch_get_unmapped_area(filp, addr0, len, pgoff, flags); } + +unsigned long +arch_get_unmapped_area(struct file *filp, unsigned long addr, + unsigned long len, unsigned long pgoff, unsigned long flags) +{ + return arch_get_unmapped_area_vmflags(filp, addr, len, pgoff, flags, 0); +} + +unsigned long +arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr, + const unsigned long len, const unsigned long pgoff, + const unsigned long flags) +{ + return arch_get_unmapped_area_topdown_vmflags(filp, addr, len, pgoff, flags, 0); +}
When memory is being placed, mmap() will take care to respect the guard gaps of certain types of memory (VM_SHADOWSTACK, VM_GROWSUP and VM_GROWSDOWN). In order to ensure guard gaps between mappings, mmap() needs to consider two things: 1. That the new mapping isn’t placed in an any existing mappings guard gaps. 2. That the new mapping isn’t placed such that any existing mappings are not in *its* guard gaps. The long standing behavior of mmap() is to ensure 1, but not take any care around 2. So for example, if there is a PAGE_SIZE free area, and a mmap() with a PAGE_SIZE size, and a type that has a guard gap is being placed, mmap() may place the shadow stack in the PAGE_SIZE free area. Then the mapping that is supposed to have a guard gap will not have a gap to the adjacent VMA. Add x86 arch implementations of arch_get_unmapped_area_vmflags/_topdown() so future changes can allow the guard gap of type of vma being placed to be taken into account. This will be used for shadow stack memory. Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- v3: - Commit log grammar v2: - Remove unnecessary added extern --- arch/x86/include/asm/pgtable_64.h | 1 + arch/x86/kernel/sys_x86_64.c | 25 ++++++++++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-)