linux-kernel - Re: [PATCH] tracing/user_events: Run BPF program if attached

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:   Mon, 15 May 2023 12:35:32 -0700
From:   Beau Belgrave <beaub@...ux.microsoft.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-trace-kernel@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        David Vernet <void@...ifault.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        dthaler@...rosoft.com, brauner@...nel.org, hch@...radead.org
Subject: Re: [PATCH] tracing/user_events: Run BPF program if attached

On Mon, May 15, 2023 at 02:33:05PM -0400, Steven Rostedt wrote:
> On Mon, 15 May 2023 09:57:07 -0700
> Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> 
> > Thank you for these details. Answer below...
> 
> Thanks for this well thought out reply!
> 
> 

[...]

> > 
> > > 	if (unlikely(ret <= 0)) {
> > > 		if (!fixup_fault)
> > > 			return -EFAULT;
> > > 
> > > 		if (!user_event_enabler_queue_fault(mm, enabler, *attempt))
> > > 			pr_warn("user_events: Unable to queue fault handler\n");  
> > 
> > This part looks questionable.
> > 
> > The only users of fixup_user_fault() were futex and KVM.
> > Now user_events are calling it too from user_event_mm_fault_in() where
> > "bool unlocked;" is uninitialized and state of this flag is not checked
> > after fixup_user_fault() call.
> > Not an MM expert, but this is suspicious.
> 
> Hmm, yeah, this should be:
> 
> static int user_event_mm_fault_in()
> {
> 	bool unlocked = false;
> 
> 	[..]
> 
> out:
> 	if (!unlocked)
> 		mmap_read_unlock(mm->mm);
> }
> 
> Good catch!
> 

I don't believe that's correct. fixup_user_fault() re-acquires the
mmap lock, and when it does, it lets you know via unlocked getting set
to true. IE: Something COULD have changed in the mmap during this call,
but the lock is still held.

See comments here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/gup.c#n1287

Thanks,
-Beau

> 
> Thank you Alexei for asking these. The above are all valid concerns.
> 
> -- Steve
>