| Message ID | 20240312222843.2505560-10-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. > > For MAP_GROWSDOWN/VM_GROWSDOWN and MAP_GROWSUP/VM_GROWSUP this has not > been a problem in practice because applications place these kinds of > mappings very early, when there is not many mappings to find a space > between. But for shadow stacks, they may be placed throughout the lifetime > of the application. > > Use the start_gap field to find a space that includes the guard gap for > the new mapping. Take care to not interfere with the alignment. > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > v3: > - Spelling fix in comment > > v2: > - Remove VM_UNMAPPED_START_GAP_SET and have struct vm_unmapped_area_info > initialized with zeros (in another patch). (Kirill) > - Drop unrelated space change (Kirill) > - Add comment around interactions of alignment and start gap step > (Kirill) > --- > include/linux/mm.h | 1 + > mm/mmap.c | 12 +++++++++--- > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index d91cde79aaee..deade7be00d0 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3418,6 +3418,7 @@ struct vm_unmapped_area_info { > unsigned long high_limit; > unsigned long align_mask; > unsigned long align_offset; > + unsigned long start_gap; Only a start_gap is needed ? No need of an end_gap ? > }; > > extern unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info); > diff --git a/mm/mmap.c b/mm/mmap.c > index b889c79d11bd..634e706fd97e 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1582,7 +1582,7 @@ static unsigned long unmapped_area(struct vm_unmapped_area_info *info) > MA_STATE(mas, ¤t->mm->mm_mt, 0, 0); > > /* Adjust search length to account for worst case alignment overhead */ > - length = info->length + info->align_mask; > + length = info->length + info->align_mask + info->start_gap; > if (length < info->length) > return -ENOMEM; > > @@ -1594,7 +1594,13 @@ static unsigned long unmapped_area(struct vm_unmapped_area_info *info) > if (mas_empty_area(&mas, low_limit, high_limit - 1, length)) > return -ENOMEM; > > - gap = mas.index; > + /* > + * Adjust for the gap first so it doesn't interfere with the > + * later alignment. The first step is the minimum needed to > + * fulill the start gap, the next steps is the minimum to align > + * that. It is the minimum needed to fulill both. > + */ > + gap = mas.index + info->start_gap; > gap += (info->align_offset - gap) & info->align_mask; > tmp = mas_next(&mas, ULONG_MAX); > if (tmp && (tmp->vm_flags & VM_STARTGAP_FLAGS)) { /* Avoid prev check if possible */ > @@ -1633,7 +1639,7 @@ static unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info) > > MA_STATE(mas, ¤t->mm->mm_mt, 0, 0); > /* Adjust search length to account for worst case alignment overhead */ > - length = info->length + info->align_mask; > + length = info->length + info->align_mask + info->start_gap; > if (length < info->length) > return -ENOMEM; >
On Wed, 2024-03-13 at 09:04 +0000, Christophe Leroy wrote: > > Only a start_gap is needed ? No need of an end_gap ? Yea, shadow stacks only have a start gap. If any end gap is needed, it will be much easier to add after all of this.
Le 13/03/2024 à 15:58, Edgecombe, Rick P a écrit : > On Wed, 2024-03-13 at 09:04 +0000, Christophe Leroy wrote: >> >> Only a start_gap is needed ? No need of an end_gap ? > > Yea, shadow stacks only have a start gap. If any end gap is needed, it > will be much easier to add after all of this. Ok Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
diff --git a/include/linux/mm.h b/include/linux/mm.h index d91cde79aaee..deade7be00d0 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3418,6 +3418,7 @@ struct vm_unmapped_area_info { unsigned long high_limit; unsigned long align_mask; unsigned long align_offset; + unsigned long start_gap; }; extern unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info); diff --git a/mm/mmap.c b/mm/mmap.c index b889c79d11bd..634e706fd97e 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1582,7 +1582,7 @@ static unsigned long unmapped_area(struct vm_unmapped_area_info *info) MA_STATE(mas, ¤t->mm->mm_mt, 0, 0); /* Adjust search length to account for worst case alignment overhead */ - length = info->length + info->align_mask; + length = info->length + info->align_mask + info->start_gap; if (length < info->length) return -ENOMEM; @@ -1594,7 +1594,13 @@ static unsigned long unmapped_area(struct vm_unmapped_area_info *info) if (mas_empty_area(&mas, low_limit, high_limit - 1, length)) return -ENOMEM; - gap = mas.index; + /* + * Adjust for the gap first so it doesn't interfere with the + * later alignment. The first step is the minimum needed to + * fulill the start gap, the next steps is the minimum to align + * that. It is the minimum needed to fulill both. + */ + gap = mas.index + info->start_gap; gap += (info->align_offset - gap) & info->align_mask; tmp = mas_next(&mas, ULONG_MAX); if (tmp && (tmp->vm_flags & VM_STARTGAP_FLAGS)) { /* Avoid prev check if possible */ @@ -1633,7 +1639,7 @@ static unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info) MA_STATE(mas, ¤t->mm->mm_mt, 0, 0); /* Adjust search length to account for worst case alignment overhead */ - length = info->length + info->align_mask; + length = info->length + info->align_mask + info->start_gap; if (length < info->length) return -ENOMEM;
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. For MAP_GROWSDOWN/VM_GROWSDOWN and MAP_GROWSUP/VM_GROWSUP this has not been a problem in practice because applications place these kinds of mappings very early, when there is not many mappings to find a space between. But for shadow stacks, they may be placed throughout the lifetime of the application. Use the start_gap field to find a space that includes the guard gap for the new mapping. Take care to not interfere with the alignment. Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- v3: - Spelling fix in comment v2: - Remove VM_UNMAPPED_START_GAP_SET and have struct vm_unmapped_area_info initialized with zeros (in another patch). (Kirill) - Drop unrelated space change (Kirill) - Add comment around interactions of alignment and start gap step (Kirill) --- include/linux/mm.h | 1 + mm/mmap.c | 12 +++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-)