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]

Message-ID: <7892ad45-d74b-6872-4364-333317bcfb1d@iogearbox.net>
Date:   Wed, 3 Apr 2019 18:45:35 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrey Ignatov <rdna@...com>, netdev@...r.kernel.org
Cc:     ast@...nel.org, kernel-team@...com
Subject: Re: [PATCH bpf-next 1/5] bpf: Reject indirect var_off stack access in
 raw mode

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 <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.
> 
>>  		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;
>>  
>>
>