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@xxxxxxxxx/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@xxxxxxxxx> > | Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > | Closes: https://lore.kernel.org/r/202410120302.bUO1BoP7-lkp@xxxxxxxxx/ > > 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) { >
