On Tue, Mar 21, 2023 at 1:47 AM Dan Carpenter <error27@xxxxxxxxx> wrote:
>
> Hello Andrii Nakryiko,
>
> The patch 06accc8779c1: "bpf: add support for open-coded iterator
> loops" from Mar 8, 2023, leads to the following Smatch static checker
> warning:
>
> kernel/bpf/verifier.c:6781 process_iter_arg()
> warn: assigning signed to unsigned: 'meta->iter.spi = spi' '(-34),0-268435454'
>
> kernel/bpf/verifier.c
> 6762 static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_idx,
> 6763 struct bpf_kfunc_call_arg_meta *meta)
> 6764 {
> 6765 struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> 6766 const struct btf_type *t;
> 6767 const struct btf_param *arg;
> 6768 int spi, err, i, nr_slots;
> 6769 u32 btf_id;
> 6770
> 6771 /* btf_check_iter_kfuncs() ensures we don't need to validate anything here */
> 6772 arg = &btf_params(meta->func_proto)[0];
> 6773 t = btf_type_skip_modifiers(meta->btf, arg->type, NULL); /* PTR */
> 6774 t = btf_type_skip_modifiers(meta->btf, t->type, &btf_id); /* STRUCT */
> 6775 nr_slots = t->size / BPF_REG_SIZE;
> 6776
> 6777 spi = iter_get_spi(env, reg, nr_slots);
> 6778 if (spi < 0 && spi != -ERANGE)
> ^^^^^^^^^^^^^^
> Assume iter_get_spi() returns -ERANGE
>
> 6779 return spi;
> 6780
> --> 6781 meta->iter.spi = spi;
>
> meta->iter.spi is a u8. How is this going to work? At the very least
> it needs a comment.
The reason why all this works is because this meta->iter.spi field is
used only for iter_next() functions, at which point it is validated by
is_iter_reg_valid_init() to not be an -ERANGE.
But I think I'll just move this part to after the below if, and
-ERANGE is not going to be an allowable case there.
Thanks for pointing this out!
>
> 6782 meta->iter.frameno = reg->frameno;
> 6783
> 6784 if (is_iter_new_kfunc(meta)) {
> 6785 /* bpf_iter_<type>_new() expects pointer to uninit iter state */
> 6786 if (!is_iter_reg_valid_uninit(env, reg, nr_slots)) {
> 6787 verbose(env, "expected uninitialized iter_%s as arg #%d\n",
> 6788 iter_type_str(meta->btf, btf_id), regno);
> 6789 return -EINVAL;
> 6790 }
> 6791
> 6792 for (i = 0; i < nr_slots * 8; i += BPF_REG_SIZE) {
> 6793 err = check_mem_access(env, insn_idx, regno,
> 6794 i, BPF_DW, BPF_WRITE, -1, false);
> 6795 if (err)
> 6796 return err;
> 6797 }
> 6798
> 6799 err = mark_stack_slots_iter(env, reg, insn_idx, meta->btf, btf_id, nr_slots);
> 6800 if (err)
> 6801 return err;
> 6802 } else {
> 6803 /* iter_next() or iter_destroy() expect initialized iter state*/
> 6804 if (!is_iter_reg_valid_init(env, reg, meta->btf, btf_id, nr_slots)) {
> 6805 verbose(env, "expected an initialized iter_%s as arg #%d\n",
> 6806 iter_type_str(meta->btf, btf_id), regno);
> 6807 return -EINVAL;
> 6808 }
> 6809
> 6810 err = mark_iter_read(env, reg, spi, nr_slots);
>
> If spi is -ERANGE here then it leads to an array underflow.
-ERANGE can't happen because is_iter_reg_valid_init() will reject it first.
>
> 6811 if (err)
> 6812 return err;
> 6813
> 6814 meta->ref_obj_id = iter_ref_obj_id(env, reg, spi);
> 6815
> 6816 if (is_iter_destroy_kfunc(meta)) {
> 6817 err = unmark_stack_slots_iter(env, reg, nr_slots);
> 6818 if (err)
> 6819 return err;
> 6820 }
> 6821 }
> 6822
> 6823 return 0;
> 6824 }
>
> regards,
> dan carpenter
