> 2023年7月17日 22:48，Dan Carpenter <dan.carpenter@xxxxxxxxxx> 写道：
>
> Hello Menglong Dong,
>
> The patch 473e3150e30a: "bpf, x86: allow function arguments up to 12
> for TRACING" from Jul 13, 2023 (linux-next), leads to the following
> Smatch static checker warning:
>
> arch/x86/net/bpf_jit_comp.c:1999 save_args()
> error: uninitialized symbol 'first_off'.
>
> arch/x86/net/bpf_jit_comp.c
> 1925 static void save_args(const struct btf_func_model *m, u8 **prog,
> 1926 int stack_size, bool for_call_origin)
> 1927 {
> 1928 int arg_regs, first_off, nr_regs = 0, nr_stack_slots = 0;
> 1929 int i, j;
> 1930
> 1931 /* Store function arguments to stack.
> 1932 * For a function that accepts two pointers the sequence will be:
> 1933 * mov QWORD PTR [rbp-0x10],rdi
> 1934 * mov QWORD PTR [rbp-0x8],rsi
> 1935 */
> 1936 for (i = 0; i < min_t(int, m->nr_args, MAX_BPF_FUNC_ARGS); i++) {
> 1937 arg_regs = (m->arg_size[i] + 7) / 8;
> 1938
> 1939 /* According to the research of Yonghong, struct members
> 1940 * should be all in register or all on the stack.
> 1941 * Meanwhile, the compiler will pass the argument on regs
> 1942 * if the remaining regs can hold the argument.
> 1943 *
> 1944 * Disorder of the args can happen. For example:
> 1945 *
> 1946 * struct foo_struct {
> 1947 * long a;
> 1948 * int b;
> 1949 * };
> 1950 * int foo(char, char, char, char, char, struct foo_struct,
> 1951 * char);
> 1952 *
> 1953 * the arg1-5,arg7 will be passed by regs, and arg6 will
> 1954 * by stack.
> 1955 */
> 1956 if (nr_regs + arg_regs > 6) {
>
> first_off is not set on else path. It's also in a loop so maybe there
> is some guarantee that we will hit an else path...
>
> 1957 /* copy function arguments from origin stack frame
> 1958 * into current stack frame.
> 1959 *
> 1960 * The starting address of the arguments on-stack
> 1961 * is:
> 1962 * rbp + 8(push rbp) +
> 1963 * 8(return addr of origin call) +
> 1964 * 8(return addr of the caller)
> 1965 * which means: rbp + 24
> 1966 */
> 1967 for (j = 0; j < arg_regs; j++) {
> 1968 emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
> 1969 nr_stack_slots * 8 + 0x18);
> 1970 emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
> 1971 -stack_size);
> 1972
> 1973 if (!nr_stack_slots)
> 1974 first_off = stack_size;
> 1975 stack_size -= 8;
> 1976 nr_stack_slots++;
> 1977 }
> 1978 } else {
> 1979 /* Only copy the arguments on-stack to current
> 1980 * 'stack_size' and ignore the regs, used to
> 1981 * prepare the arguments on-stack for orign call.
> 1982 */
> 1983 if (for_call_origin) {
> 1984 nr_regs += arg_regs;
> 1985 continue;
> 1986 }
> 1987
> 1988 /* copy the arguments from regs into stack */
> 1989 for (j = 0; j < arg_regs; j++) {
> 1990 emit_stx(prog, BPF_DW, BPF_REG_FP,
> 1991 nr_regs == 5 ? X86_REG_R9 : BPF_REG_1 + nr_regs,
> 1992 -stack_size);
> 1993 stack_size -= 8;
> 1994 nr_regs++;
> 1995 }
> 1996 }
> 1997 }
> 1998
> --> 1999 clean_stack_garbage(m, prog, nr_stack_slots, first_off);
> 2000 }
Hello,
Thanks for the reporting. The variable ‘first_off’ that passed to
clean_stack_garbage() should be ok, as it is only used when
"nr_stack_slots == 1”, in which case “first_off” should already be
initialized.
(Anyway, maybe we should initialize it to avoid passing a
uninitialized variable to a function?)
Thanks!
Menglong Dong
> regards,
> dan carpenter
>
