netdev - Re: [PATCH bpf-next 1/5] bpf: Reject indirect var

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 4 Apr 2019 01:02:15 +0000
From:   Andrey Ignatov <rdna@...com>
To:     Daniel Borkmann <daniel@...earbox.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>, Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/5] bpf: Reject indirect var_off stack access in
 raw mode

Daniel Borkmann <daniel@...earbox.net> [Wed, 2019-04-03 16:19 -0700]:
> On 04/03/2019 11:57 PM, Andrey Ignatov wrote:
> > Daniel Borkmann <daniel@...earbox.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:
...
> >>>
> >>>>  		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.

You're right of course. I saw how size argument is handled and that
'regno - 1', but for some reason missed it while writing previous
answer.

I was able to write a program that exploits max_off overflow, so yeah,
it is a problem. I'll fix it and send v3. Thanks for catching all these
tricky things!


-- 
Andrey Ignatov