linux-kernel - Re: [PATCH 2/2] uprobes: add speculative lockless VMA-to-inode-to-uprobe resolution

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: <CAEf4Bzb60-PjWXK3apuvxsJh70iXpoBo0WR2R4iXYsXwEChnsA@mail.gmail.com>
Date: Tue, 10 Sep 2024 13:56:59 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jann Horn <jannh@...gle.com>
Cc: surenb@...gle.com, Liam Howlett <liam.howlett@...cle.com>, 
	Andrii Nakryiko <andrii@...nel.org>, brauner@...nel.org, linux-trace-kernel@...r.kernel.org, 
	peterz@...radead.org, oleg@...hat.com, rostedt@...dmis.org, 
	mhiramat@...nel.org, bpf@...r.kernel.org, linux-kernel@...r.kernel.org, 
	jolsa@...nel.org, paulmck@...nel.org, willy@...radead.org, 
	akpm@...ux-foundation.org, linux-mm@...ck.org, mjguzik@...il.com, 
	Miklos Szeredi <miklos@...redi.hu>, Amir Goldstein <amir73il@...il.com>, 
	linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH 2/2] uprobes: add speculative lockless VMA-to-inode-to-uprobe
 resolution

On Tue, Sep 10, 2024 at 8:39 AM Jann Horn <jannh@...gle.com> wrote:
>
> On Mon, Sep 9, 2024 at 11:29 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> > On Mon, Sep 9, 2024 at 6:13 AM Jann Horn <jannh@...gle.com> wrote:
> > > On Fri, Sep 6, 2024 at 7:12 AM Andrii Nakryiko <andrii@...nel.org> wrote:
> > > > +static struct uprobe *find_active_uprobe_speculative(unsigned long bp_vaddr)
> > > > +{
> > > > +       const vm_flags_t flags = VM_HUGETLB | VM_MAYEXEC | VM_MAYSHARE;
> > > > +       struct mm_struct *mm = current->mm;
> > > > +       struct uprobe *uprobe;
> > > > +       struct vm_area_struct *vma;
> > > > +       struct file *vm_file;
> > > > +       struct inode *vm_inode;
> > > > +       unsigned long vm_pgoff, vm_start;
> > > > +       int seq;
> > > > +       loff_t offset;
> > > > +
> > > > +       if (!mmap_lock_speculation_start(mm, &seq))
> > > > +               return NULL;
> > > > +
> > > > +       rcu_read_lock();
> > > > +
> > > > +       vma = vma_lookup(mm, bp_vaddr);
> > > > +       if (!vma)
> > > > +               goto bail;
> > > > +
> > > > +       vm_file = data_race(vma->vm_file);
> > >
> > > A plain "data_race()" says "I'm fine with this load tearing", but
> > > you're relying on this load not tearing (since you access the vm_file
> > > pointer below).
> > > You're also relying on the "struct file" that vma->vm_file points to
> > > being populated at this point, which means you need CONSUME semantics
> > > here, which READ_ONCE() will give you, and something like RELEASE
> > > semantics on any pairing store that populates vma->vm_file, which
> > > means they'd all have to become something like smp_store_release()).
> >
> > vma->vm_file should be set in VMA before it is installed and is never
> > modified afterwards, isn't that the case? So maybe no extra barrier
> > are needed and READ_ONCE() would be enough.
>
> Ah, right, I'm not sure what I was thinking there.
>
> I... guess you only _really_ need the READ_ONCE() if something can
> actually ever change the ->vm_file pointer, otherwise just a plain
> load with no annotation whatsoever would be good enough? I'm fairly

yep, probably, I was just trying to be cautious :)

> sure nothing can ever change the ->vm_file pointer of a live VMA, and
> I think _currently_ it looks like nothing will NULL out the ->vm_file
> pointer on free either... though that last part is probably not
> something you should rely on...

This seems to be rather important, but similarly to how vm_file can't
be modified, it seems reasonable to assume that it won't be set to
NULL (it's a modification to set it to a new NULL value, isn't it?). I
mean, we can probably just add a NULL check and rely on the atomicity
of setting a pointer, so not a big deal, but seems like a pretty
reasonable assumption to make.