On 04/03/2019 11:02 PM, Andrey Ignatov wrote:
> Daniel Borkmann <dan...@iogearbox.net> [Wed, 2019-04-03 09:22 -0700]:
>> 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.
>
> Yeah, I think this will work.
>
> This will change the logic a bit though.
>
> E.g. logic in this patch will deny variable offset stack access to
> ARG_PTR_TO_UNINIT_MEM no matter if corresponding stack memory is
> initialized or not.
>
> But with `meta = NULL` verifier will accept access to
> ARG_PTR_TO_UNINIT_MEM on stack if that part of the stack is fully
> initialized for all possible offsets.
>
> I think the latter should be fine since if all possible bytes that can
> be accessed are already initialized then there should not be problem on
> return from the helper.
>
> I'll switch to `meta = NULL` in v3. Though given the difference in the
> logic, let me know if you prefer to keep the one in this patch. Thanks.
Yes I know, I mentioned it in my email wrt more flexibility, but probably
not communicated clear enough. I think that's totally fine.
>>> min_off = reg->smin_value + reg->off;
>>> max_off = reg->umax_value + reg->off;
>>> err = __check_stack_boundary(env, regno, min_off, access_size,
>>> @@ -2225,12 +2244,6 @@ static int check_stack_boundary(struct
>>> bpf_verifier_env *env, int regno,
>>> return err;
>>> }
>>>
>>> - if (meta && meta->raw_mode) {
>>> - meta->access_size = access_size;
>>> - meta->regno = regno;
>>> - return 0;
>>> - }
>>
>> This can then also stay as-is.
>>
>>> for (i = min_off; i < max_off + access_size; i++) {
>>> u8 *stype;
>>>
>>>
>>
>
