netdev - Re: [PATCH net v3] net: l2tp: fix reading optional fields of L2TPv3

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2712b823-1bf7-fd95-12ff-e9c254d95352@oracle.com>
Date:   Wed, 30 Jan 2019 15:06:04 +0800
From:   Jacob Wen <jian.w.wen@...cle.com>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net v3] net: l2tp: fix reading optional fields of L2TPv3

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.

-- 
Jacob