I wonder if bpfwrite() can be fixed instead? Can you please share a reproducing bpf writing program?

Thank you for the question, glebius.

The reproducer is below. Compile and run as root on any interface that uses DLT_RAW (lo0 works):

cc -o repro-bpf-sa-len repro-bpf-sa-len.c
./repro-bpf-sa-len lo0

Output on FreeBSD 14.5-RELEASE:

write: Address family not supported by protocol family

This confirms the path: bpfwrite() -> bpf_movein() -> if_output with sa_len=0.

Regarding fixing bpfwrite() instead:

bpfwrite() already calls bzero(&dst, sizeof(dst)) before bpf_movein(), so sa_len=0 is the correct initial state for most DLT types. bpf_movein() sets sa_family for each DLT case but initializes sa_len in only one case: DLT_IEEE802_11_RADIO sets sa_len=12 with a XXX comment. All other DLT cases (DLT_RAW, DLT_EN10MB, DLT_NULL, DLT_PPP, DLT_FDDI, DLT_ATM_RFC1483) leave sa_len=0.

Fixing in bpfwrite()/bpf_movein() would require adding the correct sa_len value for each DLT case, which is more invasive and requires knowledge of the expected header size per link type. The fix in if_output is a defensive check at the point of use: if sa_len is too small to contain a valid af value, fall back to af=0 rather than reading past the end of sa_data.

I am open to the alternative approach if you prefer the fix to live in bpfwrite(). Happy to prepare a patch there instead if that is the direction you recommend.

Reproducer source:

/*
 * repro-bpf-sa-len.c
 * Reproduces sa_len=0 path in bpfwrite() -> if_output on FreeBSD.
 * Compile: cc -o repro-bpf-sa-len repro-bpf-sa-len.c
 * Run as root: ./repro-bpf-sa-len <interface>
 */
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/ioctl.h>
#include <net/bpf.h>
#include <net/if.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>

int
main(int argc, char *argv[])
{
        int fd;
        struct ifreq ifr;
        unsigned char pkt[] = {
                0x45, 0x00, 0x00, 0x14,
                0x00, 0x01, 0x00, 0x00,
                0x40, 0x00, 0x00, 0x00,
                0x7f, 0x00, 0x00, 0x01,
                0x7f, 0x00, 0x00, 0x01
        };

        if (argc < 2) {
                fprintf(stderr, "usage: %s <interface>\n", argv[0]);
                return 1;
        }

        fd = open("/dev/bpf", O_RDWR);
        if (fd < 0) { perror("open /dev/bpf"); return 1; }

        memset(&ifr, 0, sizeof(ifr));
        strlcpy(ifr.ifr_name, argv[1], sizeof(ifr.ifr_name));
        if (ioctl(fd, BIOCSETIF, &ifr) < 0) {
                perror("BIOCSETIF"); close(fd); return 1;
        }

        ssize_t n = write(fd, pkt, sizeof(pkt));
        if (n < 0)
                perror("write");
        else
                printf("wrote %zd bytes -- sa_len=0 path triggered\n", n);

        close(fd);
        return 0;
}

Thanks for the correction on DLT_NULL and for reviewing this carefully. You are right on both points.

The original reproducer was missing the 32-bit address-family header required for DLT_NULL. With AF_INET prepended in host byte order the packet injects correctly on lo0. I will verify this locally and update the reproducer in the experiment record.

On the patch itself: I understand now that bpf_ifnet_write() already ensures dst.sa_data contains the correct hlen bytes before calling if_output, so the sa_len check in the individual drivers is defending against a condition that cannot arise via that path. The contract is enforced upstream.

Regarding the MPASS(hlen <= sizeof(sa->sa_data)) suggestion: I would be glad to update D59140 to add that assertion in bpf_ifnet_write() as a hardening measure, replacing the current approach. Should I prepare that as an update to this revision or as a new diff?

In D59140#1368201, @ing.castellanosdz_gmail.com wrote:

Regarding the MPASS(hlen <= sizeof(sa->sa_data)) suggestion: I would be glad to update D59140 to add that assertion in bpf_ifnet_write() as a hardening measure, replacing the current approach. Should I prepare that as an update to this revision or as a new diff?

Yes, IMHO better to reuse this revision number so that all discussion stays in one place for history. However, you will need to update both the revision title and summary.

Updated as suggested: replaced the sa_len check in individual drivers
with MPASS(hlen <= sizeof(sockp->sa_data)) in bpf_movein() before the
bcopy. Summary updated to reflect the evolution of the revision.