Re: [devel-ipsec] [PATCH ipsec-next v5 05/17] xfrm: netlink: add config (netlink) options

[Christian Hopps <chopps@chopps.org>]

[-- Attachment #1: Type: text/plain, Size: 3807 bytes --]


Sabrina Dubroca via Devel <devel@linux-ipsec.org> writes:

> 2024-07-14, 16:22:33 -0400, Christian Hopps wrote:
>> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
>> index a552cfa623ea..d42805314a2a 100644
>> --- a/net/xfrm/xfrm_user.c
>> +++ b/net/xfrm/xfrm_user.c
>> @@ -297,6 +297,16 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
>>  			NL_SET_ERR_MSG(extack, "TFC padding can only be used in tunnel mode");
>>  			goto out;
>>  		}
>> +		if ((attrs[XFRMA_IPTFS_DROP_TIME] ||
>> +		     attrs[XFRMA_IPTFS_REORDER_WINDOW] ||
>> +		     attrs[XFRMA_IPTFS_DONT_FRAG] ||
>> +		     attrs[XFRMA_IPTFS_INIT_DELAY] ||
>> +		     attrs[XFRMA_IPTFS_MAX_QSIZE] ||
>> +		     attrs[XFRMA_IPTFS_PKT_SIZE]) &&
>> +		    p->mode != XFRM_MODE_IPTFS) {
>> +			NL_SET_ERR_MSG(extack, "IP-TFS options can only be used in IP-TFS mode");
>
> AFAICT this only excludes the IPTFS options from ESP with a non-IPTFS
> mode, but not from AH, IPcomp, etc.

Ok, the change I'll make here is to only allow IPTFS mode selection for proto == IPPROTO_ESP (as that reflects reality). This handles the above issue as well as adds an additional useful check.

>
>> +			goto out;
>> +		}
>>  		break;
>>
>>  	case IPPROTO_COMP:
>> @@ -417,6 +427,18 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
>>  			goto out;
>>  		}
>>
>> +		if (attrs[XFRMA_IPTFS_DROP_TIME]) {
>> +			NL_SET_ERR_MSG(extack, "Drop time should not be set for output SA");
>
> Maybe add "IPTFS" to all those error messages, to help narrow down the
> bogus attribute.

Done.

>
>> +			err = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		if (attrs[XFRMA_IPTFS_REORDER_WINDOW]) {
>> +			NL_SET_ERR_MSG(extack, "Reorder window should not be set for output SA");
>> +			err = -EINVAL;
>> +			goto out;
>> +		}
>> +
>>  		if (attrs[XFRMA_REPLAY_VAL]) {
>>  			struct xfrm_replay_state *replay;
>>
>> @@ -454,6 +476,30 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
>>  			}
>>
>>  		}
>> +
>> +		if (attrs[XFRMA_IPTFS_DONT_FRAG]) {
>> +			NL_SET_ERR_MSG(extack, "Don't fragment should not be set for input SA");
>> +			err = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		if (attrs[XFRMA_IPTFS_INIT_DELAY]) {
>> +			NL_SET_ERR_MSG(extack, "Initial delay should not be set for input SA");
>> +			err = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		if (attrs[XFRMA_IPTFS_MAX_QSIZE]) {
>> +			NL_SET_ERR_MSG(extack, "Max queue size should not be set for input SA");
>> +			err = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		if (attrs[XFRMA_IPTFS_PKT_SIZE]) {
>> +			NL_SET_ERR_MSG(extack, "Packet size should not be set for input SA");
>> +			err = -EINVAL;
>> +			goto out;
>> +		}
>>  	}
>>
>>  out:
>> @@ -3177,6 +3223,12 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
>>  	[XFRMA_MTIMER_THRESH]   = { .type = NLA_U32 },
>>  	[XFRMA_SA_DIR]          = NLA_POLICY_RANGE(NLA_U8, XFRM_SA_DIR_IN, XFRM_SA_DIR_OUT),
>>  	[XFRMA_NAT_KEEPALIVE_INTERVAL] = { .type = NLA_U32 },
>> +	[XFRMA_IPTFS_DROP_TIME]		= { .type = NLA_U32 },
>> +	[XFRMA_IPTFS_REORDER_WINDOW]	= { .type = NLA_U16 },
>
> The corresponding sysctl is a u32, should this be NLA_U32?

While it's not unbelievable that one might want to handle more than 255 out-of-order packets, it is unbelievable that one would try and handle more than 65535. :)

So the change I'll make is to switch the sysctl handler to proc_douintvec_minmax with a max value to 65535 -- the sysctl functions work with "uint" and "ulong" there doesn't appear to be any support for "ushort"/"u16"s.

Thanks,
Chris.

>
>> +	[XFRMA_IPTFS_DONT_FRAG]		= { .type = NLA_FLAG },
>> +	[XFRMA_IPTFS_INIT_DELAY]	= { .type = NLA_U32 },
>> +	[XFRMA_IPTFS_MAX_QSIZE]		= { .type = NLA_U32 },
>> +	[XFRMA_IPTFS_PKT_SIZE]	= { .type = NLA_U32 },
>>  };
>>  EXPORT_SYMBOL_GPL(xfrma_policy);
>
> --
> Sabrina


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]