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?
>> @@ -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;
>>
>>
>
