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'
drivers/hid/bpf/hid_bpf_dispatch.c
256 noinline int
257 hid_bpf_attach_prog(unsigned int hid_id, int prog_fd, __u32 flags)
258 {
259 struct hid_device *hdev;
260 struct device *dev;
261 int fd, err, prog_type = hid_bpf_get_prog_attach_type(prog_fd);
^^^^^^^
fdget() here
262
263 if (!hid_bpf_ops)
264 return -EINVAL;
265
266 if (prog_type < 0)
267 return prog_type;
268
269 if (prog_type >= HID_BPF_PROG_TYPE_MAX)
We're doing checks to ensure that prog_type is correct
270 return -EINVAL;
271
272 if ((flags & ~HID_BPF_FLAG_MASK))
273 return -EINVAL;
274
275 dev = bus_find_device(hid_bpf_ops->bus_type, NULL, &hid_id, device_match_id);
276 if (!dev)
277 return -EINVAL;
278
279 hdev = to_hid_device(dev);
280
281 if (prog_type == HID_BPF_PROG_TYPE_DEVICE_EVENT) {
282 err = hid_bpf_allocate_event_data(hdev);
283 if (err)
284 return err;
285 }
286
287 fd = __hid_bpf_attach_prog(hdev, prog_type, prog_fd, flags);
^^^^^^^
But then we look it up again so it's not necessarily the same file.
288 if (fd < 0)
289 return fd;
290
291 if (prog_type == HID_BPF_PROG_TYPE_RDESC_FIXUP) {
292 err = hid_bpf_reconnect(hdev);
293 if (err) {
294 close_fd(fd);
295 return err;
296 }
297 }
298
299 return fd;
300 }
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...
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.
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);
4009 break;
4010 default:
4011 ret = -EINVAL;
4012 }
4013
4014 if (prog)
4015 bpf_prog_put(prog);
4016 return ret;
4017 }
regards,
dan carpenter
