Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

From: Martin KaFai Lau <kafai@fb.com>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc
Date: Wed, 2 Mar 2022 15:31:41 -0800	[thread overview]
Message-ID: <20220302233141.qob5kwnkrtwnt6qf@kafai-mbp> (raw)
In-Reply-To: <20220302224418.5ph7nkzx2qmcy36n@ast-mbp.dhcp.thefacebook.com>

On Wed, Mar 02, 2022 at 02:44:18PM -0800, Alexei Starovoitov wrote:
> > So IIUC what you're saying is that once someone performs increment, we reset the
> > ref_obj_id to 0, then the reference state is still present so
> > check_reference_leak would complain, but releasing such modified register won't
> > work since ref_obj_id is 0 (so no ref state for that ref_obj_id).
I meant error similar to release_reference() should be used and it
should be treated as release-without-prior-acquire error.  The reg
is pointing at something that its reference has not been acquired
before.

You are right, reset ref_obj_id to 0 during increment won't work
as you have explained in the following.

> > 
> > But I think clang (or even user writing BPF ASM) would be well within its rights
> > to temporarily add an offset to the register, pass member pointer to some other
> > helper, or read some data, and then decrement it again to shift the pointer
> > backwards setting reg->off to 0. Then they should be able to again pass such
> > register to release helper or kfunc. I think it would be unlikely (you can save
> > original pointer to caller saved reg, or spill to stack, or use offset in LDX,
> > etc.) but certainly not impossible.
> 
> I don't think llvm will ever do such thing. Passing into a helper means
> that the register is scratched. It won't be reused after the call.
> Saving modified into a stack to restore later just to do a math on it
> goes against "optimization" goal of the compiler.
> 
> > I think the key point is that we want to make user pass the register as it was
> > when it was acquired, they can do any changes to off between acquire and
> > release, just that it should be set back to 0 when release function is called.
> 
> Correct and this patch is covering that.
> I'm not sure what is the contention point here.
> Sorry I'm behind the mailing list.
> 
> > > >
> > > > Again, given we can only pass one referenced reg, if we see release func and a
> > > > reg with ref_obj_id, it is the one being released.
> > > >
> > > > In the end, it's more of a preference thing, if you feel strongly about it I can
> > > > go with the __check_ptr_off_reg call too.
> > > Yeah, it is a preference thing and not feeling strongly.
> > > Without the need for the release-func/reg->off preemptive fix, adding
> > > one __check_ptr_off_reg() seems to be a cleaner fix to me but
> > > I won't insist.
> 
> fwiw I like patches 1-3.
> I think extra check here for release func is justified on its own.
> Converting it into:
>   fixed_off_ok = false;
>   if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
>           fixed_off_ok = true;
> obfuscates the check to me.
> if (rel && reg->off) check
> is pretty obvious.
Yeah, I am fine with an extra check and the "must have zero offset
message when passed to release kfunc".  The error is obvious enough
on what may be wrong in the bpf prog.