Message ID | 7e4c380b853e31d6ec4b25db4c9812efa37b108c.1515940731.git.lorenzo.bianconi@redhat.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
Series | l2tp: set l2specific_len based on l2specific_type | expand |
On Sun, Jan 14, 2018 at 03:50:56PM +0100, Lorenzo Bianconi wrote: > --- a/net/l2tp/l2tp_core.h > +++ b/net/l2tp/l2tp_core.h > @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session) > l2tp_session_free(session); > } > > +static inline int l2tp_get_l2specific_len(struct l2tp_session *session) > +{ > + switch (session->l2specific_type) { > + case L2TP_L2SPECTYPE_NONE: > + return 0; > + case L2TP_L2SPECTYPE_DEFAULT: > + default: > + return 4; > + } > +} > The data path only compares ->l2specific_type to L2SPECTYPE_DEFAULT and treats any other value as L2SPECTYPE_NONE. Therefore, we should keep this logic here and return 0 for unknown types.
> On Sun, Jan 14, 2018 at 03:50:56PM +0100, Lorenzo Bianconi wrote: >> --- a/net/l2tp/l2tp_core.h >> +++ b/net/l2tp/l2tp_core.h >> @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session) >> l2tp_session_free(session); >> } >> >> +static inline int l2tp_get_l2specific_len(struct l2tp_session *session) >> +{ >> + switch (session->l2specific_type) { >> + case L2TP_L2SPECTYPE_NONE: >> + return 0; >> + case L2TP_L2SPECTYPE_DEFAULT: >> + default: >> + return 4; >> + } >> +} >> > The data path only compares ->l2specific_type to L2SPECTYPE_DEFAULT and > treats any other value as L2SPECTYPE_NONE. Therefore, we should keep > this logic here and return 0 for unknown types. The data path only compares l2specific_type to L2SPECTYPE_DEFAULT since in the other supported case (L2SPECTYPE_NONE) there is no action required. Moreover L2SPECTYPE_DEFAULT is default configured value if the user does not provide any value for l2specific_type so there are no 'unknown' types and I thought L2TP_L2SPECTYPE_DEFAULT was a better choice for default value
On Mon, Jan 15, 2018 at 07:43:18PM +0100, Lorenzo Bianconi wrote: > > On Sun, Jan 14, 2018 at 03:50:56PM +0100, Lorenzo Bianconi wrote: > >> --- a/net/l2tp/l2tp_core.h > >> +++ b/net/l2tp/l2tp_core.h > >> @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session) > >> l2tp_session_free(session); > >> } > >> > >> +static inline int l2tp_get_l2specific_len(struct l2tp_session *session) > >> +{ > >> + switch (session->l2specific_type) { > >> + case L2TP_L2SPECTYPE_NONE: > >> + return 0; > >> + case L2TP_L2SPECTYPE_DEFAULT: > >> + default: > >> + return 4; > >> + } > >> +} > >> > > The data path only compares ->l2specific_type to L2SPECTYPE_DEFAULT and > > treats any other value as L2SPECTYPE_NONE. Therefore, we should keep > > this logic here and return 0 for unknown types. > > The data path only compares l2specific_type to L2SPECTYPE_DEFAULT > since in the other supported case (L2SPECTYPE_NONE) there is no action > required. Moreover L2SPECTYPE_DEFAULT is default configured value if > the user does not provide any value for l2specific_type so there are > no 'unknown' types and I thought L2TP_L2SPECTYPE_DEFAULT was a better > choice for default value > Yes, but what I meant is that the data patch treats unknow values as L2SPECTYPE_NONE, while l2tp_get_l2specific_len() now treats them as L2TP_L2SPECTYPE_DEFAULT. I'd just prefer to avoid that inconsistency; it makes it easier to reason about the code. But if you really prefer L2SPECTYPE_DEFAULT, then fine. Unless someone messes with new l2spec types, we should never reach this case.
> On Mon, Jan 15, 2018 at 07:43:18PM +0100, Lorenzo Bianconi wrote: >> > On Sun, Jan 14, 2018 at 03:50:56PM +0100, Lorenzo Bianconi wrote: >> >> --- a/net/l2tp/l2tp_core.h >> >> +++ b/net/l2tp/l2tp_core.h >> >> @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session) >> >> l2tp_session_free(session); >> >> } >> >> >> >> +static inline int l2tp_get_l2specific_len(struct l2tp_session *session) >> >> +{ >> >> + switch (session->l2specific_type) { >> >> + case L2TP_L2SPECTYPE_NONE: >> >> + return 0; >> >> + case L2TP_L2SPECTYPE_DEFAULT: >> >> + default: >> >> + return 4; >> >> + } >> >> +} >> >> >> > The data path only compares ->l2specific_type to L2SPECTYPE_DEFAULT and >> > treats any other value as L2SPECTYPE_NONE. Therefore, we should keep >> > this logic here and return 0 for unknown types. >> >> The data path only compares l2specific_type to L2SPECTYPE_DEFAULT >> since in the other supported case (L2SPECTYPE_NONE) there is no action >> required. Moreover L2SPECTYPE_DEFAULT is default configured value if >> the user does not provide any value for l2specific_type so there are >> no 'unknown' types and I thought L2TP_L2SPECTYPE_DEFAULT was a better >> choice for default value >> > Yes, but what I meant is that the data patch treats unknow values as > L2SPECTYPE_NONE, while l2tp_get_l2specific_len() now treats them as > L2TP_L2SPECTYPE_DEFAULT. I'd just prefer to avoid that inconsistency; > it makes it easier to reason about the code. > > But if you really prefer L2SPECTYPE_DEFAULT, then fine. Unless someone > messes with new l2spec types, we should never reach this case. Yes, there are now way to hit just default case so there are no difference to use L2SPECTYPE_DEFAULT or L2SPECTYPE_NONE as default actually. Moreover from now on there are no 'unknow' values for l2specific_type. Anyway if you think that change is important I can respin a v3, no issue from my side. Regards, Lorenzo
On Mon, Jan 15, 2018 at 10:11:04PM +0100, Lorenzo Bianconi wrote: > > On Mon, Jan 15, 2018 at 07:43:18PM +0100, Lorenzo Bianconi wrote: > >> > On Sun, Jan 14, 2018 at 03:50:56PM +0100, Lorenzo Bianconi wrote: > >> >> --- a/net/l2tp/l2tp_core.h > >> >> +++ b/net/l2tp/l2tp_core.h > >> >> @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session) > >> >> l2tp_session_free(session); > >> >> } > >> >> > >> >> +static inline int l2tp_get_l2specific_len(struct l2tp_session *session) > >> >> +{ > >> >> + switch (session->l2specific_type) { > >> >> + case L2TP_L2SPECTYPE_NONE: > >> >> + return 0; > >> >> + case L2TP_L2SPECTYPE_DEFAULT: > >> >> + default: > >> >> + return 4; > >> >> + } > >> >> +} > >> >> > >> > The data path only compares ->l2specific_type to L2SPECTYPE_DEFAULT and > >> > treats any other value as L2SPECTYPE_NONE. Therefore, we should keep > >> > this logic here and return 0 for unknown types. > >> > >> The data path only compares l2specific_type to L2SPECTYPE_DEFAULT > >> since in the other supported case (L2SPECTYPE_NONE) there is no action > >> required. Moreover L2SPECTYPE_DEFAULT is default configured value if > >> the user does not provide any value for l2specific_type so there are > >> no 'unknown' types and I thought L2TP_L2SPECTYPE_DEFAULT was a better > >> choice for default value > >> > > Yes, but what I meant is that the data patch treats unknow values as > > L2SPECTYPE_NONE, while l2tp_get_l2specific_len() now treats them as > > L2TP_L2SPECTYPE_DEFAULT. I'd just prefer to avoid that inconsistency; > > it makes it easier to reason about the code. > > > > But if you really prefer L2SPECTYPE_DEFAULT, then fine. Unless someone > > messes with new l2spec types, we should never reach this case. > > Yes, there are now way to hit just default case so there are no > difference to use L2SPECTYPE_DEFAULT or L2SPECTYPE_NONE as default > actually. > Moreover from now on there are no 'unknow' values for l2specific_type. > Everything that isn't L2SPECTYPE_NONE or L2SPECTYPE_DEFAULT is unknown. The datapath reads them as L2SPECTYPE_NONE and l2tp_get_l2specific_len() as L2SPECTYPE_DEFAULT. That's inconsistent and one has to read the rest of the code to figure out that, in fact, that's not an issue because such values actually can't be configured. Using "session->l2specific_type == L2TP_L2SPECTYPE_DEFAULT ? 4 : 0" would have been enough IMO. > Anyway if you think that change is important I can respin a v3, no > issue from my side. > It's certainly not worth a v3 on its own. I just give you my preference, but the patchset is yours and I can live with either version.
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 62285fc6eb59..88efb8b845ca 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -730,11 +730,9 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb, "%s: recv data ns=%u, session nr=%u\n", session->name, ns, session->nr); } + ptr += 4; } - /* Advance past L2-specific header, if present */ - ptr += session->l2specific_len; - if (L2TP_SKB_CB(skb)->has_seq) { /* Received a packet with sequence numbers. If we're the LNS, * check if we sre sending sequence numbers and if not, @@ -1048,21 +1046,20 @@ static int l2tp_build_l2tpv3_header(struct l2tp_session *session, void *buf) memcpy(bufp, &session->cookie[0], session->cookie_len); bufp += session->cookie_len; } - if (session->l2specific_len) { - if (session->l2specific_type == L2TP_L2SPECTYPE_DEFAULT) { - u32 l2h = 0; - if (session->send_seq) { - l2h = 0x40000000 | session->ns; - session->ns++; - session->ns &= 0xffffff; - l2tp_dbg(session, L2TP_MSG_SEQ, - "%s: updated ns to %u\n", - session->name, session->ns); - } + if (session->l2specific_type == L2TP_L2SPECTYPE_DEFAULT) { + u32 l2h = 0; - *((__be32 *) bufp) = htonl(l2h); + if (session->send_seq) { + l2h = 0x40000000 | session->ns; + session->ns++; + session->ns &= 0xffffff; + l2tp_dbg(session, L2TP_MSG_SEQ, + "%s: updated ns to %u\n", + session->name, session->ns); } - bufp += session->l2specific_len; + + *((__be32 *)bufp) = htonl(l2h); + bufp += 4; } return bufp - optr; @@ -1719,7 +1716,7 @@ int l2tp_session_delete(struct l2tp_session *session) EXPORT_SYMBOL_GPL(l2tp_session_delete); /* We come here whenever a session's send_seq, cookie_len or - * l2specific_len parameters are set. + * l2specific_type parameters are set. */ void l2tp_session_set_header_len(struct l2tp_session *session, int version) { @@ -1728,7 +1725,8 @@ void l2tp_session_set_header_len(struct l2tp_session *session, int version) if (session->send_seq) session->hdr_len += 4; } else { - session->hdr_len = 4 + session->cookie_len + session->l2specific_len; + session->hdr_len = 4 + session->cookie_len; + session->hdr_len += l2tp_get_l2specific_len(session); if (session->tunnel->encap == L2TP_ENCAPTYPE_UDP) session->hdr_len += 4; } diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h index c2e9bbd79b35..06128a159a3c 100644 --- a/net/l2tp/l2tp_core.h +++ b/net/l2tp/l2tp_core.h @@ -302,6 +302,17 @@ static inline void l2tp_session_dec_refcount(struct l2tp_session *session) l2tp_session_free(session); } +static inline int l2tp_get_l2specific_len(struct l2tp_session *session) +{ + switch (session->l2specific_type) { + case L2TP_L2SPECTYPE_NONE: + return 0; + case L2TP_L2SPECTYPE_DEFAULT: + default: + return 4; + } +} + #define l2tp_printk(ptr, type, func, fmt, ...) \ do { \ if (((ptr)->debug) & (type)) \
Remove l2specific_len dependency while building l2tpv3 header or parsing the received frame since default L2-Specific Sublayer is always four bytes long and we don't need to rely on a user supplied value. Moreover in l2tp netlink code there are no sanity checks to enforce the relation between l2specific_len and l2specific_type, so sending a malformed netlink message is possible to set l2specific_type to L2TP_L2SPECTYPE_DEFAULT (or even L2TP_L2SPECTYPE_NONE) and set l2specific_len to a value greater than 4 leaking memory on the wire and sending corrupted frames. Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- net/l2tp/l2tp_core.c | 34 ++++++++++++++++------------------ net/l2tp/l2tp_core.h | 11 +++++++++++ 2 files changed, 27 insertions(+), 18 deletions(-)