Sumanth, thanks for details!
On 08/05, Sumanth Korikkar wrote:
>
> So semphore incremented to 2 when a tracer was attached and decremented
> back to 0 when tracer was detached. install_breakpoint() was called for
> both vaddr and succeeded, also remove_breakpoint() succeeded for both
> vaddr.
Then I agree with Andrii, we don't really care. IIUC, only the value of
.active != 0 matters correctness-wise.
> > > --- a/kernel/events/uprobes.c
> > > +++ b/kernel/events/uprobes.c
> > > @@ -1155,6 +1155,9 @@ static int install_breakpoint(struct uprobe
> > > *uprobe, struct vm_area_struct *vma,
> > > bool first_uprobe;
> > > int ret;
> > >
> > > + if (!(vma->vm_flags & VM_EXEC))
> > > + return 0;
> > > +
> >
> > Well, but then it makes more sense to change valid_vma() to nack the
> > non VM_EXEC mappings ?
>
> After looking at your 2012 commit 78a320542e6c ("uprobes: Change valid_vma()
> to demand VM_MAYEXEC rather than VM_EXEC"), I thought changing it in
> valid_vma() was not the right approach.
>
> "If a program maps memory as non executable initially, but it has
> VM_MAYEXEC permission, the program can later call mprotect(PROT_EXEC)
> to make it executable." So adding VM_EXEC in valid_vma() can be too
> strict.
Yes,
> Hence, I think install_breakpoint() can be one point where non VM_EXEC
> mapping can be restricted.
But your change in install_breakpoint() essentually reverts that commit?
If uprobe_register() -> register_for_each_vma() finds a VM_MAYEXEC vma
without VM_EXEC valid_vma() will return true, but then install_breakpoint()
will fail anyway.
Oleg.
