[PATCH net-next 0/3] set l2specific_len based on l2specific_type

[PATCH net-next 0/3] set l2specific_len based on l2specific_type

[Lorenzo Bianconi]

Do not rely on l2specific_len value provided by userspace but set sublayer
length according to l2specific_type.

Lorenzo Bianconi (3):
  l2tp: fix switch default error handling in
    l2tp_nl_cmd_session_create()
  l2tp: configure l2specific_len according to l2specific_type
  l2tp: mark L2TP_ATTR_L2SPEC_LEN as not used

 include/uapi/linux/l2tp.h |  2 +-
 net/l2tp/l2tp_netlink.c   | 23 +++++++++++++++++------
 2 files changed, 18 insertions(+), 7 deletions(-)

-- 
2.13.6

[PATCH net-next 1/3] l2tp: fix switch default error handling in l2tp_nl_cmd_session_create()

[Lorenzo Bianconi]

Although this issue is harmless since that code path is protected by the
check on l2tp_nl_cmd_ops[]/l2tp_nl_cmd_ops[]->session_create(), fix error
handling for L2TP_PWTYPE_IP/default case in l2tp_nl_cmd_session_create()

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 net/l2tp/l2tp_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index e1ca29f79821..48b5bf30ec50 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -635,7 +635,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 	case L2TP_PWTYPE_IP:
 	default:
 		ret = -EPROTONOSUPPORT;
-		break;
+		goto out_tunnel;
 	}
 
 	ret = l2tp_nl_cmd_ops[cfg.pw_type]->session_create(net, tunnel,
-- 
2.13.6

[PATCH net-next 2/3] l2tp: configure l2specific_len according to l2specific_type

[Lorenzo Bianconi]

Set l2specific_len according to the L2-Specific Sublayer type specified
by the userspace and not rely on a user supplied value for sublayer len
since invalid usage of L2TP_ATTR_L2SPEC_LEN allows leaking memory on the
network and sending corrupted frames.
Moreover add a sanity check on l2specific_type provided by userspace

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 net/l2tp/l2tp_netlink.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 48b5bf30ec50..404e5482c4e7 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -550,13 +550,24 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 		if (info->attrs[L2TP_ATTR_DATA_SEQ])
 			cfg.data_seq = nla_get_u8(info->attrs[L2TP_ATTR_DATA_SEQ]);
 
-		cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
-		if (info->attrs[L2TP_ATTR_L2SPEC_TYPE])
+		if (info->attrs[L2TP_ATTR_L2SPEC_TYPE]) {
 			cfg.l2specific_type = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_TYPE]);
 
-		cfg.l2specific_len = 4;
-		if (info->attrs[L2TP_ATTR_L2SPEC_LEN])
-			cfg.l2specific_len = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_LEN]);
+			switch (cfg.l2specific_type) {
+			case L2TP_L2SPECTYPE_DEFAULT:
+				cfg.l2specific_len = 4;
+				break;
+			case L2TP_L2SPECTYPE_NONE:
+				cfg.l2specific_len = 0;
+				break;
+			default:
+				ret = -EINVAL;
+				goto out_tunnel;
+			}
+		} else {
+			cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
+			cfg.l2specific_len = 4;
+		}
 
 		if (info->attrs[L2TP_ATTR_COOKIE]) {
 			u16 len = nla_len(info->attrs[L2TP_ATTR_COOKIE]);
-- 
2.13.6

[PATCH net-next 3/3] l2tp: mark L2TP_ATTR_L2SPEC_LEN as not used

[Lorenzo Bianconi]

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 include/uapi/linux/l2tp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/l2tp.h b/include/uapi/linux/l2tp.h
index 71e62795104d..ec609a017691 100644
--- a/include/uapi/linux/l2tp.h
+++ b/include/uapi/linux/l2tp.h
@@ -97,7 +97,7 @@ enum {
 	L2TP_ATTR_OFFSET,		/* u16 (not used) */
 	L2TP_ATTR_DATA_SEQ,		/* u16 */
 	L2TP_ATTR_L2SPEC_TYPE,		/* u8, enum l2tp_l2spec_type */
-	L2TP_ATTR_L2SPEC_LEN,		/* u8, enum l2tp_l2spec_type */
+	L2TP_ATTR_L2SPEC_LEN,		/* u8, (not used) */
 	L2TP_ATTR_PROTO_VERSION,	/* u8 */
 	L2TP_ATTR_IFNAME,		/* string */
 	L2TP_ATTR_CONN_ID,		/* u32 */
-- 
2.13.6

Re: [PATCH net-next 2/3] l2tp: configure l2specific_len according to l2specific_type

[Guillaume Nault]

On Wed, Jan 10, 2018 at 06:20:41PM +0100, Lorenzo Bianconi wrote:
> Set l2specific_len according to the L2-Specific Sublayer type specified
> by the userspace and not rely on a user supplied value for sublayer len
> since invalid usage of L2TP_ATTR_L2SPEC_LEN allows leaking memory on the
> network and sending corrupted frames.
> Moreover add a sanity check on l2specific_type provided by userspace
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>  net/l2tp/l2tp_netlink.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
> index 48b5bf30ec50..404e5482c4e7 100644
> --- a/net/l2tp/l2tp_netlink.c
> +++ b/net/l2tp/l2tp_netlink.c
> @@ -550,13 +550,24 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
>  		if (info->attrs[L2TP_ATTR_DATA_SEQ])
>  			cfg.data_seq = nla_get_u8(info->attrs[L2TP_ATTR_DATA_SEQ]);
>  
> -		cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
> -		if (info->attrs[L2TP_ATTR_L2SPEC_TYPE])
> +		if (info->attrs[L2TP_ATTR_L2SPEC_TYPE]) {
>  			cfg.l2specific_type = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_TYPE]);
>  
> -		cfg.l2specific_len = 4;
> -		if (info->attrs[L2TP_ATTR_L2SPEC_LEN])
> -			cfg.l2specific_len = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_LEN]);
> +			switch (cfg.l2specific_type) {
> +			case L2TP_L2SPECTYPE_DEFAULT:
> +				cfg.l2specific_len = 4;
> +				break;
> +			case L2TP_L2SPECTYPE_NONE:
> +				cfg.l2specific_len = 0;
> +				break;
> +			default:
> +				ret = -EINVAL;
> +				goto out_tunnel;
> +			}
> +		} else {
> +			cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
> +			cfg.l2specific_len = 4;
> +		}
>  
>  		if (info->attrs[L2TP_ATTR_COOKIE]) {
>  			u16 len = nla_len(info->attrs[L2TP_ATTR_COOKIE]);
> 
I think we can go one step further and remove .l2specific_len from
struct l2tp_session and struct l2tp_session_cfg. We never need this
information. Then we can start ignoring the L2TP_ATTR_L2SPEC_LEN
attribute just like we've done with L2TP_ATTR_OFFSET.

Re: [PATCH net-next 3/3] l2tp: mark L2TP_ATTR_L2SPEC_LEN as not used

[Guillaume Nault]

On Wed, Jan 10, 2018 at 06:20:42PM +0100, Lorenzo Bianconi wrote:
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>  include/uapi/linux/l2tp.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/l2tp.h b/include/uapi/linux/l2tp.h
> index 71e62795104d..ec609a017691 100644
> --- a/include/uapi/linux/l2tp.h
> +++ b/include/uapi/linux/l2tp.h
> @@ -97,7 +97,7 @@ enum {
>  	L2TP_ATTR_OFFSET,		/* u16 (not used) */
>  	L2TP_ATTR_DATA_SEQ,		/* u16 */
>  	L2TP_ATTR_L2SPEC_TYPE,		/* u8, enum l2tp_l2spec_type */
> -	L2TP_ATTR_L2SPEC_LEN,		/* u8, enum l2tp_l2spec_type */
> +	L2TP_ATTR_L2SPEC_LEN,		/* u8, (not used) */
> 
Nitpick: if you repost this series, you can remove the comma between 'u8'
and '(not used)' (to keep the format used by L2TP_ATTR_OFFSET).

Re: [PATCH net-next 2/3] l2tp: configure l2specific_len according to l2specific_type

[Lorenzo Bianconi]

> On Wed, Jan 10, 2018 at 06:20:41PM +0100, Lorenzo Bianconi wrote:
>> Set l2specific_len according to the L2-Specific Sublayer type specified
>> by the userspace and not rely on a user supplied value for sublayer len
>> since invalid usage of L2TP_ATTR_L2SPEC_LEN allows leaking memory on the
>> network and sending corrupted frames.
>> Moreover add a sanity check on l2specific_type provided by userspace
>>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> ---
>>  net/l2tp/l2tp_netlink.c | 21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
>> index 48b5bf30ec50..404e5482c4e7 100644
>> --- a/net/l2tp/l2tp_netlink.c
>> +++ b/net/l2tp/l2tp_netlink.c
>> @@ -550,13 +550,24 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
>>               if (info->attrs[L2TP_ATTR_DATA_SEQ])
>>                       cfg.data_seq = nla_get_u8(info->attrs[L2TP_ATTR_DATA_SEQ]);
>>
>> -             cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
>> -             if (info->attrs[L2TP_ATTR_L2SPEC_TYPE])
>> +             if (info->attrs[L2TP_ATTR_L2SPEC_TYPE]) {
>>                       cfg.l2specific_type = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_TYPE]);
>>
>> -             cfg.l2specific_len = 4;
>> -             if (info->attrs[L2TP_ATTR_L2SPEC_LEN])
>> -                     cfg.l2specific_len = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_LEN]);
>> +                     switch (cfg.l2specific_type) {
>> +                     case L2TP_L2SPECTYPE_DEFAULT:
>> +                             cfg.l2specific_len = 4;
>> +                             break;
>> +                     case L2TP_L2SPECTYPE_NONE:
>> +                             cfg.l2specific_len = 0;
>> +                             break;
>> +                     default:
>> +                             ret = -EINVAL;
>> +                             goto out_tunnel;
>> +                     }
>> +             } else {
>> +                     cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
>> +                     cfg.l2specific_len = 4;
>> +             }
>>
>>               if (info->attrs[L2TP_ATTR_COOKIE]) {
>>                       u16 len = nla_len(info->attrs[L2TP_ATTR_COOKIE]);
>>
> I think we can go one step further and remove .l2specific_len from
> struct l2tp_session and struct l2tp_session_cfg. We never need this
> information. Then we can start ignoring the L2TP_ATTR_L2SPEC_LEN
> attribute just like we've done with L2TP_ATTR_OFFSET.

Hi Guillaume,

I was thinking about it, let's way for some feedbacks and then I can
respin a v2.
Regards,

Lorenzo