Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 3 Dec 2021 09:05:46 +0300
From:      =?UTF-8?B?w5Z6a2FuIEtJUklL?= <ozkan.kirik@gmail.com>
To:        Kristof Provost <kp@freebsd.org>
Cc:        freebsd-pf@freebsd.org
Subject:   Re: git: 7f944794868f - stable/12 - pf: Introduce ridentifier
Message-ID:  <CAAcX-AHfWOcqEZBeFHGXo6VZ=ar6DyBEj1bvhjLHsX3UGCQVNw@mail.gmail.com>
In-Reply-To: <52E4AB7A-6D27-4B11-ABCD-94BB12D389F4@FreeBSD.org>
References:  <202111261940.1AQJeGLZ022802@gitrepo.freebsd.org> <CAAcX-AFshuFUo7g9q_S3%2B5iS3Ko_Y%2BD0M5_nNEMPWOeMp66T3Q@mail.gmail.com> <52E4AB7A-6D27-4B11-ABCD-94BB12D389F4@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help
Thank you Kristof. The patch works properly!

On Wed, Dec 1, 2021 at 11:58 PM Kristof Provost <kp@freebsd.org> wrote:
>
> On 1 Dec 2021, at 5:59, =C3=96zkan KIRIK wrote:
>
> Because tshark/wireshark don't know this header change yet.
>
> I=E2=80=99ve looked at the Wireshark parser code, and .. well, it=E2=80=
=99s wrong. It arbitrarily adds 3 bytes to the header length, and that=E2=
=80=99s not the correct solution. I=E2=80=99m not going to implement kernel=
 workarounds for application bugs.
>
> And even though tcpdump has been patched by this commit, it still
> cannot parse the packet properly also.
>
> Try this patch:
>
> diff --git a/sys/net/if_pflog.h b/sys/net/if_pflog.h
> index c77d8da1440a..93a69a2bb3a5 100644
> --- a/sys/net/if_pflog.h
> +++ b/sys/net/if_pflog.h
> @@ -31,6 +31,8 @@
>  #ifndef _NET_IF_PFLOG_H_
>  #define        _NET_IF_PFLOG_H_
>
> +#include <net/bpf.h>
> +
>  #define        PFLOGIFS_MAX    16
>
>  #define        PFLOG_RULESET_NAME_SIZE 16
> @@ -51,11 +53,13 @@ struct pfloghdr {
>         u_int8_t        dir;
>         u_int8_t        pad[3];
>         u_int32_t       ridentifier;
> +       u_int8_t        reserve;        /* Appease broken software like W=
ireshark. */
> +       u_int8_t        pad2[3];
>  };
>
> -#define        PFLOG_HDRLEN            sizeof(struct pfloghdr)
> +#define        PFLOG_HDRLEN            BPF_WORDALIGN(offsetof(struct pfl=
oghdr, pad2))
>  /* minus pad, also used as a signature */
> -#define        PFLOG_REAL_HDRLEN       offsetof(struct pfloghdr, pad)
> +#define        PFLOG_REAL_HDRLEN       offsetof(struct pfloghdr, pad2)
>
>  #ifdef _KERNEL
>  struct pf_rule;
> diff --git a/sys/netpfil/pf/if_pflog.c b/sys/netpfil/pf/if_pflog.c
> index 4853c1301d6f..5ccdf3a7dd45 100644
> --- a/sys/netpfil/pf/if_pflog.c
> +++ b/sys/netpfil/pf/if_pflog.c
> @@ -215,7 +215,8 @@ pflog_packet(struct pfi_kkif *kif, struct mbuf *m, sa=
_family_t af, u_int8_t dir,
>                 return (0);
>
>         bzero(&hdr, sizeof(hdr));
> -       hdr.length =3D PFLOG_HDRLEN;
> +       hdr.length =3D PFLOG_REAL_HDRLEN;
>         hdr.af =3D af;
>         hdr.action =3D rm->action;
>         hdr.reason =3D reason;
>
> It looks like I had assumed that the header was expected to be aligned to=
 4 bytes, but it=E2=80=99s actually expected to be aligned to sizeof(long).=
 This should fix that.
>
> I=E2=80=99ve gone one further and added a dummy field to arrange the upda=
ted struct so that Wireshark will work, essentially retaining its incorrect=
 assumption. It really should be fixed as well though.
>
> Kristof



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAAcX-AHfWOcqEZBeFHGXo6VZ=ar6DyBEj1bvhjLHsX3UGCQVNw>