diff mbox

[net-next,2/3] xfrm: clamp down spi range for IPComp when allocating spi

Message ID 52A562DF.4090302@windriver.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

fan.du Dec. 9, 2013, 6:27 a.m. UTC 
On 2013年12月06日 19:42, Steffen Klassert wrote:
> On Thu, Nov 28, 2013 at 10:52:40AM +0800, Fan Du wrote:
>> otherwise xfrm state can not be found properly by peers.
>>
>> Signed-off-by: Fan Du<fan.du@windriver.com>
>> ---
>>   net/xfrm/xfrm_state.c |   13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
>> index 68c2f35..a6716d7 100644
>> --- a/net/xfrm/xfrm_state.c
>> +++ b/net/xfrm/xfrm_state.c
>> @@ -1506,6 +1506,19 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
>>   	__be32 maxspi = htonl(high);
>>   	u32 mark = x->mark.v&  x->mark.m;
>>
>> +	/* Compression Parameter Index(CPI) is 16bits wide
>> +	 * An 32 bits spi value will hash xfrm_state into wrong hash slot.
>> +	 * When the upper 16bits of spi values is used as CPI for the peer
>> +	 * to look up xfrm state, it would generate XfrmOutNoStates error,
>> +	 * as apparently we are looking for the wrong hash slot.
>> +	 *
>> +	 * So clamp down the spi range into only 16bits valid wide.
>> +	 */
>> +	if (x->id.proto == IPPROTO_COMP) {
>> +		minspi = htonl(0xc00);
>> +		maxspi = htonl(0xff00);
>> +	}
>
> This does not make sense. The spi is chosen based on minspi only
> if minspi is equal to maxspi. This will be never the case for
> IPPROTO_COMP with your patch.
>
> Also, the spi range is user defined, we should respect the
> users configuration if the range is valid.

Ok, then, speaking of respect user defined range, how about below informal
patch which only check the validity of the range? My original thoughts is CPI
is only 16bits wide, kernel itself can keep the CPI's validity. btw, v2 will
also fix patch1/3 align issue.

Comments

Steffen Klassert Dec. 9, 2013, 8:57 a.m. UTC | #1 
On Mon, Dec 09, 2013 at 02:27:43PM +0800, Fan Du wrote:
> On 2013年12月06日 19:42, Steffen Klassert wrote:
> >
> >Also, the spi range is user defined, we should respect the
> >users configuration if the range is valid.
> 
> Ok, then, speaking of respect user defined range, how about below informal
> patch which only check the validity of the range? My original thoughts is CPI
> is only 16bits wide, kernel itself can keep the CPI's validity. btw, v2 will
> also fix patch1/3 align issue.
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 6a9c402..2c6fb99 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1507,6 +1507,9 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
> 
>         err = -ENOENT;
> 
> +       if ((x->id.proto == IPPROTO_COMP) && (high > 0xFFFF))
> +               goto unlock;
> +

This check is already done in verify_userspi_info() if xfrm_alloc_spi()
is called from xfrm_alloc_userspi().

Instead of doing this check here again, we should implement an equivalent
to verify_userspi_info() for pfkey. Then we are sure to have a valid range
in any case.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
fan.du Dec. 9, 2013, 9:13 a.m. UTC | #2 
On 2013年12月09日 16:57, Steffen Klassert wrote:
> On Mon, Dec 09, 2013 at 02:27:43PM +0800, Fan Du wrote:
>> On 2013年12月06日 19:42, Steffen Klassert wrote:
>>>
>>> Also, the spi range is user defined, we should respect the
>>> users configuration if the range is valid.
>>
>> Ok, then, speaking of respect user defined range, how about below informal
>> patch which only check the validity of the range? My original thoughts is CPI
>> is only 16bits wide, kernel itself can keep the CPI's validity. btw, v2 will
>> also fix patch1/3 align issue.
>>
>> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
>> index 6a9c402..2c6fb99 100644
>> --- a/net/xfrm/xfrm_state.c
>> +++ b/net/xfrm/xfrm_state.c
>> @@ -1507,6 +1507,9 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
>>
>>          err = -ENOENT;
>>
>> +       if ((x->id.proto == IPPROTO_COMP)&&  (high>  0xFFFF))
>> +               goto unlock;
>> +
>
> This check is already done in verify_userspi_info() if xfrm_alloc_spi()
> is called from xfrm_alloc_userspi().
>
> Instead of doing this check here again, we should implement an equivalent
> to verify_userspi_info() for pfkey. Then we are sure to have a valid range
> in any case.
>

How about export an common function in xfrm_state.c to check this corner case?
This could be shared by both netlink and pfkey interface, and verify_userspi_info
simplified also?

int check_ipcomp_spirange(u8 proto, u32 high)
{
	if ((proto == IPPROTO_COMP) && (high > 0xFFFF))
		return -EINVAL;
	else return 0;
}
EXPORT_SYMBOL(check_ipcomp_spirange);
Steffen Klassert Dec. 9, 2013, 9:51 a.m. UTC | #3 
On Mon, Dec 09, 2013 at 05:13:52PM +0800, Fan Du wrote:
> 
> 
> On 2013年12月09日 16:57, Steffen Klassert wrote:
> >
> >Instead of doing this check here again, we should implement an equivalent
> >to verify_userspi_info() for pfkey. Then we are sure to have a valid range
> >in any case.
> >
> 
> How about export an common function in xfrm_state.c to check this corner case?
> This could be shared by both netlink and pfkey interface, and verify_userspi_info
> simplified also?
> 
> int check_ipcomp_spirange(u8 proto, u32 high)
> {
> 	if ((proto == IPPROTO_COMP) && (high > 0xFFFF))
> 		return -EINVAL;
> 	else return 0;
> }
> EXPORT_SYMBOL(check_ipcomp_spirange);

I don't think that we should export such a function,
it is not sufficient.

The netlink interface is ok, it does verify_userspi_info(),
and the pfkey interface need all the checks done in
verify_userspi_info() too. In particular the check if
the minimum spi value is not bigger than the maximum.

So we could either make verify_userspi_info() shared,
or implement a own function for pfkey.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
fan.du Dec. 9, 2013, 9:58 a.m. UTC | #4 
On 2013年12月09日 17:51, Steffen Klassert wrote:
> On Mon, Dec 09, 2013 at 05:13:52PM +0800, Fan Du wrote:
>>
>>
>> On 2013年12月09日 16:57, Steffen Klassert wrote:
>>>
>>> Instead of doing this check here again, we should implement an equivalent
>>> to verify_userspi_info() for pfkey. Then we are sure to have a valid range
>>> in any case.
>>>
>>
>> How about export an common function in xfrm_state.c to check this corner case?
>> This could be shared by both netlink and pfkey interface, and verify_userspi_info
>> simplified also?
>>
>> int check_ipcomp_spirange(u8 proto, u32 high)
>> {
>> 	if ((proto == IPPROTO_COMP)&&  (high>  0xFFFF))
>> 		return -EINVAL;
>> 	else return 0;
>> }
>> EXPORT_SYMBOL(check_ipcomp_spirange);
>
> I don't think that we should export such a function,
> it is not sufficient.
>
> The netlink interface is ok, it does verify_userspi_info(),
> and the pfkey interface need all the checks done in
> verify_userspi_info() too. In particular the check if
> the minimum spi value is not bigger than the maximum.
>
> So we could either make verify_userspi_info() shared,

Ok, I will try to export verify_userspi_info then.
Is there any comments on patch3/3 before I make v2?
diff mbox

Patch

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 6a9c402..2c6fb99 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1507,6 +1507,9 @@  int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)

         err = -ENOENT;

+       if ((x->id.proto == IPPROTO_COMP) && (high > 0xFFFF))
+               goto unlock;
+
         if (minspi == maxspi) {
                 x0 = xfrm_state_lookup(net, mark, &x->id.daddr, minspi, x->id.proto, x->props.family);
                 if (x0) {