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 <gna...@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.
