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

[Guillaume Nault <g.nault@alphalink.fr>]

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.