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  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:   Sun, 14 May 2017 21:00:05 -0400 (EDT)
From:   David Miller <davem@...emloft.net>
To:     daniel@...earbox.net
Cc:     ast@...com, alexei.starovoitov@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH 2/3] bpf: Track alignment of MAP pointers in verifier.

From: Daniel Borkmann <daniel@...earbox.net>
Date: Sun, 14 May 2017 16:31:10 +0200

> On 05/13/2017 04:28 AM, David Miller wrote:
>> @@ -823,10 +825,27 @@ static int check_pkt_ptr_alignment(const struct
>> bpf_reg_state *reg,
>>   }
>>
>>   static int check_val_ptr_alignment(const struct bpf_reg_state *reg,
>> -				   int size, bool strict)
>> +				   int off, int size, bool strict)
>>   {
>> -	if (strict && size != 1) {
>> -		verbose("Unknown alignment. Only byte-sized access allowed in value
>> -		access.\n");
>> +	int reg_off;
>> +
>> +	/* Byte size accesses are always allowed. */
>> +	if (!strict || size == 1)
>> +		return 0;
>> +
>> +	reg_off = reg->off;
>> +	if (reg->id) {
>> +		if (reg->aux_off_align % size) {
>> + verbose("Value access is only %u byte aligned, %d byte access not
>> allowed\n",
>> +				reg->aux_off_align, size);
>> +			return -EACCES;
>> +		}
>> +		reg_off += reg->aux_off;
>> +	}
>
> What are the semantics of using id here? In ptr_to_pkt, we have it,
> so that eventually, in find_good_pkt_pointers() we can match on id
> and update the range for all such regs with the same id. I'm just
> wondering as the side effect of this is that this makes state
> pruning worse.

Ok.  I was advancing reg->id so that it can be used here as the signal
that there is "auxiliary" components to the pointer, and thus we need
to take reg->aux_off_align and reg->aux_off into account.

I did not realize the state pruning component of reg->id so I'll need
to look more deeply into this.

We could use something other than reg->id to decide if there are
variable components to the pointer if necessary.

> Also, reg->off is currently only used in ptr_to_pkt types and
> checked as well in check_packet_access(). Now as semantics change,
> do we need to check for it as well in check_map_access_adj() which
> we currently don't do?

It should not be necessary.  Both before and after my changes we
validate the access range using the reg->min_value and reg->max_value.

>> -		/* a constant was added to pkt_ptr.
>> +		/* a constant was added to the pointer.
>>   		 * Remember it while keeping the same 'id'
>>   		 */
>>   		dst_reg->off += imm;
> 
> Can this now overflow for map type? Also in the UNKNOWN_VALUE case
> below since overflow checks are then only enforced in ptr_to_pkt case?

Indeed, we will have to do "something".  The reg->off used to be u16
and is now a u32 with my changes.  So it can handle something larger
than MAX_PACKET_OFF.

But we still have to handle overflow.  I am not so sure what range of
offsets is reasonable for these MAP pointers, can you make a
suggestion?

> 
>>   	} else {
>> -		bool had_id;
>> -
>> -		if (src_reg->type == PTR_TO_PACKET) {
>> +		if (is_packet && src_reg->type == PTR_TO_PACKET) {
>>   			/* R6=pkt(id=0,off=0,r=62) R7=imm22; r7 += r6 */
>>   			tmp_reg = *dst_reg;  /* save r7 state */
>>   			*dst_reg = *src_reg; /* copy pkt_ptr state r6 into r7 */
> 
> I believe clang could probably generate something similar also for
> map value pointers.

Ok, it should be easy to make that part work both with packet pointers
and MAPs.

Thanks for your feedback, I'll try to refine this some more.

Powered by blists - more mailing lists