| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220708190528.GB3166@debian.home>
Date: Fri, 8 Jul 2022 21:05:28 +0200
From: Guillaume Nault <gnault@...hat.com>
To: Marcin Szycik <marcin.szycik@...ux.intel.com>
Cc: netdev@...r.kernel.org, anthony.l.nguyen@...el.com,
davem@...emloft.net, xiyou.wangcong@...il.com,
jesse.brandeburg@...el.com, gustavoars@...nel.org,
baowen.zheng@...igine.com, boris.sukholitko@...adcom.com,
edumazet@...gle.com, kuba@...nel.org, jhs@...atatu.com,
jiri@...nulli.us, kurt@...utronix.de, pablo@...filter.org,
pabeni@...hat.com, paulb@...dia.com, simon.horman@...igine.com,
komachi.yoshiki@...il.com, zhangkaiheb@....com,
intel-wired-lan@...ts.osuosl.org,
michal.swiatkowski@...ux.intel.com, wojciech.drewek@...el.com,
alexandr.lobakin@...el.com, mostrows@...thlink.net,
paulus@...ba.org
Subject: Re: [RFC PATCH net-next v4 1/4] flow_dissector: Add PPPoE dissectors
On Fri, Jul 08, 2022 at 02:24:18PM +0200, Marcin Szycik wrote:
> From: Wojciech Drewek <wojciech.drewek@...el.com>
>
> Allow to dissect PPPoE specific fields which are:
> - session ID (16 bits)
> - ppp protocol (16 bits)
> - type (16 bits) - this is PPPoE ethertype, for now only
> ETH_P_PPP_SES is supported, possible ETH_P_PPP_DISC
> in the future
>
> The goal is to make the following TC command possible:
>
> # tc filter add dev ens6f0 ingress prio 1 protocol ppp_ses \
> flower \
> pppoe_sid 12 \
> ppp_proto ip \
> action drop
>
> Note that only PPPoE Session is supported.
>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>
> ---
> v4:
> * pppoe header validation
> * added MPLS dissection
> * added support for compressed ppp protocol field
> * flow_dissector_key_pppoe::session_id stored in __be16
> * new field: flow_dissector_key_pppoe::type
> v3: revert byte order changes in is_ppp_proto_supported from
> previous version
> v2: ntohs instead of htons in is_ppp_proto_supported
>
> include/net/flow_dissector.h | 13 ++++++
> net/core/flow_dissector.c | 84 +++++++++++++++++++++++++++++++++---
> 2 files changed, 90 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index a4c6057c7097..af0d429b9a26 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -261,6 +261,18 @@ struct flow_dissector_key_num_of_vlans {
> u8 num_of_vlans;
> };
>
> +/**
> + * struct flow_dissector_key_pppoe:
> + * @session_id: pppoe session id
> + * @ppp_proto: ppp protocol
> + * @type: pppoe eth type
> + */
> +struct flow_dissector_key_pppoe {
> + __be16 session_id;
> + __be16 ppp_proto;
> + __be16 type;
I don't understand the need for the new 'type' field.
> +};
> +
> enum flow_dissector_key_id {
> FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> @@ -291,6 +303,7 @@ enum flow_dissector_key_id {
> FLOW_DISSECTOR_KEY_CT, /* struct flow_dissector_key_ct */
> FLOW_DISSECTOR_KEY_HASH, /* struct flow_dissector_key_hash */
> FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */
> + FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe */
>
> FLOW_DISSECTOR_KEY_MAX,
> };
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 6aee04f75e3e..3a90219d2354 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -895,6 +895,42 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
> return result == BPF_OK;
> }
>
> +/**
> + * is_ppp_proto_supported - checks if inner PPP protocol should be dissected
> + * @proto: protocol type (PPP proto field)
> + */
> +static bool is_ppp_proto_supported(__be16 proto)
> +{
> + switch (proto) {
> + case htons(PPP_AT):
> + case htons(PPP_IPX):
> + case htons(PPP_VJC_COMP):
> + case htons(PPP_VJC_UNCOMP):
> + case htons(PPP_MP):
> + case htons(PPP_COMPFRAG):
> + case htons(PPP_COMP):
> + case htons(PPP_IPCP):
> + case htons(PPP_ATCP):
> + case htons(PPP_IPXCP):
> + case htons(PPP_IPV6CP):
> + case htons(PPP_CCPFRAG):
> + case htons(PPP_MPLSCP):
> + case htons(PPP_LCP):
> + case htons(PPP_PAP):
> + case htons(PPP_LQR):
> + case htons(PPP_CHAP):
> + case htons(PPP_CBCP):
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool is_pppoe_ses_hdr_valid(struct pppoe_hdr hdr)
> +{
> + return hdr.ver == 1 && hdr.type == 1 && hdr.code == 0;
> +}
> +
> /**
> * __skb_flow_dissect - extract the flow_keys struct and return it
> * @net: associated network namespace, derived from @skb if NULL
> @@ -1214,26 +1250,60 @@ bool __skb_flow_dissect(const struct net *net,
> struct pppoe_hdr hdr;
> __be16 proto;
> } *hdr, _hdr;
> + __be16 ppp_proto;
> +
> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
> if (!hdr) {
> fdret = FLOW_DISSECT_RET_OUT_BAD;
> break;
> }
>
> - nhoff += PPPOE_SES_HLEN;
> - switch (hdr->proto) {
> - case htons(PPP_IP):
> + if (!is_pppoe_ses_hdr_valid(hdr->hdr)) {
> + fdret = FLOW_DISSECT_RET_OUT_BAD;
> + break;
> + }
> +
> + /* least significant bit of the first byte
> + * indicates if protocol field was compressed
> + */
> + if (hdr->proto & 1) {
> + ppp_proto = hdr->proto << 8;
This is little endian specific code. We can't make such assumptions.
> + nhoff += PPPOE_SES_HLEN - 1;
> + } else {
> + ppp_proto = hdr->proto;
> + nhoff += PPPOE_SES_HLEN;
> + }
> +
> + if (ppp_proto == htons(PPP_IP)) {
> proto = htons(ETH_P_IP);
> fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> - break;
> - case htons(PPP_IPV6):
> + } else if (ppp_proto == htons(PPP_IPV6)) {
> proto = htons(ETH_P_IPV6);
> fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> - break;
> - default:
> + } else if (ppp_proto == htons(PPP_MPLS_UC)) {
> + proto = htons(ETH_P_MPLS_UC);
> + fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> + } else if (ppp_proto == htons(PPP_MPLS_MC)) {
> + proto = htons(ETH_P_MPLS_MC);
> + fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> + } else if (is_ppp_proto_supported(ppp_proto)) {
> + fdret = FLOW_DISSECT_RET_OUT_GOOD;
> + } else {
> fdret = FLOW_DISSECT_RET_OUT_BAD;
> break;
> }
> +
> + if (dissector_uses_key(flow_dissector,
> + FLOW_DISSECTOR_KEY_PPPOE)) {
> + struct flow_dissector_key_pppoe *key_pppoe;
> +
> + key_pppoe = skb_flow_dissector_target(flow_dissector,
> + FLOW_DISSECTOR_KEY_PPPOE,
> + target_container);
> + key_pppoe->session_id = hdr->hdr.sid;
> + key_pppoe->ppp_proto = ppp_proto;
> + key_pppoe->type = htons(ETH_P_PPP_SES);
> + }
> break;
> }
> case htons(ETH_P_TIPC): {
> --
> 2.35.1
>
Powered by blists - more mailing lists