| 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: <MW4PR11MB577640BD1BAC97D3BB27A339FD899@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Wed, 13 Jul 2022 07:58:59 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: Guillaume Nault <gnault@...hat.com>
CC: Marcin Szycik <marcin.szycik@...ux.intel.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
"gustavoars@...nel.org" <gustavoars@...nel.org>,
"baowen.zheng@...igine.com" <baowen.zheng@...igine.com>,
"boris.sukholitko@...adcom.com" <boris.sukholitko@...adcom.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"jhs@...atatu.com" <jhs@...atatu.com>,
"jiri@...nulli.us" <jiri@...nulli.us>,
"kurt@...utronix.de" <kurt@...utronix.de>,
"pablo@...filter.org" <pablo@...filter.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"paulb@...dia.com" <paulb@...dia.com>,
"simon.horman@...igine.com" <simon.horman@...igine.com>,
"komachi.yoshiki@...il.com" <komachi.yoshiki@...il.com>,
"zhangkaiheb@....com" <zhangkaiheb@....com>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"michal.swiatkowski@...ux.intel.com"
<michal.swiatkowski@...ux.intel.com>,
"Lobakin, Alexandr" <alexandr.lobakin@...el.com>,
"mostrows@...thlink.net" <mostrows@...thlink.net>,
"paulus@...ba.org" <paulus@...ba.org>
Subject: RE: [RFC PATCH net-next v4 1/4] flow_dissector: Add PPPoE dissectors
> -----Original Message-----
> From: Guillaume Nault <gnault@...hat.com>
> Sent: wtorek, 12 lipca 2022 19:20
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: Marcin Szycik <marcin.szycik@...ux.intel.com>; netdev@...r.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> davem@...emloft.net; xiyou.wangcong@...il.com; Brandeburg, Jesse <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; Lobakin,
> Alexandr <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 Mon, Jul 11, 2022 at 10:23:50AM +0000, Drewek, Wojciech wrote:
> > > > 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.
> >
> > Let's say user want to add below filter with just protocol field:
> > tc filter add dev ens6f0 ingress prio 1 protocol ppp_ses action drop
> >
> > cls_flower would set basic.n_proto to ETH_P_PPP_SES, then PPPoE packet
> > arrives with ppp_proto = PPP_IP, which means that in __skb_flow_dissect basic.n_proto is going to
> > be set to ETH_P_IP. We have a mismatch here cls_flower set basic.n_proto to ETH_P_PPP_SES and
> > flow_dissector set it to ETH_P_IP. That's why in such example basic.n_proto has to be set to 0 (it works the same
> > with vlans) and key_pppoe::type has to be used. In other words basic.n_proto can't be used for storing
> > ETH_P_PPP_SES because it will store encapsulated protocol.
> >
> > We could also use it to match on ETH_P_PPP_DISC.
>
> Thanks for the explanation. That makes sense.
>
> > > > @@ -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.
> >
> > Both ppp_proto and hdr->prot are stored in __be16 so left shift by 8 bits
> > should always be ok, am I right?
>
> Sorry, I don't understand. How could the test and the bit shift
> operation give the correct result on a big endian machine?
>
> Let's say we handle an IPv4 packet and the PPP protocol field isn't
> compressed. That is, protocol is 0x0021.
> On a big endian machine 'hdr->proto & 1' is true and the bit shift sets
> ppp_proto to 0x2100, while the code should have left the original value
> untouched.
>
Ok, I see now, we'll fix it in the next version.
> > Should I use cpu_to_be16 on both 1 and 8. Is that what you mean?
>
> I can't see how cpu_to_be16() could help here. I was thinking of simply
> using ntohs(hdr->proto).
Powered by blists - more mailing lists