Re: [PATCH bpf-next 2/5] bpf: reject program if a __user tagged memory accessed in kernel way

[Yonghong Song <yhs@fb.com>]

On 12/19/21 5:45 PM, Alexei Starovoitov wrote:
> On Thu, Dec 09, 2021 at 09:35:48AM -0800, Yonghong Song wrote:
>> BPF verifier supports direct memory access for BPF_PROG_TYPE_TRACING type
>> of bpf programs, e.g., a->b. If "a" is a pointer
>> pointing to kernel memory, bpf verifier will allow user to write
>> code in C like a->b and the verifier will translate it to a kernel
>> load properly. If "a" is a pointer to user memory, it is expected
>> that bpf developer should be bpf_probe_read_user() helper to
>> get the value a->b. Without utilizing BTF __user tagging information,
>> current verifier will assume that a->b is a kernel memory access
>> and this may generate incorrect result.
> 
> The patch set looks great overall.
> 
>> +/* The pointee address space encoded in BTF. */
>> +enum btf_addr_space {
>> +	BTF_ADDRSPACE_UNSPEC	= 0,
>> +	BTF_ADDRSPACE_USER	= 1,
>> +};
>> +
>>   /* The information passed from prog-specific *_is_valid_access
>>    * back to the verifier.
>>    */
>> @@ -473,6 +479,7 @@ struct bpf_insn_access_aux {
>>   		struct {
>>   			struct btf *btf;
>>   			u32 btf_id;
>> +			enum btf_addr_space addr_space;
>>   		};
> ...
>> @@ -4998,7 +4999,15 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>   
>>   	info->btf = btf;
>>   	info->btf_id = t->type;
>> +	info->addr_space = BTF_ADDRSPACE_UNSPEC;
>>   	t = btf_type_by_id(btf, t->type);
>> +
>> +	if (btf_type_is_type_tag(t)) {
>> +		tag_value = __btf_name_by_offset(btf, t->name_off);
>> +		if (strcmp(tag_value, "user") == 0)
>> +			info->addr_space = BTF_ADDRSPACE_USER;
>> +	}
>> +
>>   	/* skip modifiers */
>>   	while (btf_type_is_modifier(t)) {
> 
> bpf_insn_access_aux approach will work only for the first
> pointer deref, right?

That is true.

> Also addr_space will consume the last 4 free bytes in bpf_reg_state.
> Maybe encode user/kernel as a flag into bpf_reg_type?
> The verifier just got extended with PTR_MAYBE_NULL and MEM_RDONLY flags.
> MEM_RDONLY is roughly equivalent 'const' modifier.
> In that sense __user attribute is similar.
> wdyt?

I haven't carefully read Hao's patch set yet. But after roughly going 
through it I think your suggestion should work. Will give a try!