[2/3] bpf: Track alignment of MAP pointers in verifier.
diff mbox

Message ID 20170512.222856.46437864475991511.davem@davemloft.net
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

David Miller May 13, 2017, 2:28 a.m. UTC
Just like packet pointers, track the known alignment of MAP pointers.

In order to facilitate the state tracking, move the register offset
field into where there is an unused 32-bit padding slot on 64-bit.

The check logic is the same as for packet pointers, except we do not
apply NET_IP_ALIGN to the calculations.

Also, there are several restrictions that apply to packet pointers
which we do not extend to MAP pointers.  For example, the
MAX_PACKET_OFF limitation and the "adding integer with < 48 upper zero
bits" thing.

When we add a variable to the MAP pointer, all of the state
transitions are identical except that we elide the reg->range clear
because it is a packet pointer specific piece of state.

This changes the string emitted when an unaligned access is trapped by
the verifier.  Therefore, we need to adjust the search string used by
test_verifier.c

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 include/linux/bpf_verifier.h                |   6 +-
 kernel/bpf/verifier.c                       | 112 +++++++++++++++++++---------
 tools/testing/selftests/bpf/test_verifier.c |   3 +-
 3 files changed, 80 insertions(+), 41 deletions(-)

Comments

Daniel Borkmann May 14, 2017, 2:31 p.m. UTC | #1
On 05/13/2017 04:28 AM, David Miller wrote:
>
> Just like packet pointers, track the known alignment of MAP pointers.
>
> In order to facilitate the state tracking, move the register offset
> field into where there is an unused 32-bit padding slot on 64-bit.
>
> The check logic is the same as for packet pointers, except we do not
> apply NET_IP_ALIGN to the calculations.
>
> Also, there are several restrictions that apply to packet pointers
> which we do not extend to MAP pointers.  For example, the
> MAX_PACKET_OFF limitation and the "adding integer with < 48 upper zero
> bits" thing.
>
> When we add a variable to the MAP pointer, all of the state
> transitions are identical except that we elide the reg->range clear
> because it is a packet pointer specific piece of state.
>
> This changes the string emitted when an unaligned access is trapped by
> the verifier.  Therefore, we need to adjust the search string used by
> test_verifier.c
>
> Signed-off-by: David S. Miller <davem@davemloft.net>

(Sorry for late reply, still thinking/reviewing on it.)

[...]
> @@ -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.

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?

> +	if ((reg_off + off) % size != 0) {
> +		verbose("misaligned value access off %d+%d size %d\n",
> +			reg_off, off, size);
>   		return -EACCES;
>   	}
>
> @@ -846,7 +865,7 @@ static int check_ptr_alignment(struct bpf_verifier_env *env,
>   	case PTR_TO_PACKET:
>   		return check_pkt_ptr_alignment(reg, off, size, strict);
>   	case PTR_TO_MAP_VALUE_ADJ:
> -		return check_val_ptr_alignment(reg, size, strict);
> +		return check_val_ptr_alignment(reg, off, size, strict);
>   	default:
>   		if (off % size != 0) {
>   			verbose("misaligned access off %d size %d\n",
[...]
> -static int check_packet_ptr_add(struct bpf_verifier_env *env,
> -				struct bpf_insn *insn)
> +static int check_pointer_add(struct bpf_verifier_env *env,
> +			     struct bpf_insn *insn, bool is_packet)
>   {
>   	struct bpf_reg_state *regs = env->cur_state.regs;
>   	struct bpf_reg_state *dst_reg = &regs[insn->dst_reg];
> @@ -1468,28 +1489,28 @@ static int check_packet_ptr_add(struct bpf_verifier_env *env,
>   	s32 imm;
>
>   	if (BPF_SRC(insn->code) == BPF_K) {
> -		/* pkt_ptr += imm */
> +		/* pointer += imm */
>   		imm = insn->imm;
>
>   add_imm:
> -		if (imm < 0) {
> -			verbose("addition of negative constant to packet pointer is not allowed\n");
> -			return -EACCES;
> -		}
> -		if (imm >= MAX_PACKET_OFF ||
> -		    imm + dst_reg->off >= MAX_PACKET_OFF) {
> -			verbose("constant %d is too large to add to packet pointer\n",
> -				imm);
> -			return -EACCES;
> +		if (is_packet) {
> +			if (imm < 0) {
> +				verbose("addition of negative constant to packet pointer is not allowed\n");
> +				return -EACCES;
> +			}
> +			if (imm >= MAX_PACKET_OFF ||
> +			    imm + dst_reg->off >= MAX_PACKET_OFF) {
> +				verbose("constant %d is too large to add to packet pointer\n",
> +					imm);
> +				return -EACCES;
> +			}
>   		}
> -		/* 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?

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

> @@ -1503,46 +1524,62 @@ static int check_packet_ptr_add(struct bpf_verifier_env *env,
>   		}
>
>   		if (src_reg->type == CONST_IMM) {
> -			/* pkt_ptr += reg where reg is known constant */
> +			/* pointer += reg where reg is known constant */
>   			imm = src_reg->imm;
>   			goto add_imm;
>   		}
> -		/* disallow pkt_ptr += reg
> +		/* disallow pointer += reg
>   		 * if reg is not uknown_value with guaranteed zero upper bits
> -		 * otherwise pkt_ptr may overflow and addition will become
> +		 * otherwise pointer_ptr may overflow and addition will become
>   		 * subtraction which is not allowed
>   		 */
>   		if (src_reg->type != UNKNOWN_VALUE) {
> -			verbose("cannot add '%s' to ptr_to_packet\n",
> +			verbose("cannot add '%s' to pointer\n",
>   				reg_type_str[src_reg->type]);
>   			return -EACCES;
>   		}
> -		if (src_reg->imm < 48) {
> +		if (is_packet && src_reg->imm < 48) {
>   			verbose("cannot add integer value with %lld upper zero bits to ptr_to_packet\n",
>   				src_reg->imm);
>   			return -EACCES;
>   		}
>
> -		had_id = (dst_reg->id != 0);
> -
> -		/* dst_reg stays as pkt_ptr type and since some positive
> +		/* dst_reg stays as the same type and since some positive
>   		 * integer value was added to the pointer, increment its 'id'
>   		 */
>   		dst_reg->id = ++env->id_gen;
>
> -		/* something was added to pkt_ptr, set range to zero */
>   		dst_reg->aux_off += dst_reg->off;
>   		dst_reg->off = 0;
> -		dst_reg->range = 0;
> -		if (had_id)
> +
> +		if (is_packet) {
> +			/* something was added to packet ptr, set range to zero */
> +			dst_reg->range = 0;
> +		}
> +		if (dst_reg->aux_off_align) {
>   			dst_reg->aux_off_align = min(dst_reg->aux_off_align,
David Miller May 15, 2017, 1 a.m. UTC | #2
From: Daniel Borkmann <daniel@iogearbox.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.
Daniel Borkmann May 15, 2017, 1:10 p.m. UTC | #3
On 05/15/2017 03:00 AM, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.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?

The worst-case maximum allowed value size is currently at KMALLOC_MAX_SIZE
(see array_map_alloc()), so we might need to take that one into account.

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

Ok, thanks!
David Miller May 15, 2017, 3:34 p.m. UTC | #4
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Mon, 15 May 2017 15:10:02 +0200

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

Daniel, I looked at the state pruning for maps.  The situation is
quite interesting.

Once we have env->varlen_map_value_access (and load or store via a
PTR_TO_MAP_VALUE_ADJ pointer), the state pruning gets more strict, the
relevant tests are:

		if (memcmp(rold, rcur, sizeof(*rold)) == 0)
			continue;

		/* If the ranges were not the same, but everything else was and
		 * we didn't do a variable access into a map then we are a-ok.
		 */
		if (!varlen_map_access &&
		    memcmp(rold, rcur, offsetofend(struct bpf_reg_state, id)) == 0)
			continue;

The first memcmp() is not going to match any time we adjust any
component of a MAP pointer reg.  The offset, the alignment, anything.
That means any side effect whatsoever performed by check_pointer_add()
even if we changed the code to not modify reg->id

The second check elides:

	s64 min_value;
	u64 max_value;
	u32 min_align;
	u32 aux_off;
	u32 aux_off_align;

from the comparison but only if we haven't done a variable length
MAP access.

The only conclusion I can come to is that changing reg->id for
PTR_TO_MAP_VALUE_ADJ has no side effect for state pruning, unless we
perform PTR_TO_MAP_VALUE_ADJ register adjustments without ever
accessing the map via that pointer in the entire program.

I could add some new state to avoid the reg->id change, but given
the above I don't think that it is really necessary.
Daniel Borkmann May 15, 2017, 9:55 p.m. UTC | #5
On 05/15/2017 05:34 PM, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Mon, 15 May 2017 15:10:02 +0200
>
>>>> 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.
>
> Daniel, I looked at the state pruning for maps.  The situation is
> quite interesting.
>
> Once we have env->varlen_map_value_access (and load or store via a
> PTR_TO_MAP_VALUE_ADJ pointer), the state pruning gets more strict, the
> relevant tests are:
>
> 		if (memcmp(rold, rcur, sizeof(*rold)) == 0)
> 			continue;
>
> 		/* If the ranges were not the same, but everything else was and
> 		 * we didn't do a variable access into a map then we are a-ok.
> 		 */
> 		if (!varlen_map_access &&
> 		    memcmp(rold, rcur, offsetofend(struct bpf_reg_state, id)) == 0)
> 			continue;
>
> The first memcmp() is not going to match any time we adjust any
> component of a MAP pointer reg.  The offset, the alignment, anything.
> That means any side effect whatsoever performed by check_pointer_add()
> even if we changed the code to not modify reg->id
>
> The second check elides:
>
> 	s64 min_value;
> 	u64 max_value;
> 	u32 min_align;
> 	u32 aux_off;
> 	u32 aux_off_align;
>
> from the comparison but only if we haven't done a variable length
> MAP access.

I'm actually wondering about the min_align/aux_off/aux_off_align and
given this is not really related to varlen_map_access and we currently
just skip this.

We should make sure that when env->strict_alignment is false that we
ignore any difference in min_align/aux_off/aux_off_align, afaik, the
min_align would also be set on regs other than ptr_to_pkt.

What about compare_ptrs_to_packet() for when env->strict_alignment is
true in ptr_to_pkt case? Could we have a situation that prunes the search
with matching the third test? Say, in the old case, we did go all the
way and ...

   R3(off=0, r=0)
   R4 = R3 + 20   // AAA
   // now R4(off=20,r=0)
   if (R4 > data_end)
       got out;
   // BBB: now R4(off=20,r=20), R3(off=0,r=20) and R3 used to access

... verify the code under 'BBB' and found that it's safe to run,
including alignment, etc. Next time we come to this branch through
R4 = R3 + 33 (under AAA), so we have R4(off=33,r=0). What happens
if we then do R4-=4, and access 4 bytes of the packet?

The old R4(off=20,r=20) becomes R4(off=16,r=20), which was found
safe and the new R4(off=33,r=0) becomes R4(off=29,r=33) which
would end up being unaligned? Looks like we shouldn't prune in
such case? Maybe test_verifier test case helps to visualize.

> The only conclusion I can come to is that changing reg->id for
> PTR_TO_MAP_VALUE_ADJ has no side effect for state pruning, unless we
> perform PTR_TO_MAP_VALUE_ADJ register adjustments without ever
> accessing the map via that pointer in the entire program.

Why entire program, just between two state pruning points, no?
(They are marked as STATE_LIST_MARK.)

> I could add some new state to avoid the reg->id change, but given
> the above I don't think that it is really necessary.
David Miller May 16, 2017, 7:08 p.m. UTC | #6
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Mon, 15 May 2017 23:55:47 +0200

> I'm actually wondering about the min_align/aux_off/aux_off_align and
> given this is not really related to varlen_map_access and we currently
> just skip this.
> 
> We should make sure that when env->strict_alignment is false that we
> ignore any difference in min_align/aux_off/aux_off_align, afaik, the
> min_align would also be set on regs other than ptr_to_pkt.

Ok I see what you are saying, alignment related register state has to
be taken into consideration during pruning but only when
env->strict_alignment is true.

->min_align is set on any register upon which a calculation is
performed.

> What about compare_ptrs_to_packet() for when env->strict_alignment is
> true in ptr_to_pkt case?

Yes we need to do something there, and yes we do need testcases.

You also remind me that I was thinking about whether we should
propagate alignment state through branches.  For example on
the taken path of a JEQ we can set both arms of the test to
have the largest of the two arms alignment.

Patch
diff mbox

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index d5093b5..acf711b 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -18,15 +18,13 @@ 
 
 struct bpf_reg_state {
 	enum bpf_reg_type type;
+	u32 off;
 	union {
 		/* valid when type == CONST_IMM | PTR_TO_STACK | UNKNOWN_VALUE */
 		s64 imm;
 
 		/* valid when type == PTR_TO_PACKET* */
-		struct {
-			u16 off;
-			u16 range;
-		};
+		u32 range;
 
 		/* valid when type == CONST_PTR_TO_MAP | PTR_TO_MAP_VALUE |
 		 *   PTR_TO_MAP_VALUE_OR_NULL
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0f33db0..b187815 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -231,10 +231,10 @@  static void print_verifier_state(struct bpf_verifier_state *state)
 		else if (t == CONST_PTR_TO_MAP || t == PTR_TO_MAP_VALUE ||
 			 t == PTR_TO_MAP_VALUE_OR_NULL ||
 			 t == PTR_TO_MAP_VALUE_ADJ)
-			verbose("(ks=%d,vs=%d,id=%u)",
+			verbose("(ks=%d,vs=%d,id=%u,off=%u)",
 				reg->map_ptr->key_size,
 				reg->map_ptr->value_size,
-				reg->id);
+				reg->id, reg->off);
 		if (reg->min_value != BPF_REGISTER_MIN_RANGE)
 			verbose(",min_value=%lld",
 				(long long)reg->min_value);
@@ -469,6 +469,7 @@  static void init_reg_state(struct bpf_reg_state *regs)
 
 	for (i = 0; i < MAX_BPF_REG; i++) {
 		regs[i].type = NOT_INIT;
+		regs[i].off = 0;
 		regs[i].imm = 0;
 		regs[i].min_value = BPF_REGISTER_MIN_RANGE;
 		regs[i].max_value = BPF_REGISTER_MAX_RANGE;
@@ -487,6 +488,7 @@  static void init_reg_state(struct bpf_reg_state *regs)
 static void __mark_reg_unknown_value(struct bpf_reg_state *regs, u32 regno)
 {
 	regs[regno].type = UNKNOWN_VALUE;
+	regs[regno].off = 0;
 	regs[regno].id = 0;
 	regs[regno].imm = 0;
 }
@@ -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;
+	}
+
+	if ((reg_off + off) % size != 0) {
+		verbose("misaligned value access off %d+%d size %d\n",
+			reg_off, off, size);
 		return -EACCES;
 	}
 
@@ -846,7 +865,7 @@  static int check_ptr_alignment(struct bpf_verifier_env *env,
 	case PTR_TO_PACKET:
 		return check_pkt_ptr_alignment(reg, off, size, strict);
 	case PTR_TO_MAP_VALUE_ADJ:
-		return check_val_ptr_alignment(reg, size, strict);
+		return check_val_ptr_alignment(reg, off, size, strict);
 	default:
 		if (off % size != 0) {
 			verbose("misaligned access off %d size %d\n",
@@ -1336,6 +1355,7 @@  static void clear_all_pkt_pointers(struct bpf_verifier_env *env)
 		    reg->type != PTR_TO_PACKET_END)
 			continue;
 		reg->type = UNKNOWN_VALUE;
+		reg->off = 0;
 		reg->imm = 0;
 	}
 }
@@ -1415,6 +1435,7 @@  static int check_call(struct bpf_verifier_env *env, int func_id, int insn_idx)
 	for (i = 0; i < CALLER_SAVED_REGS; i++) {
 		reg = regs + caller_saved[i];
 		reg->type = NOT_INIT;
+		reg->off = 0;
 		reg->imm = 0;
 	}
 
@@ -1458,8 +1479,8 @@  static int check_call(struct bpf_verifier_env *env, int func_id, int insn_idx)
 	return 0;
 }
 
-static int check_packet_ptr_add(struct bpf_verifier_env *env,
-				struct bpf_insn *insn)
+static int check_pointer_add(struct bpf_verifier_env *env,
+			     struct bpf_insn *insn, bool is_packet)
 {
 	struct bpf_reg_state *regs = env->cur_state.regs;
 	struct bpf_reg_state *dst_reg = &regs[insn->dst_reg];
@@ -1468,28 +1489,28 @@  static int check_packet_ptr_add(struct bpf_verifier_env *env,
 	s32 imm;
 
 	if (BPF_SRC(insn->code) == BPF_K) {
-		/* pkt_ptr += imm */
+		/* pointer += imm */
 		imm = insn->imm;
 
 add_imm:
-		if (imm < 0) {
-			verbose("addition of negative constant to packet pointer is not allowed\n");
-			return -EACCES;
-		}
-		if (imm >= MAX_PACKET_OFF ||
-		    imm + dst_reg->off >= MAX_PACKET_OFF) {
-			verbose("constant %d is too large to add to packet pointer\n",
-				imm);
-			return -EACCES;
+		if (is_packet) {
+			if (imm < 0) {
+				verbose("addition of negative constant to packet pointer is not allowed\n");
+				return -EACCES;
+			}
+			if (imm >= MAX_PACKET_OFF ||
+			    imm + dst_reg->off >= MAX_PACKET_OFF) {
+				verbose("constant %d is too large to add to packet pointer\n",
+					imm);
+				return -EACCES;
+			}
 		}
-		/* 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;
 	} 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 */
@@ -1503,46 +1524,62 @@  static int check_packet_ptr_add(struct bpf_verifier_env *env,
 		}
 
 		if (src_reg->type == CONST_IMM) {
-			/* pkt_ptr += reg where reg is known constant */
+			/* pointer += reg where reg is known constant */
 			imm = src_reg->imm;
 			goto add_imm;
 		}
-		/* disallow pkt_ptr += reg
+		/* disallow pointer += reg
 		 * if reg is not uknown_value with guaranteed zero upper bits
-		 * otherwise pkt_ptr may overflow and addition will become
+		 * otherwise pointer_ptr may overflow and addition will become
 		 * subtraction which is not allowed
 		 */
 		if (src_reg->type != UNKNOWN_VALUE) {
-			verbose("cannot add '%s' to ptr_to_packet\n",
+			verbose("cannot add '%s' to pointer\n",
 				reg_type_str[src_reg->type]);
 			return -EACCES;
 		}
-		if (src_reg->imm < 48) {
+		if (is_packet && src_reg->imm < 48) {
 			verbose("cannot add integer value with %lld upper zero bits to ptr_to_packet\n",
 				src_reg->imm);
 			return -EACCES;
 		}
 
-		had_id = (dst_reg->id != 0);
-
-		/* dst_reg stays as pkt_ptr type and since some positive
+		/* dst_reg stays as the same type and since some positive
 		 * integer value was added to the pointer, increment its 'id'
 		 */
 		dst_reg->id = ++env->id_gen;
 
-		/* something was added to pkt_ptr, set range to zero */
 		dst_reg->aux_off += dst_reg->off;
 		dst_reg->off = 0;
-		dst_reg->range = 0;
-		if (had_id)
+
+		if (is_packet) {
+			/* something was added to packet ptr, set range to zero */
+			dst_reg->range = 0;
+		}
+		if (dst_reg->aux_off_align) {
 			dst_reg->aux_off_align = min(dst_reg->aux_off_align,
 						     src_reg->min_align);
-		else
+		} else {
 			dst_reg->aux_off_align = src_reg->min_align;
+		}
+		if (!dst_reg->aux_off_align)
+			dst_reg->aux_off_align = 1;
 	}
 	return 0;
 }
 
+static int check_packet_ptr_add(struct bpf_verifier_env *env,
+				struct bpf_insn *insn)
+{
+	return check_pointer_add(env, insn, true);
+}
+
+static int check_map_ptr_add(struct bpf_verifier_env *env,
+			     struct bpf_insn *insn)
+{
+	return check_pointer_add(env, insn, false);
+}
+
 static int evaluate_reg_alu(struct bpf_verifier_env *env, struct bpf_insn *insn)
 {
 	struct bpf_reg_state *regs = env->cur_state.regs;
@@ -2056,10 +2093,12 @@  static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
 		if (env->allow_ptr_leaks &&
 		    BPF_CLASS(insn->code) == BPF_ALU64 && opcode == BPF_ADD &&
 		    (dst_reg->type == PTR_TO_MAP_VALUE ||
-		     dst_reg->type == PTR_TO_MAP_VALUE_ADJ))
+		     dst_reg->type == PTR_TO_MAP_VALUE_ADJ)) {
 			dst_reg->type = PTR_TO_MAP_VALUE_ADJ;
-		else
+			check_map_ptr_add(env, insn);
+		} else {
 			mark_reg_unknown_value(regs, insn->dst_reg);
+		}
 	}
 
 	return 0;
@@ -2480,6 +2519,7 @@  static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
 	for (i = 0; i < CALLER_SAVED_REGS; i++) {
 		reg = regs + caller_saved[i];
 		reg->type = NOT_INIT;
+		reg->off = 0;
 		reg->imm = 0;
 	}
 
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 3773562..889fb5a 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -5070,7 +5070,8 @@  static void do_test_single(struct bpf_test *test, bool unpriv,
 
 	reject_from_alignment = fd_prog < 0 &&
 				(test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS) &&
-				strstr(bpf_vlog, "Unknown alignment.");
+				(strstr(bpf_vlog, "misaligned value access") ||
+				 strstr(bpf_vlog, "Value access is only"));
 #ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
 	if (reject_from_alignment) {
 		printf("FAIL\nFailed due to alignment despite having efficient unaligned access: '%s'!\n",