* Re: bpf: allow bpf programs to tail-call other bpf programs
2015-05-23 17:33 bpf: allow bpf programs to tail-call other bpf programs Dan Carpenter
@ 2015-05-26 16:01 ` Alexei Starovoitov
2015-05-26 16:16 ` Daniel Borkmann
2015-05-26 16:30 ` Alexei Starovoitov
2 siblings, 0 replies; 4+ messages in thread
From: Alexei Starovoitov @ 2015-05-26 16:01 UTC (permalink / raw)
To: kernel-janitors
On 5/23/15 10:33 AM, Dan Carpenter wrote:
> Hello Alexei Starovoitov,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 04fd61ab36ec: "bpf: allow bpf programs to tail-call other
> bpf programs" from May 19, 2015, leads to the following Smatch
> complaint:
>
> kernel/bpf/verifier.c:921 check_call()
> error: we previously assumed 'map' could be null (see line 911)
>
> kernel/bpf/verifier.c
> 910
> 911 if (map && map->map_type = BPF_MAP_TYPE_PROG_ARRAY &&
> ^^^
> Patch introduces a check for NULL.
>
> 912 func_id != BPF_FUNC_tail_call)
> 913 /* prog_array map type needs extra care:
> 914 * only allow to pass it into bpf_tail_call() for now.
> 915 * bpf_map_delete_elem() can be allowed in the future,
> 916 * while bpf_map_update_elem() must only be done via syscall
> 917 */
> 918 return -EINVAL;
> 919
> 920 if (func_id = BPF_FUNC_tail_call &&
> 921 map->map_type != BPF_MAP_TYPE_PROG_ARRAY)
> ^^^^^^^^^^^^^
> New unchecked dereference.
that is false positive.
See that 'if (func_id = BPF_FUNC_tail_call ...)' is done before
map->map_type dereference.
tail_call_proto has:
.arg2_type = ARG_CONST_MAP_PTR
so call at line 869:
check_func_arg(env, BPF_REG_2, fn->arg2_type, &map);
will check that arg2 is map_ptr and will assign valid map pointer
into map variable.
So if line 920 is true, then line 869 was ok as well and map
pointer is valid. No need for extra check at line 921.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: bpf: allow bpf programs to tail-call other bpf programs
2015-05-23 17:33 bpf: allow bpf programs to tail-call other bpf programs Dan Carpenter
2015-05-26 16:01 ` Alexei Starovoitov
@ 2015-05-26 16:16 ` Daniel Borkmann
2015-05-26 16:30 ` Alexei Starovoitov
2 siblings, 0 replies; 4+ messages in thread
From: Daniel Borkmann @ 2015-05-26 16:16 UTC (permalink / raw)
To: kernel-janitors
On 05/26/2015 06:01 PM, Alexei Starovoitov wrote:
> On 5/23/15 10:33 AM, Dan Carpenter wrote:
>> Hello Alexei Starovoitov,
>>
>> This is a semi-automatic email about new static checker warnings.
>>
>> The patch 04fd61ab36ec: "bpf: allow bpf programs to tail-call other
>> bpf programs" from May 19, 2015, leads to the following Smatch
>> complaint:
>>
>> kernel/bpf/verifier.c:921 check_call()
>> error: we previously assumed 'map' could be null (see line 911)
>>
>> kernel/bpf/verifier.c
>> 910
>> 911 if (map && map->map_type = BPF_MAP_TYPE_PROG_ARRAY &&
>> ^^^
>> Patch introduces a check for NULL.
>>
>> 912 func_id != BPF_FUNC_tail_call)
>> 913 /* prog_array map type needs extra care:
>> 914 * only allow to pass it into bpf_tail_call() for now.
>> 915 * bpf_map_delete_elem() can be allowed in the future,
>> 916 * while bpf_map_update_elem() must only be done via syscall
>> 917 */
>> 918 return -EINVAL;
>> 919
>> 920 if (func_id = BPF_FUNC_tail_call &&
>> 921 map->map_type != BPF_MAP_TYPE_PROG_ARRAY)
>> ^^^^^^^^^^^^^
>> New unchecked dereference.
>
> that is false positive.
> See that 'if (func_id = BPF_FUNC_tail_call ...)' is done before
> map->map_type dereference.
> tail_call_proto has:
> .arg2_type = ARG_CONST_MAP_PTR
> so call at line 869:
> check_func_arg(env, BPF_REG_2, fn->arg2_type, &map);
> will check that arg2 is map_ptr and will assign valid map pointer
> into map variable.
> So if line 920 is true, then line 869 was ok as well and map
> pointer is valid. No need for extra check at line 921.
A bit hard to parse, perhaps. The control flow might have been easier
to understand, if it would have looked like:
if (!map)
/* Nothing to check further. */
return 0;
if (map->map_type = BPF_MAP_TYPE_PROG_ARRAY &&
func_id != BPF_FUNC_tail_call)
return -EINVAL;
if (map->map_type != BPF_MAP_TYPE_PROG_ARRAY &&
func_id = BPF_FUNC_tail_call)
return -EINVAL;
return 0;
Cheers,
Daniel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: bpf: allow bpf programs to tail-call other bpf programs
2015-05-23 17:33 bpf: allow bpf programs to tail-call other bpf programs Dan Carpenter
2015-05-26 16:01 ` Alexei Starovoitov
2015-05-26 16:16 ` Daniel Borkmann
@ 2015-05-26 16:30 ` Alexei Starovoitov
2 siblings, 0 replies; 4+ messages in thread
From: Alexei Starovoitov @ 2015-05-26 16:30 UTC (permalink / raw)
To: kernel-janitors
On 5/26/15 9:16 AM, Daniel Borkmann wrote:
> On 05/26/2015 06:01 PM, Alexei Starovoitov wrote:
>> On 5/23/15 10:33 AM, Dan Carpenter wrote:
>>> Hello Alexei Starovoitov,
>>>
>>> This is a semi-automatic email about new static checker warnings.
>>>
>>> The patch 04fd61ab36ec: "bpf: allow bpf programs to tail-call other
>>> bpf programs" from May 19, 2015, leads to the following Smatch
>>> complaint:
>>>
>>> kernel/bpf/verifier.c:921 check_call()
>>> error: we previously assumed 'map' could be null (see line 911)
>>>
>>> kernel/bpf/verifier.c
>>> 910
>>> 911 if (map && map->map_type = BPF_MAP_TYPE_PROG_ARRAY &&
>>> ^^^
>>> Patch introduces a check for NULL.
>>>
>>> 912 func_id != BPF_FUNC_tail_call)
>>> 913 /* prog_array map type needs extra care:
>>> 914 * only allow to pass it into bpf_tail_call() for
>>> now.
>>> 915 * bpf_map_delete_elem() can be allowed in the
>>> future,
>>> 916 * while bpf_map_update_elem() must only be done
>>> via syscall
>>> 917 */
>>> 918 return -EINVAL;
>>> 919
>>> 920 if (func_id = BPF_FUNC_tail_call &&
>>> 921 map->map_type != BPF_MAP_TYPE_PROG_ARRAY)
>>> ^^^^^^^^^^^^^
>>> New unchecked dereference.
>>
>> that is false positive.
>> See that 'if (func_id = BPF_FUNC_tail_call ...)' is done before
>> map->map_type dereference.
>> tail_call_proto has:
>> .arg2_type = ARG_CONST_MAP_PTR
>> so call at line 869:
>> check_func_arg(env, BPF_REG_2, fn->arg2_type, &map);
>> will check that arg2 is map_ptr and will assign valid map pointer
>> into map variable.
>> So if line 920 is true, then line 869 was ok as well and map
>> pointer is valid. No need for extra check at line 921.
>
> A bit hard to parse, perhaps. The control flow might have been easier
> to understand, if it would have looked like:
>
> if (!map)
> /* Nothing to check further. */
> return 0;
> if (map->map_type = BPF_MAP_TYPE_PROG_ARRAY &&
> func_id != BPF_FUNC_tail_call)
> return -EINVAL;
>
> if (map->map_type != BPF_MAP_TYPE_PROG_ARRAY &&
> func_id = BPF_FUNC_tail_call)
> return -EINVAL;
not really. map = NULL is valid case.
Currently the existing two 'if' are logically checking two
different conditions. Splitting them into 3 like above
makes me cringe a little, since the checking logic is not
contained. I agree that the above _looks_ nicer, but it
feels wrong to me, since checks are split up this way.
^ permalink raw reply [flat|nested] 4+ messages in thread