Re: [PATCH bpf-next v10 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Wenbo Zhang <ethercflow@gmail.com>,
	bpf@vger.kernel.org, ast@kernel.org.com, daniel@iogearbox.net,
	yhs@fb.com, andrii.nakryiko@gmail.com, netdev@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH bpf-next v10 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname
Date: Fri, 22 Nov 2019 21:19:21 -0800	[thread overview]
Message-ID: <20191123051919.dsw7v6jyad4j4ilc@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20191123045151.GH26530@ZenIV.linux.org.uk>

On Sat, Nov 23, 2019 at 04:51:51AM +0000, Al Viro wrote:
> On Fri, Nov 22, 2019 at 07:18:28PM -0800, Alexei Starovoitov wrote:
> > > +	f = fget_raw(fd);
> > > +	if (!f)
> > > +		goto error;
> > > +
> > > +	/* For unmountable pseudo filesystem, it seems to have no meaning
> > > +	 * to get their fake paths as they don't have path, and to be no
> > > +	 * way to validate this function pointer can be always safe to call
> > > +	 * in the current context.
> > > +	 */
> > > +	if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname)
> > > +		return -EINVAL;
> 
> An obvious leak here, BTW.

ohh. right.

> Depends.  Which context is it running in?  In particular, which
> locks might be already held?

hard to tell. It will be run out of bpf prog that attaches to kprobe or
tracepoint. What is the concern about locking?
d_path() doesn't take any locks and doesn't depend on any locks. Above 'if'
checks that plain d_path() is used and not some specilized callback with
unknown logic.

> Anyway, what could that be used for?  I mean, if you want to check
> something about syscall arguments, that's an unfixably racy way to go.
> Descriptor table can be a shared data structure, and two consequent
> fdget() on the same number can bloody well yield completely unrelated
> struct file references.

yes. It is racy. There are no guarantees on correctness of FD.
The program can pass arbitrary integer into this helper.

> IOW, anything that does descriptor -> struct file * translation more than
> once is an instant TOCTOU suspect.  In this particular case, the function
> will produce a pathname of something that was once reachable via descriptor
> with this number; quite possibly never before that function had been called
> _and_ not once after it has returned.

Right. TOCTOU is not a concern here. It's tracing. It's ok for full path to be
'one time deal'. Right now people use bpf_probe_read() to replicate what
d_path() does.
See https://github.com/iovisor/bcc/issues/237#issuecomment-547564661
It sort of works, but calling d_path() is simpler and more accurate.
The key thing that bpf helpers need to make sure is that regardless of
how they're called and what integer is passed in as an FD the helper
must not crash or lockup the kernel or cause it to misbehave.
Hence above in_interrupt() and other checks to limit the context.