mirror of
https://github.com/opnsense/src.git
synced 2026-05-28 04:12:45 -04:00
bpf: Fix potential race conditions
There're two possible race conditions,
1. Concurrent bpfattach() and bpf_setif(), i.e., BIOCSETIF ioctl,
2. Concurrent bpfdetach() and bpf_setif().
For the first case, userland may see BPF interface attached but it has
not been in the attached interfaces list `bpf_iflist` yet. Well it
will eventually be so this case does not matter.
For the second one, bpf_setif() may reference `dead_bpf_if` and the
kernel will panic (spotted by change [1], without the change we will
end up silently corrupted memory).
A simple fix could be that, we add additional check for `dead_bpf_if`
in the function `bpf_setif()`. But that requires to extend protection
of global lock (BPF_LOCK), i.e., BPF_LOCK should also protect the
assignment of `ifp->if_bpf`. That simple fix works but is apparently
not a good design. Since the attached interfaces list `bpf_iflist` is
the single source of truth, we look through it rather than check
against the interface's side, aka `ifp->if_bpf`.
This change has performance regression, that the cost of BPF interface
attach operation (BIOCSETIF ioctl) goes back from O(1) to O(N) (where
N is the number of BPF interfaces). Well we normally have sane amounts
of interfaces, an O(N) should be affordable.
[1] 7a974a649848 bpf: Make dead_bpf_if const
Fixes: 16d878cc99 Fix the following bpf(4) race condition ...
MFC after: 4 days
Differential Revision: https://reviews.freebsd.org/D45725
(cherry picked from commit 7def047a1ae93b3b10bd57ed1bd28e861f94b596)
This commit is contained in:
parent
2eba2832e9
commit
067b29595a
1 changed files with 12 additions and 2 deletions
|
|
@ -2092,10 +2092,20 @@ bpf_setif(struct bpf_d *d, struct ifreq *ifr)
|
|||
BPF_LOCK_ASSERT();
|
||||
|
||||
theywant = ifunit(ifr->ifr_name);
|
||||
if (theywant == NULL || theywant->if_bpf == NULL)
|
||||
if (theywant == NULL)
|
||||
return (ENXIO);
|
||||
/*
|
||||
* Look through attached interfaces for the named one.
|
||||
*/
|
||||
CK_LIST_FOREACH(bp, &bpf_iflist, bif_next) {
|
||||
if (bp->bif_ifp == theywant &&
|
||||
bp->bif_bpf == &theywant->if_bpf)
|
||||
break;
|
||||
}
|
||||
if (bp == NULL)
|
||||
return (ENXIO);
|
||||
|
||||
bp = theywant->if_bpf;
|
||||
MPASS(bp == theywant->if_bpf);
|
||||
/*
|
||||
* At this point, we expect the buffer is already allocated. If not,
|
||||
* return an error.
|
||||
|
|
|
|||
Loading…
Reference in a new issue