Re: [PATCH net] bpf: fix ri->map prog pointer on bpf_prog_realloc

[Daniel Borkmann <daniel@iogearbox.net>]

On 09/19/2017 03:43 AM, Alexei Starovoitov wrote:
> On Tue, Sep 19, 2017 at 03:16:44AM +0200, Daniel Borkmann wrote:
>> Commit 109980b894e9 ("bpf: don't select potentially stale
>> ri->map from buggy xdp progs") passed the pointer to the prog
>> itself to be loaded into r4 prior on bpf_redirect_map() helper
>> call, so that we can store the owner into ri->map_owner out of
>> the helper.
>>
>> Issue with that is that the actual address of the prog is still
>> subject to change when subsequent rewrites occur, e.g. through
>> patching other helper functions or constant blinding. Thus, we
>> really need to take prog->aux as the address we're holding, and
>> then during runtime fetch the actual pointer via aux->prog. This
>> also works with prog clones as they share the same aux and fixup
>> pointer to self after blinding finished.
>>
>> Fixes: 109980b894e9 ("bpf: don't select potentially stale ri->map from buggy xdp progs")
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>>   kernel/bpf/verifier.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 799b245..243c09f 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -4205,9 +4205,17 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
>>   		}
>>
>>   		if (insn->imm == BPF_FUNC_redirect_map) {
>> -			u64 addr = (unsigned long)prog;
>> +			/* Note, we cannot use prog directly as imm as subsequent
>> +			 * rewrites would still change the prog pointer. The only
>> +			 * stable address we can use is aux, which also works with
>> +			 * prog clones during blinding.
>> +			 */
>
> good catch. extra load at runtime sucks, but I don't see better solution.
>
>> +			u64 addr = (unsigned long)prog->aux;
>> +			const int r4 = BPF_REG_4;
>>   			struct bpf_insn r4_ld[] = {
>> -				BPF_LD_IMM64(BPF_REG_4, addr),
>> +				BPF_LD_IMM64(r4, addr),
>> +				BPF_LDX_MEM(BPF_DW, r4, r4,
>> +					    offsetof(struct bpf_prog_aux, prog)),
>
> needs to be BPF_FIELD_SIZEOF(struct bpf_prog_aux, prog) to work on 32-bit

Good point, will spin a v2. Thanks!