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