diff mbox series

[bpf-next,05/16] bpf: Support map key with dynptr in verifier

Message ID 20241008091501.8302-6-houtao@huaweicloud.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series Support dynptr key for hash map | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 6 this patch: 6
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 13 of 13 maintainers
netdev/build_clang success Errors and warnings before: 6 this patch: 6
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 15 this patch: 15
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns WARNING: line length of 95 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns WARNING: line length of 98 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18

Commit Message

Hou Tao Oct. 8, 2024, 9:14 a.m. UTC 
From: Hou Tao <houtao1@huawei.com>

The patch basically does the following three things to enable dynptr key
for bpf map:

1) Only allow PTR_TO_STACK typed register for dynptr key
The main reason is that bpf_dynptr can only be defined in the stack, so
for dynptr key only PTR_TO_STACK typed register is allowed. bpf_dynptr
could also be represented by CONST_PTR_TO_DYNPTR typed register (e.g.,
in callback func or subprog), but it is not supported now.

2) Only allow fixed-offset for PTR_TO_STACK register
Variable-offset for PTR_TO_STACK typed register is disallowed, because
it is impossible to check whether or not the stack access is aligned
with BPF_REG_SIZE and is matched with the location of dynptr or
non-dynptr part in the map key.

3) Check the layout of the stack content is matched with the btf_record
Firstly check the start offset of the stack access is aligned with
BPF_REG_SIZE, then check the offset and the size of dynptr/non-dynptr
parts in the stack content is consistent with the btf_record of the map
key.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 kernel/bpf/verifier.c | 143 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 134 insertions(+), 9 deletions(-)

Comments

Eduard Zingerman Oct. 10, 2024, 8:30 p.m. UTC | #1 
On Tue, 2024-10-08 at 17:14 +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> The patch basically does the following three things to enable dynptr key
> for bpf map:
> 
> 1) Only allow PTR_TO_STACK typed register for dynptr key
> The main reason is that bpf_dynptr can only be defined in the stack, so
> for dynptr key only PTR_TO_STACK typed register is allowed. bpf_dynptr
> could also be represented by CONST_PTR_TO_DYNPTR typed register (e.g.,
> in callback func or subprog), but it is not supported now.
> 
> 2) Only allow fixed-offset for PTR_TO_STACK register
> Variable-offset for PTR_TO_STACK typed register is disallowed, because
> it is impossible to check whether or not the stack access is aligned
> with BPF_REG_SIZE and is matched with the location of dynptr or
> non-dynptr part in the map key.
> 
> 3) Check the layout of the stack content is matched with the btf_record
> Firstly check the start offset of the stack access is aligned with
> BPF_REG_SIZE, then check the offset and the size of dynptr/non-dynptr
> parts in the stack content is consistent with the btf_record of the map
> key.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---

The logic of this patch looks correct, however I find it cumbersome.
The only place where access to dynptr key is allowed is 'case ARG_PTR_TO_MAP_KEY'
in check_func_arg(), a lot of places are modified to facilitate this.
It seems that logic would be easier to follow if there would be a
dedicated function to check dynptr key constraints, called only for
the 'case ARG_PTR_TO_MAP_KEY'. This would als make 'struct dynptr_key_state'
unnecessary as this state would be tracked inside such function.
Wdyt?

[...]
Eduard Zingerman Oct. 10, 2024, 8:57 p.m. UTC | #2 
On Thu, 2024-10-10 at 13:30 -0700, Eduard Zingerman wrote:

[...]

> The logic of this patch looks correct, however I find it cumbersome.
> The only place where access to dynptr key is allowed is 'case ARG_PTR_TO_MAP_KEY'
> in check_func_arg(), a lot of places are modified to facilitate this.
> It seems that logic would be easier to follow if there would be a
> dedicated function to check dynptr key constraints, called only for
> the 'case ARG_PTR_TO_MAP_KEY'. This would als make 'struct dynptr_key_state'
> unnecessary as this state would be tracked inside such function.
> Wdyt?

Just realized that change to check_stack_range_initialized would still
be necessary, as it forbids dynptr access at the moment. Unfortunate.
Dan Carpenter Oct. 13, 2024, 1:07 p.m. UTC | #3 
Hi Hou,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Hou-Tao/bpf-Introduce-map-flag-BPF_F_DYNPTR_IN_KEY/20241008-171136
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20241008091501.8302-6-houtao%40huaweicloud.com
patch subject: [PATCH bpf-next 05/16] bpf: Support map key with dynptr in verifier
config: x86_64-randconfig-161-20241011 (https://download.01.org/0day-ci/archive/20241012/202410120302.bUO1BoP7-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202410120302.bUO1BoP7-lkp@intel.com/

smatch warnings:
kernel/bpf/verifier.c:7471 check_stack_range_initialized() error: we previously assumed 'meta' could be null (see line 7439)

vim +/meta +7471 kernel/bpf/verifier.c

01f810ace9ed37 Andrei Matei            2021-02-06  7361  static int check_stack_range_initialized(
01f810ace9ed37 Andrei Matei            2021-02-06  7362  		struct bpf_verifier_env *env, int regno, int off,
81b030a7eaa2ee Hou Tao                 2024-10-08  7363  		int access_size, unsigned int access_flags,
61df10c7799e27 Kumar Kartikeya Dwivedi 2022-04-25  7364  		enum bpf_access_src type, struct bpf_call_arg_meta *meta)
17a5267067f3c3 Alexei Starovoitov      2014-09-26  7365  {
2a159c6f82381a Daniel Borkmann         2018-10-21  7366  	struct bpf_reg_state *reg = reg_state(env, regno);
f4d7e40a5b7157 Alexei Starovoitov      2017-12-14  7367  	struct bpf_func_state *state = func(env, reg);
f7cf25b2026dc8 Alexei Starovoitov      2019-06-15  7368  	int err, min_off, max_off, i, j, slot, spi;
01f810ace9ed37 Andrei Matei            2021-02-06  7369  	char *err_extra = type == ACCESS_HELPER ? " indirect" : "";
01f810ace9ed37 Andrei Matei            2021-02-06  7370  	enum bpf_access_type bounds_check_type;
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7371  	struct dynptr_key_state dynptr_key;
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7372  	bool dynptr_read_allowed;
01f810ace9ed37 Andrei Matei            2021-02-06  7373  	/* Some accesses can write anything into the stack, others are
01f810ace9ed37 Andrei Matei            2021-02-06  7374  	 * read-only.
01f810ace9ed37 Andrei Matei            2021-02-06  7375  	 */
01f810ace9ed37 Andrei Matei            2021-02-06  7376  	bool clobber = false;
17a5267067f3c3 Alexei Starovoitov      2014-09-26  7377  
81b030a7eaa2ee Hou Tao                 2024-10-08  7378  	if (access_size == 0 && !(access_flags & ACCESS_F_ZERO_SIZE_ALLOWED)) {
01f810ace9ed37 Andrei Matei            2021-02-06  7379  		verbose(env, "invalid zero-sized read\n");
01f810ace9ed37 Andrei Matei            2021-02-06  7380  		return -EACCES;
01f810ace9ed37 Andrei Matei            2021-02-06  7381  	}
01f810ace9ed37 Andrei Matei            2021-02-06  7382  
01f810ace9ed37 Andrei Matei            2021-02-06  7383  	if (type == ACCESS_HELPER) {
01f810ace9ed37 Andrei Matei            2021-02-06  7384  		/* The bounds checks for writes are more permissive than for
01f810ace9ed37 Andrei Matei            2021-02-06  7385  		 * reads. However, if raw_mode is not set, we'll do extra
01f810ace9ed37 Andrei Matei            2021-02-06  7386  		 * checks below.
01f810ace9ed37 Andrei Matei            2021-02-06  7387  		 */
01f810ace9ed37 Andrei Matei            2021-02-06  7388  		bounds_check_type = BPF_WRITE;
01f810ace9ed37 Andrei Matei            2021-02-06  7389  		clobber = true;
01f810ace9ed37 Andrei Matei            2021-02-06  7390  	} else {
01f810ace9ed37 Andrei Matei            2021-02-06  7391  		bounds_check_type = BPF_READ;
01f810ace9ed37 Andrei Matei            2021-02-06  7392  	}
01f810ace9ed37 Andrei Matei            2021-02-06  7393  	err = check_stack_access_within_bounds(env, regno, off, access_size,
01f810ace9ed37 Andrei Matei            2021-02-06  7394  					       type, bounds_check_type);
2011fccfb61bbd Andrey Ignatov          2019-03-28  7395  	if (err)
2011fccfb61bbd Andrey Ignatov          2019-03-28  7396  		return err;
01f810ace9ed37 Andrei Matei            2021-02-06  7397  
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7398  	dynptr_read_allowed = access_flags & ACCESS_F_DYNPTR_READ_ALLOWED;
01f810ace9ed37 Andrei Matei            2021-02-06  7399  	if (tnum_is_const(reg->var_off)) {
01f810ace9ed37 Andrei Matei            2021-02-06  7400  		min_off = max_off = reg->var_off.value + off;
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7401  
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7402  		if (dynptr_read_allowed && (min_off % BPF_REG_SIZE)) {
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7403  			verbose(env, "R%d misaligned offset %d for dynptr-key\n", regno, min_off);
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7404  			return -EACCES;
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7405  		}

can meta be NULL on this path?  If not then this is a false positive.

2011fccfb61bbd Andrey Ignatov          2019-03-28  7406  	} else {
088ec26d9c2da9 Andrey Ignatov          2019-04-03  7407  		/* Variable offset is prohibited for unprivileged mode for
088ec26d9c2da9 Andrey Ignatov          2019-04-03  7408  		 * simplicity since it requires corresponding support in
088ec26d9c2da9 Andrey Ignatov          2019-04-03  7409  		 * Spectre masking for stack ALU.
088ec26d9c2da9 Andrey Ignatov          2019-04-03  7410  		 * See also retrieve_ptr_limit().
088ec26d9c2da9 Andrey Ignatov          2019-04-03  7411  		 */
2c78ee898d8f10 Alexei Starovoitov      2020-05-13  7412  		if (!env->bypass_spec_v1) {
f1174f77b50c94 Edward Cree             2017-08-07  7413  			char tn_buf[48];
f1174f77b50c94 Edward Cree             2017-08-07  7414  
914cb781ee1a35 Alexei Starovoitov      2017-11-30  7415  			tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
01f810ace9ed37 Andrei Matei            2021-02-06  7416  			verbose(env, "R%d%s variable offset stack access prohibited for !root, var_off=%s\n",
01f810ace9ed37 Andrei Matei            2021-02-06  7417  				regno, err_extra, tn_buf);
ea25f914dc164c Jann Horn               2017-12-18  7418  			return -EACCES;
f1174f77b50c94 Edward Cree             2017-08-07  7419  		}
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7420  
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7421  		if (dynptr_read_allowed) {
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7422  			verbose(env, "R%d variable offset prohibited for dynptr-key\n", regno);
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7423  			return -EACCES;
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7424  		}
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7425  
f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7426  		/* Only initialized buffer on stack is allowed to be accessed
f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7427  		 * with variable offset. With uninitialized buffer it's hard to
f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7428  		 * guarantee that whole memory is marked as initialized on
f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7429  		 * helper return since specific bounds are unknown what may
f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7430  		 * cause uninitialized stack leaking.
f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7431  		 */
f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7432  		if (meta && meta->raw_mode)
f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7433  			meta = NULL;
f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7434  
01f810ace9ed37 Andrei Matei            2021-02-06  7435  		min_off = reg->smin_value + off;
01f810ace9ed37 Andrei Matei            2021-02-06  7436  		max_off = reg->smax_value + off;
107c26a70ca81b Andrey Ignatov          2019-04-03  7437  	}
17a5267067f3c3 Alexei Starovoitov      2014-09-26  7438  
435faee1aae9c1 Daniel Borkmann         2016-04-13 @7439  	if (meta && meta->raw_mode) {

Check for NULL

ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7440  		/* Ensure we won't be overwriting dynptrs when simulating byte
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7441  		 * by byte access in check_helper_call using meta.access_size.
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7442  		 * This would be a problem if we have a helper in the future
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7443  		 * which takes:
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7444  		 *
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7445  		 *	helper(uninit_mem, len, dynptr)
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7446  		 *
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7447  		 * Now, uninint_mem may overlap with dynptr pointer. Hence, it
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7448  		 * may end up writing to dynptr itself when touching memory from
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7449  		 * arg 1. This can be relaxed on a case by case basis for known
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7450  		 * safe cases, but reject due to the possibilitiy of aliasing by
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7451  		 * default.
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7452  		 */
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7453  		for (i = min_off; i < max_off + access_size; i++) {
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7454  			int stack_off = -i - 1;
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7455  
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7456  			spi = __get_spi(i);
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7457  			/* raw_mode may write past allocated_stack */
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7458  			if (state->allocated_stack <= stack_off)
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7459  				continue;
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7460  			if (state->stack[spi].slot_type[stack_off % BPF_REG_SIZE] == STACK_DYNPTR) {
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7461  				verbose(env, "potential write to dynptr at off=%d disallowed\n", i);
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7462  				return -EACCES;
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7463  			}
ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7464  		}
435faee1aae9c1 Daniel Borkmann         2016-04-13  7465  		meta->access_size = access_size;
435faee1aae9c1 Daniel Borkmann         2016-04-13  7466  		meta->regno = regno;
435faee1aae9c1 Daniel Borkmann         2016-04-13  7467  		return 0;
435faee1aae9c1 Daniel Borkmann         2016-04-13  7468  	}
435faee1aae9c1 Daniel Borkmann         2016-04-13  7469  
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7470  	if (dynptr_read_allowed) {
cf5a0c90a8bc5f Hou Tao                 2024-10-08 @7471  		err = init_dynptr_key_state(env, meta->map_ptr->key_record, &dynptr_key);
                                                                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^
Unchecked dereference

cf5a0c90a8bc5f Hou Tao                 2024-10-08  7472  		if (err)
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7473  			return err;
cf5a0c90a8bc5f Hou Tao                 2024-10-08  7474  	}
2011fccfb61bbd Andrey Ignatov          2019-03-28  7475  	for (i = min_off; i < max_off + access_size; i++) {
cc2b14d51053eb Alexei Starovoitov      2017-12-14  7476  		u8 *stype;
cc2b14d51053eb Alexei Starovoitov      2017-12-14  7477  
2011fccfb61bbd Andrey Ignatov          2019-03-28  7478  		slot = -i - 1;
638f5b90d46016 Alexei Starovoitov      2017-10-31  7479  		spi = slot / BPF_REG_SIZE;
6b4a64bafd107e Andrei Matei            2023-12-07  7480  		if (state->allocated_stack <= slot) {
Hou Tao Oct. 21, 2024, 1:50 p.m. UTC | #4 
On 10/11/2024 4:57 AM, Eduard Zingerman wrote:
> On Thu, 2024-10-10 at 13:30 -0700, Eduard Zingerman wrote:
>
> [...]
>
>> The logic of this patch looks correct, however I find it cumbersome.
>> The only place where access to dynptr key is allowed is 'case ARG_PTR_TO_MAP_KEY'
>> in check_func_arg(), a lot of places are modified to facilitate this.
>> It seems that logic would be easier to follow if there would be a
>> dedicated function to check dynptr key constraints, called only for
>> the 'case ARG_PTR_TO_MAP_KEY'. This would als make 'struct dynptr_key_state'
>> unnecessary as this state would be tracked inside such function.
>> Wdyt?
> Just realized that change to check_stack_range_initialized would still
> be necessary, as it forbids dynptr access at the moment. Unfortunate.
Thanks for the suggestion. Will check later whether a cleaner way is
available or not.
Hou Tao Oct. 31, 2024, 2:39 a.m. UTC | #5 
Hi,

On 10/13/2024 9:07 PM, Dan Carpenter wrote:
> Hi Hou,
>
> kernel test robot noticed the following build warnings:
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Hou-Tao/bpf-Introduce-map-flag-BPF_F_DYNPTR_IN_KEY/20241008-171136
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> patch link:    https://lore.kernel.org/r/20241008091501.8302-6-houtao%40huaweicloud.com
> patch subject: [PATCH bpf-next 05/16] bpf: Support map key with dynptr in verifier
> config: x86_64-randconfig-161-20241011 (https://download.01.org/0day-ci/archive/20241012/202410120302.bUO1BoP7-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202410120302.bUO1BoP7-lkp@intel.com/
>
> smatch warnings:
> kernel/bpf/verifier.c:7471 check_stack_range_initialized() error: we previously assumed 'meta' could be null (see line 7439)
>
> vim +/meta +7471 kernel/bpf/verifier.c

Thanks for the report. Sorry for the late reply. The mail is lost in my
email client. It is a false positive. Because when
ACCESS_F_DYNPTR_READ_ALLOWED is enabled, meta must be no NULL. But I
think it incurs no harm to make dynptr_read_allowed depends on a no-NULL
meta pointer.
>
> 01f810ace9ed37 Andrei Matei            2021-02-06  7361  static int check_stack_range_initialized(
> 01f810ace9ed37 Andrei Matei            2021-02-06  7362  		struct bpf_verifier_env *env, int regno, int off,
> 81b030a7eaa2ee Hou Tao                 2024-10-08  7363  		int access_size, unsigned int access_flags,
> 61df10c7799e27 Kumar Kartikeya Dwivedi 2022-04-25  7364  		enum bpf_access_src type, struct bpf_call_arg_meta *meta)
> 17a5267067f3c3 Alexei Starovoitov      2014-09-26  7365  {
> 2a159c6f82381a Daniel Borkmann         2018-10-21  7366  	struct bpf_reg_state *reg = reg_state(env, regno);
> f4d7e40a5b7157 Alexei Starovoitov      2017-12-14  7367  	struct bpf_func_state *state = func(env, reg);
> f7cf25b2026dc8 Alexei Starovoitov      2019-06-15  7368  	int err, min_off, max_off, i, j, slot, spi;
> 01f810ace9ed37 Andrei Matei            2021-02-06  7369  	char *err_extra = type == ACCESS_HELPER ? " indirect" : "";
> 01f810ace9ed37 Andrei Matei            2021-02-06  7370  	enum bpf_access_type bounds_check_type;
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7371  	struct dynptr_key_state dynptr_key;
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7372  	bool dynptr_read_allowed;
> 01f810ace9ed37 Andrei Matei            2021-02-06  7373  	/* Some accesses can write anything into the stack, others are
> 01f810ace9ed37 Andrei Matei            2021-02-06  7374  	 * read-only.
> 01f810ace9ed37 Andrei Matei            2021-02-06  7375  	 */
> 01f810ace9ed37 Andrei Matei            2021-02-06  7376  	bool clobber = false;
> 17a5267067f3c3 Alexei Starovoitov      2014-09-26  7377  
> 81b030a7eaa2ee Hou Tao                 2024-10-08  7378  	if (access_size == 0 && !(access_flags & ACCESS_F_ZERO_SIZE_ALLOWED)) {
> 01f810ace9ed37 Andrei Matei            2021-02-06  7379  		verbose(env, "invalid zero-sized read\n");
> 01f810ace9ed37 Andrei Matei            2021-02-06  7380  		return -EACCES;
> 01f810ace9ed37 Andrei Matei            2021-02-06  7381  	}
> 01f810ace9ed37 Andrei Matei            2021-02-06  7382  
> 01f810ace9ed37 Andrei Matei            2021-02-06  7383  	if (type == ACCESS_HELPER) {
> 01f810ace9ed37 Andrei Matei            2021-02-06  7384  		/* The bounds checks for writes are more permissive than for
> 01f810ace9ed37 Andrei Matei            2021-02-06  7385  		 * reads. However, if raw_mode is not set, we'll do extra
> 01f810ace9ed37 Andrei Matei            2021-02-06  7386  		 * checks below.
> 01f810ace9ed37 Andrei Matei            2021-02-06  7387  		 */
> 01f810ace9ed37 Andrei Matei            2021-02-06  7388  		bounds_check_type = BPF_WRITE;
> 01f810ace9ed37 Andrei Matei            2021-02-06  7389  		clobber = true;
> 01f810ace9ed37 Andrei Matei            2021-02-06  7390  	} else {
> 01f810ace9ed37 Andrei Matei            2021-02-06  7391  		bounds_check_type = BPF_READ;
> 01f810ace9ed37 Andrei Matei            2021-02-06  7392  	}
> 01f810ace9ed37 Andrei Matei            2021-02-06  7393  	err = check_stack_access_within_bounds(env, regno, off, access_size,
> 01f810ace9ed37 Andrei Matei            2021-02-06  7394  					       type, bounds_check_type);
> 2011fccfb61bbd Andrey Ignatov          2019-03-28  7395  	if (err)
> 2011fccfb61bbd Andrey Ignatov          2019-03-28  7396  		return err;
> 01f810ace9ed37 Andrei Matei            2021-02-06  7397  
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7398  	dynptr_read_allowed = access_flags & ACCESS_F_DYNPTR_READ_ALLOWED;
> 01f810ace9ed37 Andrei Matei            2021-02-06  7399  	if (tnum_is_const(reg->var_off)) {
> 01f810ace9ed37 Andrei Matei            2021-02-06  7400  		min_off = max_off = reg->var_off.value + off;
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7401  
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7402  		if (dynptr_read_allowed && (min_off % BPF_REG_SIZE)) {
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7403  			verbose(env, "R%d misaligned offset %d for dynptr-key\n", regno, min_off);
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7404  			return -EACCES;
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7405  		}
>
> can meta be NULL on this path?  If not then this is a false positive.
>
> 2011fccfb61bbd Andrey Ignatov          2019-03-28  7406  	} else {
> 088ec26d9c2da9 Andrey Ignatov          2019-04-03  7407  		/* Variable offset is prohibited for unprivileged mode for
> 088ec26d9c2da9 Andrey Ignatov          2019-04-03  7408  		 * simplicity since it requires corresponding support in
> 088ec26d9c2da9 Andrey Ignatov          2019-04-03  7409  		 * Spectre masking for stack ALU.
> 088ec26d9c2da9 Andrey Ignatov          2019-04-03  7410  		 * See also retrieve_ptr_limit().
> 088ec26d9c2da9 Andrey Ignatov          2019-04-03  7411  		 */
> 2c78ee898d8f10 Alexei Starovoitov      2020-05-13  7412  		if (!env->bypass_spec_v1) {
> f1174f77b50c94 Edward Cree             2017-08-07  7413  			char tn_buf[48];
> f1174f77b50c94 Edward Cree             2017-08-07  7414  
> 914cb781ee1a35 Alexei Starovoitov      2017-11-30  7415  			tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
> 01f810ace9ed37 Andrei Matei            2021-02-06  7416  			verbose(env, "R%d%s variable offset stack access prohibited for !root, var_off=%s\n",
> 01f810ace9ed37 Andrei Matei            2021-02-06  7417  				regno, err_extra, tn_buf);
> ea25f914dc164c Jann Horn               2017-12-18  7418  			return -EACCES;
> f1174f77b50c94 Edward Cree             2017-08-07  7419  		}
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7420  
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7421  		if (dynptr_read_allowed) {
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7422  			verbose(env, "R%d variable offset prohibited for dynptr-key\n", regno);
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7423  			return -EACCES;
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7424  		}
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7425  
> f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7426  		/* Only initialized buffer on stack is allowed to be accessed
> f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7427  		 * with variable offset. With uninitialized buffer it's hard to
> f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7428  		 * guarantee that whole memory is marked as initialized on
> f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7429  		 * helper return since specific bounds are unknown what may
> f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7430  		 * cause uninitialized stack leaking.
> f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7431  		 */
> f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7432  		if (meta && meta->raw_mode)
> f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7433  			meta = NULL;
> f2bcd05ec7b839 Andrey Ignatov          2019-04-03  7434  
> 01f810ace9ed37 Andrei Matei            2021-02-06  7435  		min_off = reg->smin_value + off;
> 01f810ace9ed37 Andrei Matei            2021-02-06  7436  		max_off = reg->smax_value + off;
> 107c26a70ca81b Andrey Ignatov          2019-04-03  7437  	}
> 17a5267067f3c3 Alexei Starovoitov      2014-09-26  7438  
> 435faee1aae9c1 Daniel Borkmann         2016-04-13 @7439  	if (meta && meta->raw_mode) {
>
> Check for NULL
>
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7440  		/* Ensure we won't be overwriting dynptrs when simulating byte
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7441  		 * by byte access in check_helper_call using meta.access_size.
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7442  		 * This would be a problem if we have a helper in the future
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7443  		 * which takes:
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7444  		 *
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7445  		 *	helper(uninit_mem, len, dynptr)
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7446  		 *
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7447  		 * Now, uninint_mem may overlap with dynptr pointer. Hence, it
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7448  		 * may end up writing to dynptr itself when touching memory from
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7449  		 * arg 1. This can be relaxed on a case by case basis for known
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7450  		 * safe cases, but reject due to the possibilitiy of aliasing by
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7451  		 * default.
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7452  		 */
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7453  		for (i = min_off; i < max_off + access_size; i++) {
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7454  			int stack_off = -i - 1;
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7455  
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7456  			spi = __get_spi(i);
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7457  			/* raw_mode may write past allocated_stack */
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7458  			if (state->allocated_stack <= stack_off)
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7459  				continue;
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7460  			if (state->stack[spi].slot_type[stack_off % BPF_REG_SIZE] == STACK_DYNPTR) {
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7461  				verbose(env, "potential write to dynptr at off=%d disallowed\n", i);
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7462  				return -EACCES;
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7463  			}
> ef8fc7a07c0e16 Kumar Kartikeya Dwivedi 2023-01-21  7464  		}
> 435faee1aae9c1 Daniel Borkmann         2016-04-13  7465  		meta->access_size = access_size;
> 435faee1aae9c1 Daniel Borkmann         2016-04-13  7466  		meta->regno = regno;
> 435faee1aae9c1 Daniel Borkmann         2016-04-13  7467  		return 0;
> 435faee1aae9c1 Daniel Borkmann         2016-04-13  7468  	}
> 435faee1aae9c1 Daniel Borkmann         2016-04-13  7469  
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7470  	if (dynptr_read_allowed) {
> cf5a0c90a8bc5f Hou Tao                 2024-10-08 @7471  		err = init_dynptr_key_state(env, meta->map_ptr->key_record, &dynptr_key);
>                                                                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^
> Unchecked dereference
>
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7472  		if (err)
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7473  			return err;
> cf5a0c90a8bc5f Hou Tao                 2024-10-08  7474  	}
> 2011fccfb61bbd Andrey Ignatov          2019-03-28  7475  	for (i = min_off; i < max_off + access_size; i++) {
> cc2b14d51053eb Alexei Starovoitov      2017-12-14  7476  		u8 *stype;
> cc2b14d51053eb Alexei Starovoitov      2017-12-14  7477  
> 2011fccfb61bbd Andrey Ignatov          2019-03-28  7478  		slot = -i - 1;
> 638f5b90d46016 Alexei Starovoitov      2017-10-31  7479  		spi = slot / BPF_REG_SIZE;
> 6b4a64bafd107e Andrei Matei            2023-12-07  7480  		if (state->allocated_stack <= slot) {
>
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2090d2472d7c..345b45edf2a7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5042,6 +5042,7 @@  enum bpf_access_src {
 };
 
 #define ACCESS_F_ZERO_SIZE_ALLOWED BIT(0)
+#define ACCESS_F_DYNPTR_READ_ALLOWED BIT(1)
 
 static int check_stack_range_initialized(struct bpf_verifier_env *env,
 					 int regno, int off, int access_size,
@@ -7267,6 +7268,86 @@  static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
 	return 0;
 }
 
+struct dynptr_key_state {
+	const struct btf_record *rec;
+	const struct btf_field *cur_dynptr;
+	bool valid_dynptr_id;
+	int cur_dynptr_id;
+};
+
+static int init_dynptr_key_state(struct bpf_verifier_env *env, const struct btf_record *rec,
+				 struct dynptr_key_state *state)
+{
+	unsigned int i;
+
+	/* Find the first dynptr in the dynptr-key */
+	for (i = 0; i < rec->cnt; i++) {
+		if (rec->fields[i].type == BPF_DYNPTR)
+			break;
+	}
+	if (i >= rec->cnt) {
+		verbose(env, "verifier bug: dynptr not found\n");
+		return -EFAULT;
+	}
+
+	state->rec = rec;
+	state->cur_dynptr = &rec->fields[i];
+	state->valid_dynptr_id = false;
+
+	return 0;
+}
+
+static int check_dynptr_key_access(struct bpf_verifier_env *env, struct dynptr_key_state *state,
+				   struct bpf_reg_state *reg, u8 stype, int offset)
+{
+	const struct btf_field *dynptr = state->cur_dynptr;
+
+	/* Non-dynptr part before a dynptr or non-dynptr part after
+	 * the last dynptr.
+	 */
+	if (offset < dynptr->offset || offset >= dynptr->offset + dynptr->size) {
+		if (stype == STACK_DYNPTR) {
+			verbose(env,
+				"dynptr-key expects non-dynptr at offset %d cur_dynptr_offset %u\n",
+				offset, dynptr->offset);
+			return -EACCES;
+		}
+	} else {
+		if (stype != STACK_DYNPTR) {
+			verbose(env,
+				"dynptr-key expects dynptr at offset %d cur_dynptr_offset %u\n",
+				offset, dynptr->offset);
+			return -EACCES;
+		}
+
+		/* A dynptr is composed of parts from two dynptrs */
+		if (state->valid_dynptr_id && reg->id != state->cur_dynptr_id) {
+			verbose(env, "malformed dynptr-key at offset %d cur_dynptr_offset %u\n",
+				offset, dynptr->offset);
+			return -EACCES;
+		}
+		if (!state->valid_dynptr_id) {
+			state->valid_dynptr_id = true;
+			state->cur_dynptr_id = reg->id;
+		}
+
+		if (offset == dynptr->offset + dynptr->size - 1) {
+			const struct btf_record *rec = state->rec;
+			unsigned int i;
+
+			for (i = dynptr - rec->fields + 1; i < rec->cnt; i++) {
+				if (rec->fields[i].type == BPF_DYNPTR) {
+					state->cur_dynptr = &rec->fields[i];
+					state->valid_dynptr_id = false;
+					break;
+				}
+			}
+		}
+	}
+
+	return 0;
+}
+
 /* When register 'regno' is used to read the stack (either directly or through
  * a helper function) make sure that it's within stack boundary and, depending
  * on the access type and privileges, that all elements of the stack are
@@ -7287,6 +7368,8 @@  static int check_stack_range_initialized(
 	int err, min_off, max_off, i, j, slot, spi;
 	char *err_extra = type == ACCESS_HELPER ? " indirect" : "";
 	enum bpf_access_type bounds_check_type;
+	struct dynptr_key_state dynptr_key;
+	bool dynptr_read_allowed;
 	/* Some accesses can write anything into the stack, others are
 	 * read-only.
 	 */
@@ -7312,9 +7395,14 @@  static int check_stack_range_initialized(
 	if (err)
 		return err;
 
-
+	dynptr_read_allowed = access_flags & ACCESS_F_DYNPTR_READ_ALLOWED;
 	if (tnum_is_const(reg->var_off)) {
 		min_off = max_off = reg->var_off.value + off;
+
+		if (dynptr_read_allowed && (min_off % BPF_REG_SIZE)) {
+			verbose(env, "R%d misaligned offset %d for dynptr-key\n", regno, min_off);
+			return -EACCES;
+		}
 	} else {
 		/* Variable offset is prohibited for unprivileged mode for
 		 * simplicity since it requires corresponding support in
@@ -7329,6 +7417,12 @@  static int check_stack_range_initialized(
 				regno, err_extra, tn_buf);
 			return -EACCES;
 		}
+
+		if (dynptr_read_allowed) {
+			verbose(env, "R%d variable offset prohibited for dynptr-key\n", regno);
+			return -EACCES;
+		}
+
 		/* 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
@@ -7373,19 +7467,26 @@  static int check_stack_range_initialized(
 		return 0;
 	}
 
+	if (dynptr_read_allowed) {
+		err = init_dynptr_key_state(env, meta->map_ptr->key_record, &dynptr_key);
+		if (err)
+			return err;
+	}
 	for (i = min_off; i < max_off + access_size; i++) {
 		u8 *stype;
 
 		slot = -i - 1;
 		spi = slot / BPF_REG_SIZE;
 		if (state->allocated_stack <= slot) {
-			verbose(env, "verifier bug: allocated_stack too small");
+			verbose(env, "verifier bug: allocated_stack too small\n");
 			return -EFAULT;
 		}
 
 		stype = &state->stack[spi].slot_type[slot % BPF_REG_SIZE];
 		if (*stype == STACK_MISC)
 			goto mark;
+		if (dynptr_read_allowed && *stype == STACK_DYNPTR)
+			goto mark;
 		if ((*stype == STACK_ZERO) ||
 		    (*stype == STACK_INVALID && env->allow_uninit_stack)) {
 			if (clobber) {
@@ -7418,18 +7519,28 @@  static int check_stack_range_initialized(
 		}
 		return -EACCES;
 mark:
+		if (dynptr_read_allowed) {
+			err = check_dynptr_key_access(env, &dynptr_key,
+						      &state->stack[spi].spilled_ptr, *stype,
+						      i - min_off);
+			if (err)
+				return err;
+		}
+
 		/* reading any byte out of 8-byte 'spill_slot' will cause
 		 * the whole slot to be marked as 'read'
-		 */
-		mark_reg_read(env, &state->stack[spi].spilled_ptr,
-			      state->stack[spi].spilled_ptr.parent,
-			      REG_LIVE_READ64);
-		/* We do not set REG_LIVE_WRITTEN for stack slot, as we can not
+		 *
+		 * We do not set REG_LIVE_WRITTEN for stack slot, as we can not
 		 * be sure that whether stack slot is written to or not. Hence,
 		 * we must still conservatively propagate reads upwards even if
 		 * helper may write to the entire memory range.
 		 */
+		mark_reg_read(env, &state->stack[spi].spilled_ptr,
+			      state->stack[spi].spilled_ptr.parent,
+			      REG_LIVE_READ64);
+
 	}
+
 	return 0;
 }
 
@@ -8933,6 +9044,9 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		meta->map_uid = reg->map_uid;
 		break;
 	case ARG_PTR_TO_MAP_KEY:
+	{
+		u32 access_flags = 0;
+
 		/* bpf_map_xxx(..., map_ptr, ..., key) call:
 		 * check that [key, key + map->key_size) are within
 		 * stack limits and initialized
@@ -8946,10 +9060,21 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 			verbose(env, "invalid map_ptr to access map->key\n");
 			return -EACCES;
 		}
+		/* Only allow PTR_TO_STACK for dynptr-key */
+		if (bpf_map_has_dynptr_key(meta->map_ptr)) {
+			if (base_type(reg->type) != PTR_TO_STACK) {
+				verbose(env, "map dynptr-key requires stack ptr but got %s\n",
+					reg_type_str(env, reg->type));
+				return -EACCES;
+			}
+			access_flags |= ACCESS_F_DYNPTR_READ_ALLOWED;
+		}
+		meta->raw_mode = false;
 		err = check_helper_mem_access(env, regno,
-					      meta->map_ptr->key_size, 0,
-					      NULL);
+					      meta->map_ptr->key_size, access_flags,
+					      meta);
 		break;
+	}
 	case ARG_PTR_TO_MAP_VALUE:
 		if (type_may_be_null(arg_type) && register_is_null(reg))
 			return 0;