Bug 261566
| Summary: | Padding of DLT_PFLOG packets should be done differently | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Guy Harris <gharris> |
| Component: | kern | Assignee: | freebsd-bugs (Nobody) <bugs> |
| Status: | Closed FIXED | ||
| Severity: | Affects Some People | CC: | kp |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
Note also that, if you try to read a DLT_PFLOG capture with the OpenBSD struct pfloghdr, and round the length up to a multiple of 8, you will *NOT* correctly read it.
Here's version 1.29 of sys/net/if_pflog.h:
https://cvsweb.openbsd.org/src/sys/net/if_pflog.h?rev=1.29&content-type=text/x-cvsweb-markup
struct pf_addr is 16 bytes (containing a union big enough to hold either an IPv4 or an IPv6 address, as per version 1.505 of sys/net/pfvar.h:
https://cvsweb.openbsd.org/src/sys/net/pfvar.h?rev=1.505&content-type=text/x-cvsweb-markup
), IFNAMSIZ is 16, sa_family_t is 1 byte, and uid_t and pid_t are both 4 bytes, for a total of 1+1+1+1+16+16+4+4+4+4+4+4+1+1+1+1+16+16+2+2 = 100 bytes, which is *not* a multiple of 8, although it *is* a multiple of 4.
The new-style header was introduced in version 1.8:
https://cvsweb.openbsd.org/src/sys/net/if_pflog.h?rev=1.8&content-type=text/x-cvsweb-markup
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/if_pflog.h.diff?r1=1.7&r2=1.8&f=h
and they got rid of the "PFLOG_REAL_HDRLEN doesn't include the padding to a 4-byte boundary" stuff in version 1.16:
https://cvsweb.openbsd.org/src/sys/net/if_pflog.h?rev=1.16&content-type=text/x-cvsweb-markup
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/if_pflog.h.diff?r1=1.15&r2=1.16&f=h
Thanks for the report.
I distinctly recall looking at the Wireshark code before implementing the BPF_WORDALIGN(), however it clearly does `padded_length = WS_ROUNDUP_4(length);`, so I don't know what happened there.
How does this look to you:
diff --git a/sys/net/if_pflog.h b/sys/net/if_pflog.h
index 0406f78474a8..443c1cc36cf6 100644
--- a/sys/net/if_pflog.h
+++ b/sys/net/if_pflog.h
@@ -33,7 +33,6 @@
#include <sys/types.h>
-#include <net/bpf.h>
#include <net/if.h>
#define PFLOGIFS_MAX 16
@@ -60,7 +59,9 @@ struct pfloghdr {
u_int8_t pad2[3];
};
-#define PFLOG_HDRLEN BPF_WORDALIGN(offsetof(struct pfloghdr, pad2))
+#define PFLOG_ALIGNMENT sizeof(uint32_t)
+#define PFLOG_ALIGN(x) (((x) + PFLOG_ALIGNMENT - 1) & ~(PFLOG_ALIGNMENT - 1))
+#define PFLOG_HDRLEN PFLOG_ALIGN(offsetof(struct pfloghdr, pad2))
/* minus pad, also used as a signature */
#define PFLOG_REAL_HDRLEN offsetof(struct pfloghdr, pad2)
> I distinctly recall looking at the Wireshark code before implementing the BPF_WORDALIGN(), however it clearly does `padded_length = WS_ROUNDUP_4(length);`, so I don't know what happened there. It got fixed from "add 3 to the length under the assumption that there's 1 byte of padding" to "round it up to a multiple of 4". (Tcpdump now does the same thing, and libpcap now gets the pflog header length from the length field, rounding it up to a multiple of 4, rather than having a hardwired length based on whatever platform it was compiled on.) > How does this look to you: That should work. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=4daa31c10867b133bdc2a424e1e60d280384dc56 commit 4daa31c10867b133bdc2a424e1e60d280384dc56 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2022-02-01 07:56:49 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2022-02-01 17:17:44 +0000 pflog: align header to 4 bytes, not 8 6d4baa0d01 incorrectly rounded the lenght of the pflog header up to 8 bytes, rather than 4. PR: 261566 Reported by: Guy Harris <gharris@sonic.net> MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") sys/net/if_pflog.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=c6745a0cc40e77ed3b783a0a8050b42bdf682e57 commit c6745a0cc40e77ed3b783a0a8050b42bdf682e57 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2022-02-01 07:56:49 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2022-02-09 09:40:58 +0000 pflog: align header to 4 bytes, not 8 6d4baa0d01 incorrectly rounded the lenght of the pflog header up to 8 bytes, rather than 4. PR: 261566 Reported by: Guy Harris <gharris@sonic.net> MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 4daa31c10867b133bdc2a424e1e60d280384dc56) sys/net/if_pflog.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=275f412b070f0d23b028f8bde85182a1f6d0fe7c commit 275f412b070f0d23b028f8bde85182a1f6d0fe7c Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2022-02-01 07:56:49 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2022-02-09 09:56:36 +0000 pflog: align header to 4 bytes, not 8 6d4baa0d01 incorrectly rounded the lenght of the pflog header up to 8 bytes, rather than 4. PR: 261566 Reported by: Guy Harris <gharris@sonic.net> MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 4daa31c10867b133bdc2a424e1e60d280384dc56) sys/net/if_pflog.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) |
Currently, sys/net/if_pflog.h does #define PFLOG_HDRLEN BPF_WORDALIGN(offsetof(struct pfloghdr, pad2)) This is done only in FreeBSD; neither DragonFly BSD nor NetBSD nor OpenBSD nor Darwin use BPF_WORDALIGN() here, and, as FreeBSD's BPF_WORDALIGN() aligns to the size of a long, this means that the DLT_PFLOG packet format can differ depending on whether the machine writing the packet is 32-bit (ILP32) or 64-bit (LP64), which, given that neither the pcap file format nor the pcapng file format give any indication of the size of a long on the platform on which the file was written, means that the correct way to process a DLT_PFLOG packt in a given file cannot be determined from the file. The commit message for the commit that added BPF_WORDALIGN() was There were two issues with the new pflog packet length. The first is that the length is expected to be a multiple of sizeof(long), but we'd assumed it had to be a multiple of sizeof(uint32_t). but it gives not justification for the claim that "the length is expected to be a multiple of sizeof(long)". As indicated: 1) that's not the case on only *other* BSD-flavored OS with pflog and 2) that creates a packet format that can only be processed correctly by providing external information about the machine that wrote the file, which requires not only extra code, but extra time and energy on the part of whoever's trying to read the file, to determine the size of a long on the platform on which it was written and to supply that to the program reading the file. The other OSes rely on the compiler to add the padding as a consequence of the structure including some 32-bit integral values; that should suffice here, although if you want to make *sure* it's padded to a 4-byte boundary, you could use roundup2() from sys/param.h to do it rather than dragging in net/bpf.h. Note that 1) it's not clear that aligning on an 8-byte boundary will provide any performance improvement, as, even if you were to align the IP packet on an 8-byte boundary, there's no guarantee that this will put a significant number of 8-byte fields on an 8-byte boundary and 2) that will make a difference for two of the main packet analyzers that read pcap and pcapng files (tcpdump and Wireshark) only on CPUs that 1) support unaligned access but 2) impose a penalty for those accesses.