Re: [PATCH bpf-next 1/5] bpf: Reject indirect var_off stack access in raw mode

[Daniel Borkmann <daniel@iogearbox.net>]

On 04/03/2019 11:57 PM, Andrey Ignatov wrote:
> Daniel Borkmann <daniel@iogearbox.net> [Wed, 2019-04-03 09:46 -0700]:
>> On 04/03/2019 06:21 PM, Daniel Borkmann wrote:
>>> On 04/02/2019 10:19 PM, Andrey Ignatov wrote:
>>>> It's hard to guarantee that whole memory is marked as initialized on
>>>> helper return if uninitialized stack is accessed with variable offset
>>>> since specific bounds are unknown to verifier. This may cause
>>>> uninitialized stack leaking.
>>>>
>>>> Reject such an access in check_stack_boundary to prevent possible
>>>> leaking.
>>>>
>>>> There are no known use-cases for indirect uninitialized stack access
>>>> with variable offset so it shouldn't break anything.
>>>>
>>>> Fixes: 2011fccfb61b ("bpf: Support variable offset stack access from helpers")
>>>> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
>>>> Signed-off-by: Andrey Ignatov <rdna@fb.com>
>>>> ---
>>>>  kernel/bpf/verifier.c | 25 +++++++++++++++++++------
>>>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index b7a7a9caa82f..12b84307ffa8 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -2212,7 +2212,26 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
>>>>  					     zero_size_allowed);
>>>>  		if (err)
>>>>  			return err;
>>>> +		if (meta && meta->raw_mode) {
>>>> +			meta->access_size = access_size;
>>>> +			meta->regno = regno;
>>>> +			return 0;
>>>> +		}
>>>>  	} else {
>>>> +		/* Only initialized buffer on stack is allowed to be accessed
>>>> +		 * with variable offset. With uninitialized buffer it's hard to
>>>> +		 * guarantee that whole memory is marked as initialized on
>>>> +		 * helper return since specific bounds are unknown what may
>>>> +		 * cause uninitialized stack leaking.
>>>> +		 */
>>>> +		if (meta && meta->raw_mode) {
>>>> +			char tn_buf[48];
>>>> +
>>>> +			tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
>>>> +			verbose(env, "R%d invalid indirect access to uninitialized stack var_off=%s\n",
>>>> +				regno, tn_buf);
>>>> +			return -EACCES;
>>>> +		}
>>>
>>> Hmm, I think we should probably handle this in similar way like we do
>>> in case of variable stack access when it comes to stack size:
>>>
>>>                if (!tnum_is_const(reg->var_off))
>>>                         /* For unprivileged variable accesses, disable raw
>>>                          * mode so that the program is required to
>>>                          * initialize all the memory that the helper could
>>>                          * just partially fill up.
>>>                          */
>>>                         meta = NULL;
>>>
>>> So we error out naturally on the loop later where we also mark for
>>> liveness, and also allow for more flexibility if we know stack must
>>> already be initialized in this range.
>>>
>>>>  		min_off = reg->smin_value + reg->off;
>>>>  		max_off = reg->umax_value + reg->off;
>>>>  		err = __check_stack_boundary(env, regno, min_off, access_size,
>>
>> Btw, shouldn't above two additions be sanity checked for wrap-around
>> resp. truncation?
> 
> Good question.
> 
> As I can see, both reg->smin_value and reg->off are checked by
> check_reg_sane_offset() in adjust_ptr_min_max_vals() that handles
> pointer arithmetics. And I don't know how to come up with variable
> offset w/o pointer arithmetics, i.e. these both should be in
> (-BPF_MAX_VAR_OFF; BPF_MAX_VAR_OFF).
> 
> As for reg->umax_value, I see that it's checked in check_func_arg()
> before calling to check_helper_mem_access() (that in turn calls to
> check_stack_boundary()):
> 
> 		if (reg->umax_value >= BPF_MAX_VAR_SIZ) {
> 			verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n",
>                         	regno);
> 			return -EACCES;
> 		}
> 
> So my understanding is with all these checks that happen beforehand,
> there should not be overflow and int is used for offset in both the old
> code, that handles constant offset, and this new code for variable
> offset.

The latter one is on the reg with size argument, not on the reg with pointer
to stack. check_helper_mem_access() calls 'regno - 1' for the one where the
register holds the pointer to stack value.