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

[Daniel Borkmann <daniel@iogearbox.net>]

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