git: 7def047a1ae9 - main - bpf: Fix potential race conditions

The branch main has been updated by zlei:

URL: https://cgit.FreeBSD.org/src/commit/?id=7def047a1ae93b3b10bd57ed1bd28e861f94b596

commit 7def047a1ae93b3b10bd57ed1bd28e861f94b596
Author:     Zhenlei Huang <zlei@FreeBSD.org>
AuthorDate: 2025-02-03 12:13:19 +0000
Commit:     Zhenlei Huang <zlei@FreeBSD.org>
CommitDate: 2025-02-03 12:13:19 +0000

    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:          16d878cc99ef Fix the following bpf(4) race condition ...
    MFC after:      4 days
    Differential Revision:  https://reviews.freebsd.org/D45725
---
 sys/net/bpf.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/sys/net/bpf.c b/sys/net/bpf.c
index 657a34827bdb..a7d17109ed1a 100644
--- a/sys/net/bpf.c
+++ b/sys/net/bpf.c
@@ -2090,10 +2090,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.