Hi Dan,
On Tue, Jan 23, 2024 at 11:44 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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:
Not sure why e420bed02507 would introduce the warning in HID-BPF. The
double fget() you analyzed was present before that commit.
>
> 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.
Right. I did not want to have a too complex error code path there and
so I did not keep a fget() around :(
And I did not think this would be an attack vector (even if I have a
hard time getting how this could be used).
I'll send a patch later today to fix this.
>
> 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 }
Cheers,
Benjamin
