| Message ID | 20190129061813.8097-1-jian.w.wen@oracle.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v3] net: l2tp: fix reading optional fields of L2TPv3 | expand |
On Tue, Jan 29, 2019 at 02:18:13PM +0800, Jacob Wen wrote: > Use pskb_may_pull() to make sure the optional fields are in skb linear > parts, so we can safely read them in l2tp_recv_common. > Looks fine to me. Just a few nitpicks. Not sure if they're worth a repost. But if you send a v4, you can: * Add the proper Fixes tag. * Drop 'net:' from the subsystem prefix ('l2tp:' is enough). * Move your patch history inside the commit description. * Keep my Acked-by tag. > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c > index 26f1d435696a..82c28008b438 100644 > --- a/net/l2tp/l2tp_core.c > +++ b/net/l2tp/l2tp_core.c > @@ -884,6 +884,10 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb) > goto error; > } > > + if (tunnel->version != L2TP_HDR_VER_2 && > Using tunnel->version == L2TP_HDR_VER_3 would have been clearer. > diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h > index 9c9afe94d389..870f8ccf95f7 100644 > --- a/net/l2tp/l2tp_core.h > +++ b/net/l2tp/l2tp_core.h > @@ -301,6 +301,27 @@ static inline bool l2tp_tunnel_uses_xfrm(const struct l2tp_tunnel *tunnel) > } > #endif > > +/* Pull optional fields of L2TPv3 */ > +static inline int l2tp_v3_pull_opt(struct l2tp_session *session, struct sk_buff *skb, > The comment and function name are a bit misleading: nothing is pulled here. > + unsigned char **ptr, unsigned char **optr) > +{ > + int opt_len = session->peer_cookie_len + l2tp_get_l2specific_len(session); > + > + if (opt_len > 0) { > + int off = *ptr - *optr; > + > + if (!pskb_may_pull(skb, off + opt_len)) > + return -1; > + > + if (skb->data != *optr) { > + *optr = skb->data; > + *ptr = skb->data + off; > + } > + } > + > + return 0; > +} > + > #define l2tp_printk(ptr, type, func, fmt, ...) \ > do { \ > if (((ptr)->debug) & (type)) \ Fixes: f7faffa3ff8e ("l2tp: Add L2TPv3 protocol support") Fixes: 0d76751fad77 ("l2tp: Add L2TPv3 IP encapsulation (no UDP) support") Fixes: a32e0eec7042 ("l2tp: introduce L2TPv3 IP encapsulation support for IPv6") Acked-by: Guillaume Nault <gnault@redhat.com> Thanks for reporting and fixing this. BTW, Do you plan to also fix L2TPv2? It looks like defining L2TP_HDR_SIZE_MAX to 14 (size of L2TPv2 header with all optional fields) and using it in place of L2TP_HDR_SIZE_SEQ in l2tp_udp_recv_core() should be enough.
Thanks for the detailed review. On 1/30/19 6:37 AM, Guillaume Nault wrote: > On Tue, Jan 29, 2019 at 02:18:13PM +0800, Jacob Wen wrote: >> Use pskb_may_pull() to make sure the optional fields are in skb linear >> parts, so we can safely read them in l2tp_recv_common. >> > Looks fine to me. Just a few nitpicks. Not sure if they're worth a repost. > But if you send a v4, you can: > * Add the proper Fixes tag. > * Drop 'net:' from the subsystem prefix ('l2tp:' is enough). > * Move your patch history inside the commit description. > * Keep my Acked-by tag. Done in v4. >> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c >> index 26f1d435696a..82c28008b438 100644 >> --- a/net/l2tp/l2tp_core.c >> +++ b/net/l2tp/l2tp_core.c >> @@ -884,6 +884,10 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb) >> goto error; >> } >> >> + if (tunnel->version != L2TP_HDR_VER_2 && >> > Using tunnel->version == L2TP_HDR_VER_3 would have been clearer. Ditto. > >> diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h >> index 9c9afe94d389..870f8ccf95f7 100644 >> --- a/net/l2tp/l2tp_core.h >> +++ b/net/l2tp/l2tp_core.h >> @@ -301,6 +301,27 @@ static inline bool l2tp_tunnel_uses_xfrm(const struct l2tp_tunnel *tunnel) >> } >> #endif >> >> +/* Pull optional fields of L2TPv3 */ >> +static inline int l2tp_v3_pull_opt(struct l2tp_session *session, struct sk_buff *skb, >> > The comment and function name are a bit misleading: nothing is pulled > here. > You are right. The misleading is inherited from pskb_may_pull. s/l2tp_v3_pull_opt/l2tp_v3_ensure_opt_in_linear/ in v4. > > BTW, Do you plan to also fix L2TPv2? > It looks like defining L2TP_HDR_SIZE_MAX to 14 (size of L2TPv2 header > with all optional fields) and using it in place of L2TP_HDR_SIZE_SEQ in > l2tp_udp_recv_core() should be enough. I will do that with your Suggested-by. Thanks.
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 26f1d435696a..82c28008b438 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -884,6 +884,10 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb) goto error; } + if (tunnel->version != L2TP_HDR_VER_2 && + l2tp_v3_pull_opt(session, skb, &ptr, &optr)) + goto error; + l2tp_recv_common(session, skb, ptr, optr, hdrflags, length); l2tp_session_dec_refcount(session); diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h index 9c9afe94d389..870f8ccf95f7 100644 --- a/net/l2tp/l2tp_core.h +++ b/net/l2tp/l2tp_core.h @@ -301,6 +301,27 @@ static inline bool l2tp_tunnel_uses_xfrm(const struct l2tp_tunnel *tunnel) } #endif +/* Pull optional fields of L2TPv3 */ +static inline int l2tp_v3_pull_opt(struct l2tp_session *session, struct sk_buff *skb, + unsigned char **ptr, unsigned char **optr) +{ + int opt_len = session->peer_cookie_len + l2tp_get_l2specific_len(session); + + if (opt_len > 0) { + int off = *ptr - *optr; + + if (!pskb_may_pull(skb, off + opt_len)) + return -1; + + if (skb->data != *optr) { + *optr = skb->data; + *ptr = skb->data + off; + } + } + + return 0; +} + #define l2tp_printk(ptr, type, func, fmt, ...) \ do { \ if (((ptr)->debug) & (type)) \ diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c index 35f6f86d4dcc..adc736d0a553 100644 --- a/net/l2tp/l2tp_ip.c +++ b/net/l2tp/l2tp_ip.c @@ -165,6 +165,9 @@ static int l2tp_ip_recv(struct sk_buff *skb) print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, ptr, length); } + if (l2tp_v3_pull_opt(session, skb, &ptr, &optr)) + goto discard_sess; + l2tp_recv_common(session, skb, ptr, optr, 0, skb->len); l2tp_session_dec_refcount(session); diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c index 237f1a4a0b0c..a023bb6eae03 100644 --- a/net/l2tp/l2tp_ip6.c +++ b/net/l2tp/l2tp_ip6.c @@ -178,6 +178,9 @@ static int l2tp_ip6_recv(struct sk_buff *skb) print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, ptr, length); } + if (l2tp_v3_pull_opt(session, skb, &ptr, &optr)) + goto discard_sess; + l2tp_recv_common(session, skb, ptr, optr, 0, skb->len); l2tp_session_dec_refcount(session);
Use pskb_may_pull() to make sure the optional fields are in skb linear parts, so we can safely read them in l2tp_recv_common. It's easy to reproduce the issue with a net driver that supports paged skb data. Just create a L2TPv3 over IP tunnel and then generates some network traffic. Once reproduced, rx err in /sys/kernel/debug/l2tp/tunnels will increase. Signed-off-by: Jacob Wen <jian.w.wen@oracle.com> --- Changes in v3: 1. To keep consistency, move the code out of l2tp_recv_common. 2. Use "net" instead of "net-next", since this is a bug fix. Changes in v2: 1. Only fix L2TPv3 to make code simple. To fix both L2TPv3 and L2TPv2, we'd better refactor l2tp_recv_common. It's complicated to do so. 2. Reloading pointers after pskb_may_pull --- net/l2tp/l2tp_core.c | 4 ++++ net/l2tp/l2tp_core.h | 21 +++++++++++++++++++++ net/l2tp/l2tp_ip.c | 3 +++ net/l2tp/l2tp_ip6.c | 3 +++ 4 files changed, 31 insertions(+)