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]

Date:   Thu, 17 Oct 2019 14:05:19 +0000
From:   Song Liu <songliubraving@...com>
To:     Oleg Nesterov <oleg@...hat.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 Oct 17, 2019, at 1:47 AM, Oleg Nesterov <oleg@...hat.com> wrote:
> 
> 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.

So the case we worry about is: 
old_page is COW by user space, target insn is int3, and it is a huge page; 
then uprobe calls remove_breakpoint(); 

Yeah, I guess this could happen. 

For the fix, I guess return -Esomething in such case should be sufficient?

Thanks,
Song