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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 Apr 2019 00:31:22 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrey Ignatov <rdna@...com>
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

On 04/03/2019 11:02 PM, Andrey Ignatov wrote:
> Daniel Borkmann <daniel@...earbox.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 <daniel@...earbox.net>
>>> Signed-off-by: Andrey Ignatov <rdna@...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;
>>>  
>>>
>>
>