linux-kernel - Re: [PATCH v4 bpf-next 08/14] bpf: introduce the bpf_get_local

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <82abfd03-1023-e785-a3ba-c63788acc659@iogearbox.net>
Date:   Wed, 1 Aug 2018 22:15:57 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Roman Gushchin <guro@...com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-team@...com, Alexei Starovoitov <ast@...nel.org>
Subject: Re: [PATCH v4 bpf-next 08/14] bpf: introduce the
 bpf_get_local_storage() helper function

On 08/01/2018 02:28 AM, Roman Gushchin wrote:
> On Wed, Aug 01, 2018 at 12:50:16AM +0200, Daniel Borkmann wrote:
>> On 07/27/2018 11:52 PM, Roman Gushchin wrote:
>> [...]
>>> @@ -2533,6 +2541,16 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>>>  	}
>>>  
>>>  	regs = cur_regs(env);
>>> +
>>> +	/* check that flags argument in get_local_storage(map, flags) is 0,
>>> +	 * this is required because get_local_storage() can't return an error.
>>> +	 */
>>> +	if (func_id == BPF_FUNC_get_local_storage &&
>>> +	    !tnum_equals_const(regs[BPF_REG_2].var_off, 0)) {
>>> +		verbose(env, "get_local_storage() doesn't support non-zero flags\n");
>>> +		return -EINVAL;
>>> +	}
>>
>> Hmm, this check is actually not correct. You will still be able to pass non-zero
>> values in there. arg2_type from the helper is ARG_ANYTHING, so the register type
>> could for example be one of the pointer types and it will still pass the verifier.
>> The correct way to check would be to use register_is_null().
>>
>>> +
>>>  	/* reset caller saved regs */
>>>  	for (i = 0; i < CALLER_SAVED_REGS; i++) {
>>>  		mark_reg_not_init(env, regs, caller_saved[i]);
> 
> Oh, perfect catch!
> The diff is below. Please, let me know if you prefer me to resend
> the whole patch/patchset.

Yeah, please resend at that point. There are also some other minor things which
would be great if you could roll them in as well in a respin along with this fix
and the uapi helper description adjustment with test case fix:

- patch 1: bpf_map_release_memlock() should also use bpf_uncharge_memlock() directly
- patch 2: cgroup_storage_map_alloc() only checks attr->key_size and attr->value_size
  but what about attr->max_entries and attr->map_flags? Should attr->max_entries be
  forced to 0 and at least attr->map_flags that don't make any sense in this context
  get rejected on map creation?
- patch 9: not all uapi changes were copied over into tools' uapi header

Thanks,
Daniel