linux-kernel - Re: [PATCH 4/4] uprobe: only do FOLL_SPLIT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191017084714.GB17513@redhat.com>
Date:   Thu, 17 Oct 2019 10:47:14 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Song Liu <songliubraving@...com>
Cc:     open list <linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "matthew.wilcox@...cle.com" <matthew.wilcox@...cle.com>,
        Kernel Team <Kernel-team@...com>,
        "william.kucharski@...cle.com" <william.kucharski@...cle.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Subject: Re: [PATCH 4/4] uprobe: only do FOLL_SPLIT_PMD for uprobe register

On 10/16, Song Liu wrote:
>
> > On Oct 16, 2019, at 5:10 AM, Oleg Nesterov <oleg@...hat.com> wrote:
> >
> >> @@ -489,6 +492,9 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
> >> 	if (ret <= 0)
> >> 		goto put_old;
> >>
> >> +	WARN(!is_register && PageCompound(old_page),
> >> +	     "uprobe unregister should never work on compound page\n");
> >
> > But this can happen with the change above. You can't know if *vaddr was
> > previously changed by install_breakpoint() or not.
>
> > If not, verify_opcode() should likely save us, but we can't rely on it.
> > Say, someone can write "int3" into vm_file at uprobe->offset.
>
> I think this won't really happen. With is_register == false, we already
> know opcode is not "int3", so current call must be from set_orig_insn().
> Therefore, old_page must be installed by uprobe, and cannot be compound.
>
> The other way is not guaranteed. With is_register == true, it is still
> possible current call is from set_orig_insn(). However, we do not rely
> on this path.

Quite contrary.

When is_register == true we know that a) the caller is install_breakpoint()
and b) the original insn is NOT int3 unless this page was alreadt COW'ed by
userspace, say, by gdb.

If is_register == false we only know that the caller is remove_breakpoint().
We can't know if this page was COW'ed by uprobes or userspace, we can not
know if the insn we are going to replace is int3 or not, thus we can not
assume that verify_opcode() will fail and save us.

Oleg.