Re: [PATCH bpf-next 05/16] bpf: Support map key with dynptr in verifier

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2024-10-08 at 17:14 +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> The patch basically does the following three things to enable dynptr key
> for bpf map:
> 
> 1) Only allow PTR_TO_STACK typed register for dynptr key
> The main reason is that bpf_dynptr can only be defined in the stack, so
> for dynptr key only PTR_TO_STACK typed register is allowed. bpf_dynptr
> could also be represented by CONST_PTR_TO_DYNPTR typed register (e.g.,
> in callback func or subprog), but it is not supported now.
> 
> 2) Only allow fixed-offset for PTR_TO_STACK register
> Variable-offset for PTR_TO_STACK typed register is disallowed, because
> it is impossible to check whether or not the stack access is aligned
> with BPF_REG_SIZE and is matched with the location of dynptr or
> non-dynptr part in the map key.
> 
> 3) Check the layout of the stack content is matched with the btf_record
> Firstly check the start offset of the stack access is aligned with
> BPF_REG_SIZE, then check the offset and the size of dynptr/non-dynptr
> parts in the stack content is consistent with the btf_record of the map
> key.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---

The logic of this patch looks correct, however I find it cumbersome.
The only place where access to dynptr key is allowed is 'case ARG_PTR_TO_MAP_KEY'
in check_func_arg(), a lot of places are modified to facilitate this.
It seems that logic would be easier to follow if there would be a
dedicated function to check dynptr key constraints, called only for
the 'case ARG_PTR_TO_MAP_KEY'. This would als make 'struct dynptr_key_state'
unnecessary as this state would be tracked inside such function.
Wdyt?

[...]