Hi Dan,
On 1/23/24 11:43 AM, Dan Carpenter wrote:
Hello Daniel Borkmann and Benjamin Tissoires,
I've included both warnings because they're sort of related and
hopefully it will save time to have this discussion in one thread. I
recently added fdget() to my Smatch check for CVE-2023-1838 type
warnings and it generated the following output. I'm not an expert on
this stuff, I'm just a monkey see, monkey do programmer. I've filtered
out the obvious false positives but I'm not sure about these.
The patch e420bed02507: "bpf: Add fd-based tcx multi-prog infra with
link support" from Jul 19, 2023 and f5c27da4e3c8 ("HID: initial BPF
implementation") from Nov 3, 2022 introduce the following static
checker warnings:
drivers/hid/bpf/hid_bpf_dispatch.c:287 hid_bpf_attach_prog() warn: double fget(): 'prog_fd'
drivers/hid/bpf/hid_bpf_jmp_table.c:427 __hid_bpf_attach_prog() warn: fd re-used after fget(): 'prog_fd'
kernel/bpf/syscall.c:3985 bpf_prog_detach() warn: double fget(): 'attr->attach_bpf_fd'
kernel/bpf/syscall.c:3988 bpf_prog_detach() warn: double fget(): 'attr->attach_bpf_fd'
kernel/bpf/syscall.c:3991 bpf_prog_detach() warn: double fget(): 'attr->attach_bpf_fd'
kernel/bpf/syscall.c:4001 bpf_prog_detach() warn: double fget(): 'attr->attach_bpf_fd'
[...]
kernel/bpf/syscall.c
3956 static int bpf_prog_detach(const union bpf_attr *attr)
3957 {
3958 struct bpf_prog *prog = NULL;
3959 enum bpf_prog_type ptype;
3960 int ret;
3961
3962 if (CHECK_ATTR(BPF_PROG_DETACH))
3963 return -EINVAL;
3964
3965 ptype = attach_type_to_prog_type(attr->attach_type);
3966 if (bpf_mprog_supported(ptype)) {
3967 if (ptype == BPF_PROG_TYPE_UNSPEC)
3968 return -EINVAL;
3969 if (attr->attach_flags & ~BPF_F_ATTACH_MASK_MPROG)
3970 return -EINVAL;
3971 if (attr->attach_bpf_fd) {
3972 prog = bpf_prog_get_type(attr->attach_bpf_fd, ptype);
^^^^^^^^^^^^^^^^^^^
From my understanding then this prog might not be the same prog which
we detach from later...
Thanks for double checking, Dan! This branch above is only accessible when
bpf_mprog_supported(ptype) holds true which is as of today BPF_PROG_TYPE_SCHED_CLS.
3973 if (IS_ERR(prog))
3974 return PTR_ERR(prog);
3975 }
3976 } else if (attr->attach_flags ||
3977 attr->relative_fd ||
3978 attr->expected_revision) {
3979 return -EINVAL;
3980 }
3981
3982 switch (ptype) {
3983 case BPF_PROG_TYPE_SK_MSG:
3984 case BPF_PROG_TYPE_SK_SKB:
--> 3985 ret = sock_map_prog_detach(attr, ptype);
^^^^
here. Because instead of re-using prog we look it up again.
So we never enter into this switch case given the ptype.
3986 break;
3987 case BPF_PROG_TYPE_LIRC_MODE2:
3988 ret = lirc_prog_detach(attr);
3989 break;
3990 case BPF_PROG_TYPE_FLOW_DISSECTOR:
3991 ret = netns_bpf_prog_detach(attr, ptype);
3992 break;
3993 case BPF_PROG_TYPE_CGROUP_DEVICE:
3994 case BPF_PROG_TYPE_CGROUP_SKB:
3995 case BPF_PROG_TYPE_CGROUP_SOCK:
3996 case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
3997 case BPF_PROG_TYPE_CGROUP_SOCKOPT:
3998 case BPF_PROG_TYPE_CGROUP_SYSCTL:
3999 case BPF_PROG_TYPE_SOCK_OPS:
4000 case BPF_PROG_TYPE_LSM:
4001 ret = cgroup_bpf_prog_detach(attr, ptype);
4002 break;
4003 case BPF_PROG_TYPE_SCHED_CLS:
4004 if (attr->attach_type == BPF_TCX_INGRESS ||
4005 attr->attach_type == BPF_TCX_EGRESS)
4006 ret = tcx_prog_detach(attr, prog);
4007 else
4008 ret = netkit_prog_detach(attr, prog);
... only these two detach functions above.
4009 break;
4010 default:
4011 ret = -EINVAL;
4012 }
4013
4014 if (prog)
4015 bpf_prog_put(prog);
4016 return ret;
4017 }
Thanks,
Daniel
