uprobes: mprotect fixes
Subsystems (2)
Oleg Nesterov
maintainercover uprobes: mprotect fixes
Hello.
_register/_unregister doesn't work correctly if the probed application plays with mprotect.
Oleg.
kernel/events/uprobes.c | 15 +++++---------- 1 files changed, 5 insertions(+), 10 deletions(-)
Oleg Nesterov
maintainerPATCH 1/3 uprobes: Change write_opcode() to use FOLL_FORCE
write_opcode()->get_user_pages() needs FOLL_FORCE to ensure we can read the page even if the probed task did mprotect(PROT_NONE) after uprobe_register(). Without FOLL_WRITE, FOLL_FORCE doesn't have any side effect but allows to read the !VM_READ memory.
Otherwiese the subsequent uprobe_unregister()->set_orig_insn() fails and we leak "int3". If that task does mprotect(PROT_READ | EXEC) and execute the probed insn later it will be killed.
Note: in fact this is also needed for _register, see the next patch.
--- kernel/events/uprobes.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/events/uprobes.c +1 -1
@@ -221,7 +221,7 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, retry:/* Read the page with vaddr into memory */ - ret =get_user_pages (NULL , mm, vaddr,1 ,0 ,0 , &old_page, &vma); + ret =get_user_pages (NULL , mm, vaddr,1 ,0 ,1 , &old_page, &vma);if (ret <=0 )return ret;
Signature · 1 line
1.5.5.1
Re: [PATCH 1/3] uprobes: Change write_opcode() to use FOLL_FORCE
Quoted 5 lines · Oleg Nesterov
write_opcode()->get_user_pages() needs
FOLL_FORCEto ensure we can read the page even if the probed task didmprotect(PROT_NONE)afteruprobe_register(). WithoutFOLL_WRITE,FOLL_FORCEdoesn't have any side effect but allows to read the !VM_READ memory.
Quoted 7 lines
Otherwiese the subsequent uprobe_unregister()->set_orig_insn() fails and we leak "int3". If that task does
mprotect(PROT_READ | EXEC)and execute the probed insn later it will be killed.Note: in fact this is also needed for
_register, see the next patch.Signed-off-by Oleg Nesterov <[email protected]>
Quoted 20 lines
--- kernel/events/uprobes.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/events/uprobes.c +1
-1@@ -221,7 +221,7 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, retry:/* Read the page with vaddr into memory */ - ret =get_user_pages (NULL , mm, vaddr,1 ,0 ,0 , &old_page, &vma); + ret =get_user_pages (NULL , mm, vaddr,1 ,0 ,1 , &old_page, &vma);if (ret <=0 )return ret;Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 2/3 uprobes: Change valid_vma() to demand VM_MAYEXEC rather than VM_EXEC
In reply to Oleg Nesterovuprobe_register() or uprobe_mmap() requires VM_READ | VM_EXEC, this is not right. An apllication can do mprotect(PROT_EXEC) later and execute this code.
Change valid_vma(is_register => true) to check VM_MAYEXEC instead. No need to check VM_MAYREAD, it is always set.
--- kernel/events/uprobes.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
kernel/events/uprobes.c +2 -2
@@ -106,8 +106,8 @@ static bool valid_vma(struct vm_area_struct *vma, bool is_register)if (!is_register)return true; -if ((vma->vm_flags & (VM_HUGETLB |VM_READ |VM_WRITE |VM_EXEC |VM_SHARED )) - == (VM_READ |VM_EXEC )) +if ((vma->vm_flags & (VM_HUGETLB |VM_WRITE |VM_MAYEXEC |VM_SHARED )) + ==VM_MAYEXEC )return true;return false;
Signature · 1 line
1.5.5.1
Re: [PATCH 2/3] uprobes: Change valid_vma() to demand VM_MAYEXEC rather than VM_EXEC
Quoted 8 lines · Oleg Nesterov
uprobe_register()oruprobe_mmap()requiresVM_READ|VM_EXEC, this is not right. An apllication can domprotect(PROT_EXEC)later and execute this code.Change
valid_vma(is_register => true)to checkVM_MAYEXECinstead. No need to checkVM_MAYREAD, it is always set.Signed-off-by Oleg Nesterov <[email protected]>
Quoted 22 lines
--- kernel/events/uprobes.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)kernel/events/uprobes.c +2
-2@@ -106,8 +106,8 @@ static bool valid_vma(struct vm_area_struct *vma, bool is_register)if (!is_register)return true; -if ((vma->vm_flags & (VM_HUGETLB |VM_READ |VM_WRITE |VM_EXEC |VM_SHARED )) - == (VM_READ |VM_EXEC )) +if ((vma->vm_flags & (VM_HUGETLB |VM_WRITE |VM_MAYEXEC |VM_SHARED )) + ==VM_MAYEXEC )return true;return false;Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 3/3 uprobes: Restrict valid_vma(false) to skip VM_SHARED
In reply to Oleg Nesterovvalid_vma(false) ignores ->vm_flags, this is not actually right. We should never try to write into MAP_SHARED mapping, this can confuse an apllication which actually writes to ->vm_file.
With this patch valid_vma(false) ignores VM_WRITE only but checks other (immutable) bits checked by valid_vma(true). This can also speedup uprobe_munmap() and uprobe_unregister().
Note: even after this patch _unregister can confuse the probed application if it does mprotect(PROT_WRITE) after _register and installs "int3", but this is hardly possible to avoid and this doesn't differ from gdb case.
--- kernel/events/uprobes.c | 13 ++++--------- 1 files changed, 4 insertions(+), 9 deletions(-)
kernel/events/uprobes.c +4 -9
@@ -100,17 +100,12 @@ struct uprobe {*/ static bool valid_vma (struct vm_area_struct *vma,bool is_register) { -if (!vma->vm_file) -return false; - -if (!is_register) -return true; +vm_flags_t flags =VM_HUGETLB |VM_MAYEXEC |VM_SHARED ; -if ((vma->vm_flags & (VM_HUGETLB |VM_WRITE |VM_MAYEXEC |VM_SHARED )) - ==VM_MAYEXEC ) -return true; +if (is_register) + flags |=VM_WRITE ; -return false; +return vma->vm_file && (vma->vm_flags & flags) ==VM_MAYEXEC ; }static unsigned long offset_to_vaddr (struct vm_area_struct *vma,loff_t offset)
Signature · 1 line
1.5.5.1
Re: [PATCH 3/3] uprobes: Restrict valid_vma(false) to skip VM_SHARED
Quoted 3 lines · Oleg Nesterov
valid_vma(false)ignores ->vm_flags, this is not actually right. We should never try to write intoMAP_SHAREDmapping, this can confuse an apllication which actually writes to ->vm_file.
Agree,
Quoted 3 lines
With this patch
valid_vma(false)ignoresVM_WRITEonly but checks other (immutable) bits checked byvalid_vma(true).
Yes, checking for other immutable flags other than VM_WRITE is good.
Quoted 3 lines
This can also
speedupuprobe_munmap()anduprobe_unregister().
I didnt get how it speeds up uprobe_munmap() and uprobe_unregister()?
Quoted 5 lines
Note: even after this patch
_unregistercan confuse the probed application if it doesmprotect(PROT_WRITE)after_registerand installs "int3", but this is hardly possible to avoid and this doesn't differ from gdb case.
Again I didnt quite understand how unregister can confuse the probed application.
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>
The changes look good.
Quoted 33 lines
--- kernel/events/uprobes.c | 13 ++++--------- 1 files changed, 4 insertions(+), 9 deletions(-)kernel/events/uprobes.c +4
-9@@ -100,17 +100,12 @@ struct uprobe {*/ static bool valid_vma (struct vm_area_struct *vma,bool is_register) { -if (!vma->vm_file) -return false; - -if (!is_register) -return true; +vm_flags_t flags =VM_HUGETLB |VM_MAYEXEC |VM_SHARED ; -if ((vma->vm_flags & (VM_HUGETLB |VM_WRITE |VM_MAYEXEC |VM_SHARED )) - ==VM_MAYEXEC ) -return true; +if (is_register) + flags |=VM_WRITE ; -return false; +return vma->vm_file && (vma->vm_flags & flags) ==VM_MAYEXEC ; }static unsigned long offset_to_vaddr (struct vm_area_struct *vma,loff_t offset)Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerRe: [PATCH 3/3] uprobes: Restrict valid_vma(false) to skip VM_SHARED
Quoted 8 lines · Srikar Dronamraju
Quoted 3 lines · Oleg Nesterov
This can also
speedupuprobe_munmap()anduprobe_unregister().I didnt get how it speeds up
uprobe_munmap()anduprobe_unregister()?
Say, uprobe_unregister()->..->build_map_info() can skip VM_SHARED vma early and avoid the unnecessary remove_breakpoint/get_user_pages.
The same for munmap(), no need to do vma_has_uprobes/etc if we know that this vma can't have uprobes because valid_vma(true) is not possible.
Quoted 7 lines
Quoted 4 lines
Note: even after this patch
_unregistercan confuse the probed application if it doesmprotect(PROT_WRITE)after_registerand installs "int3", but this is hardly possible to avoid and this doesn't differ from gdb case.Again I didnt quite understand how unregister can confuse the probed application.
Because set_orig_insn() can never know if this "int3" was set by us (by register) or by gdb or application itself.
But I agree, the text above looks confusing, I just wanted to remind that this patch can't solve all problems like this.
But at least with this patch it is not possible to confuse the app which tries to _modify_ ->vm_file via mmap.
In the long term it would be nice to avoid these problems somehow, but this is not easy. Say, perhaps we can mark the page installed by uprobes as OWNED-BY-KERNEL-DONT-COW and offload set_swbp() to page fault.
Or, simpler, perhaps uprobe_register() can remove VM_MAYWRITE, but this affects the whole vma and it is not clear how _unregister can restore this flag correctly.
But this is off-topic.
Quoted 1 line
Thanks!
Oleg.