Hello Sergey.
I am not the developer, but I works in pipex(4) layer too. Also mpi@
wants I did rewiev for your diffs.
On Mon, May 04, 2020 at 10:03:40PM +0300, Sergey Ryazanov wrote:
> Split checks from frame accepting with header removing in the common
> PPP input function. This should fix packet capture on a PPP interfaces.
Can you describe the problem you fix? As mpi@ pointed to me, reviewers
are stupid and have no telepathy skills :)
Also your diff does two different things: (1) split frames checks and
input, and (2) modify frames passing to bpf(4). I guess you should split
your diff by two different.
I did some inlined comments below.
>
> Also forbid IP/IPv6 frames (without PPP header) passing to BPF on
> PPP interfaces to avoid mess.
>
> Initialy this change was made as a part of pipex(4) and ppp(4)
> integration work. But, since this change make the core a bit more clear
> I would like to publish it now.
>
> Ok?
>
> ---
> sys/net/pipex.c | 95 ++++++++++++++++++++++++++++---------------------
> 1 file changed, 54 insertions(+), 41 deletions(-)
>
> diff --git sys/net/pipex.c sys/net/pipex.c
> index c433e4beaa6..e0066a61598 100644
> --- sys/net/pipex.c
> +++ sys/net/pipex.c
> @@ -970,41 +970,68 @@ drop:
> Static void
> pipex_ppp_input(struct mbuf *m0, struct pipex_session *session, int
> decrypted)
> {
> - int proto, hlen = 0;
> + int proto, hlen = 0, align = 0;
> struct mbuf *n;
>
> KASSERT(m0->m_pkthdr.len >= PIPEX_PPPMINLEN);
> proto = pipex_ppp_proto(m0, session, 0, &hlen);
> + switch (proto) {
> + case PPP_IP:
> + if (session->ip_forward == 0)
> + goto drop;
> + if (!decrypted && pipex_session_is_mppe_required(session))
> + /*
> + * if ip packet received when mppe
> + * is required, discard it.
> + */
> + goto drop;
> + align = 1;
> + break;
> +#ifdef INET6
> + case PPP_IPV6:
> + if (session->ip6_forward == 0)
> + goto drop;
> + if (!decrypted && pipex_session_is_mppe_required(session))
> + /*
> + * if ip packet received when mppe
> + * is required, discard it.
> + */
> + goto drop;
> + align = 1;
> + break;
> +#endif
> #ifdef PIPEX_MPPE
> - if (proto == PPP_COMP) {
> + case PPP_COMP:
> if (decrypted)
> goto drop;
>
> /* checked this on ppp_common_input() already. */
> KASSERT(pipex_session_is_mppe_accepted(session));
> -
> - m_adj(m0, hlen);
> - pipex_mppe_input(m0, session);
> - return;
> - }
> - if (proto == PPP_CCP) {
> + break;
> + case PPP_CCP:
> if (decrypted)
> goto drop;
> + break;
> +#endif
> + default:
> + if (decrypted)
> + goto drop;
> + /* protocol must be checked on pipex_common_input() already */
> + KASSERT(0);
> + goto drop;
> + }
>
> #if NBPFILTER > 0
> - {
> + {
> struct ifnet *ifp = session->pipex_iface->ifnet_this;
> +
> if (ifp->if_bpf && ifp->if_type == IFT_PPP)
> bpf_mtap(ifp->if_bpf, m0, BPF_DIRECTION_IN);
> - }
> -#endif
> - m_adj(m0, hlen);
> - pipex_ccp_input(m0, session);
> - return;
> }
> #endif
For INET[46] cases you can align frame after it's passed to bpf(4). Should
this frame be aligned before passing to bpf(4)?
> +
> m_adj(m0, hlen);
> - if (!ALIGNED_POINTER(mtod(m0, caddr_t), uint32_t)) {
> + if (align && !ALIGNED_POINTER(mtod(m0, caddr_t), uint32_t)) {
> n = m_dup_pkt(m0, 0, M_NOWAIT);
> if (n == NULL)
> goto drop;
> @@ -1014,35 +1041,21 @@ pipex_ppp_input(struct mbuf *m0, struct pipex_session
> *session, int decrypted)
>
> switch (proto) {
> case PPP_IP:
> - if (session->ip_forward == 0)
> - goto drop;
> - if (!decrypted && pipex_session_is_mppe_required(session))
> - /*
> - * if ip packet received when mppe
> - * is required, discard it.
> - */
> - goto drop;
> pipex_ip_input(m0, session);
> - return;
> + break;
> #ifdef INET6
> case PPP_IPV6:
> - if (session->ip6_forward == 0)
> - goto drop;
> - if (!decrypted && pipex_session_is_mppe_required(session))
> - /*
> - * if ip packet received when mppe
> - * is required, discard it.
> - */
> - goto drop;
> pipex_ip6_input(m0, session);
> - return;
> + break;
> +#endif
> +#ifdef PIPEX_MPPE
> + case PPP_COMP:
> + pipex_mppe_input(m0, session);
> + break;
> + case PPP_CCP:
> + pipex_ccp_input(m0, session);
> + break;
> #endif
> - default:
> - if (decrypted)
> - goto drop;
> - /* protocol must be checked on pipex_common_input() already */
> - KASSERT(0);
> - goto drop;
Hipotethically, someone inncaurate can introduce memory leak here in
future. I like to keep default case with assertion or with "goto drop".
> }
>
> return;
> @@ -1105,7 +1118,7 @@ pipex_ip_input(struct mbuf *m0, struct pipex_session
> *session)
> len = m0->m_pkthdr.len;
>
> #if NBPFILTER > 0
> - if (ifp->if_bpf)
> + if (ifp->if_bpf && ifp->if_type != IFT_PPP)
> bpf_mtap_af(ifp->if_bpf, AF_INET, m0, BPF_DIRECTION_IN);
> #endif
>
> @@ -1151,7 +1164,7 @@ pipex_ip6_input(struct mbuf *m0, struct pipex_session
> *session)
> len = m0->m_pkthdr.len;
>
> #if NBPFILTER > 0
> - if (ifp->if_bpf)
> + if (ifp->if_bpf && ifp->if_type != IFT_PPP)
> bpf_mtap_af(ifp->if_bpf, AF_INET6, m0, BPF_DIRECTION_IN);
> #endif
>
> --
> 2.26.0
>
