| Message ID | 1581150928-3214-1-git-send-email-lixinhai.lxh@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | mm: Fix misuse of parent anon_vma in dup_mmap path | expand |
On Sat, 8 Feb 2020 08:35:25 +0000 Li Xinhai <lixinhai.lxh@gmail.com> wrote: > This patchset fix the misuse of parenet anon_vma, which mainly caused by > child vma's vm_next and vm_prev are left same as its parent after > duplicate vma. Finally, code reached parent vma's neighbor by referring > pointer of child vma and executed wrong logic. > > The first two patches fix relevant issues, and the third patch sets vm_next > and vm_prev to NULL when duplicate vma to prevent potential misuse in future. What are the runtime effects of this bug? How is the bug triggered?
On 2020-02-10 at 08:52 Andrew Morton wrote: >On Sat, 8 Feb 2020 08:35:25 +0000 Li Xinhai <lixinhai.lxh@gmail.com> wrote: > >> This patchset fix the misuse of parenet anon_vma, which mainly caused by >> child vma's vm_next and vm_prev are left same as its parent after >> duplicate vma. Finally, code reached parent vma's neighbor by referring >> pointer of child vma and executed wrong logic. >> >> The first two patches fix relevant issues, and the third patch sets vm_next >> and vm_prev to NULL when duplicate vma to prevent potential misuse in future. > >What are the runtime effects of this bug? How is the bug triggered? Effects of the first one is that causes ramp code to check both parent and child's page table, although a page couldn't be mapped by both parent and child, because child vma has WIPEONFORK so all pages mapped by child are 'new' and not relevant to parent. Effects of the second one is that the relationship of anon_vma of parent and child are totally convoluted. It would cause 'son', 'grandson', ..., etc, to share 'parent' anon_vma, which disobey the design rule of reusing anon_vma (the rule to be followed is that reusing should among vma of same process, and vma should not gone through fork). So, both issues should cause unnecessary rmap walking and have unexpected complexity. These two issues would not be directly visible, I used debugging code to check the anon_vma pointers of parent and child when inspecting the suspicious implementation of issue #2, then find the problem.
On Sat, 8 Feb 2020 08:35:25 +0000 Li Xinhai <lixinhai.lxh@gmail.com> wrote: > This patchset fix the misuse of parenet anon_vma, which mainly caused by > child vma's vm_next and vm_prev are left same as its parent after > duplicate vma. Finally, code reached parent vma's neighbor by referring > pointer of child vma and executed wrong logic. > > The first two patches fix relevant issues, and the third patch sets vm_next > and vm_prev to NULL when duplicate vma to prevent potential misuse in future. > > Li Xinhai (3): > mm: don't prepare anon_vma if vma has VM_WIPEONFORK > Revert "mm/rmap.c: reuse mergeable anon_vma as parent when fork" > mm: set vm_next and vm_prev to NULL in vm_area_dup() > > kernel/fork.c | 10 ++++++---- > mm/rmap.c | 13 ------------- > 2 files changed, 6 insertions(+), 17 deletions(-) Can we please have some review input on this series?
On Mon 10-02-20 10:15:53, Li Xinhai wrote: [...] > >What are the runtime effects of this bug? How is the bug triggered? > > Effects of the first one is that causes ramp code to check both parent and > child's page table, although a page couldn't be mapped by both parent > and child, because child vma has WIPEONFORK so all pages mapped by child > are 'new' and not relevant to parent. > > Effects of the second one is that the relationship of anon_vma of parent and > child are totally convoluted. It would cause 'son', 'grandson', ..., etc, to share > 'parent' anon_vma, which disobey the design rule of reusing anon_vma (the > rule to be followed is that reusing should among vma of same process, and > vma should not gone through fork). > > So, both issues should cause unnecessary rmap walking and have unexpected > complexity. > > These two issues would not be directly visible, I used debugging code to check > the anon_vma pointers of parent and child when inspecting the suspicious > implementation of issue #2, then find the problem. I am still not completely clear on the user effect. Does this allow an adversary to generate too long anon_vma chains that wouldn't normally happen or some other nasties that would make backporting to older kernels really necessary? Because from my current understanding it would only make WIPEONFORK vmas rmap walks less efficient. Or is there any other functional issue?
On 2020-04-20 at 20:06 Michal Hocko wrote: >On Mon 10-02-20 10:15:53, Li Xinhai wrote: >[...] >> >What are the runtime effects of this bug? How is the bug triggered? >> >> Effects of the first one is that causes ramp code to check both parent and >> child's page table, although a page couldn't be mapped by both parent >> and child, because child vma has WIPEONFORK so all pages mapped by child >> are 'new' and not relevant to parent. >> >> Effects of the second one is that the relationship of anon_vma of parent and >> child are totally convoluted. It would cause 'son', 'grandson', ..., etc, to share >> 'parent' anon_vma, which disobey the design rule of reusing anon_vma (the >> rule to be followed is that reusing should among vma of same process, and >> vma should not gone through fork). >> >> So, both issues should cause unnecessary rmap walking and have unexpected >> complexity. >> >> These two issues would not be directly visible, I used debugging code to check >> the anon_vma pointers of parent and child when inspecting the suspicious >> implementation of issue #2, then find the problem. > >I am still not completely clear on the user effect. Does this allow an >adversary to generate too long anon_vma chains that wouldn't normally >happen or some other nasties that would make backporting to older >kernels really necessary? It allows for generating long anon_vma chain by keep forking child process, although the memory consumed for anon_vma structure wouldn't increase accordingly. I don't see other impact of those two issues. > Because from my current understanding it would >only make WIPEONFORK vmas rmap walks less efficient. Or is there any >other functional issue? #1 issue is for case WIPEONFORK, #2 issue for all other cases of anonymous VMA dup (i.e., when parent vmas is currently sharing anon_vma among themself, child vma will share its parent vma's anon_vma). Both cause inefficient rmap walking, no other functional issues. In my understanding, this patchset can be backported to stable kernel. E.g., If someone mmap a big vm area, and split that vma to several adjacent vmas(e.g., by mprotect), then sharing anon_vma at parent process level is achivied. After that, begin fork child, maybe choose to keep forking child from the top parent process, or let child process keep forking its own child. At this moment, sharing anon_vma among children and parent is achieved and has long chain. >-- >Michal Hocko >SUSE Labs
On Tue 21-04-20 11:53:02, Li Xinhai wrote: > On 2020-04-20 at 20:06 Michal Hocko wrote: [...] > > Because from my current understanding it would > >only make WIPEONFORK vmas rmap walks less efficient. Or is there any > >other functional issue? > #1 issue is for case WIPEONFORK, #2 issue for all other cases of anonymous VMA dup > (i.e., when parent vmas is currently sharing anon_vma among themself, child vma will > share its parent vma's anon_vma). OK, it was not really clear to me that also !WIPEONFORK vmas are affected. Thanks for the clarification.