On 04/03/2019 11:57 PM, Andrey Ignatov wrote:
> Daniel Borkmann <dan...@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 <dan...@iogearbox.net>
>>>> Signed-off-by: Andrey Ignatov <r...@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.
