| 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: <MW4PR11MB57763D9CD8EA3F31DB7E3E19FD899@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Wed, 13 Jul 2022 13:54:35 +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: Drewek, Wojciech
> Sent: środa, 13 lipca 2022 09:59
> To: Guillaume Nault <gnault@...hat.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
>
>
>
> > -----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.
I think this should work with both LE and BE arch, what do you think Guillaume?
We don't want to spam so much with next versions so maybe it is better
to ask earlier.
u16 ppp_proto;
ppp_proto = ntohs(hdr->proto);
if (ppp_proto & 256) {
ppp_proto = htons(ppp_proto >> 8);
nhoff += PPPOE_SES_HLEN - 1;
} else {
ppp_proto = htons(ppp_proto);
nhoff += PPPOE_SES_HLEN;
}
>
> > > 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