Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[Kazunori MIYAZAWA <kazunori@miyazawa.org>]

Hello David,

Thank you for your tracing the bug.

I understood the issue.
Mmm, if we can not use ut->family, can we use
ut->id.family instead?

Or is it also uninitialized?

David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Sun, 03 Dec 2006 17:58:47 -0800 (PST)
> 
>> Kazunori, a bug from the changes I did apply:
>>
>> [  761.318131] kernel BUG at net/key/af_key.c:1925!
> 
> I found the problem, it's because of the xfrm_user.c change where
> we clobber the xp->family value with ut->family.
> 
> But we never ever verified nor cared about the ut->family value
> because previously templates were all of the same family as the
> policy, so there was no reason to check or verify the ut->family
> value.
> 
> So applications left it at zero.
> 
> This means you did no testing of the xfrm_user.c netlink changes.
> 
> We can "fix" this with some patch like the below, changing
> ut->family to xp->family if it is left at zero, but it is clear
> that since we've never checked this value it can be any value.
> What if it is left uninitialized by the application and the
> garbage value just happens to be AF_INET6 or something?
> 
> To me this means that ut->family is %100 unreliable and we cannot
> count on it in any way, and we'll need to specify the family in
> some other way.
> 
> BTW, is it OK to clobber the entire policy's xp->family with the
> top-most ut->family?  Shouldn't the application set the policy's
> family to AF_INET6 if it wants the outer-most template to be
> AF_INET6?
> 
> How can changing the policy family be valid?  Doing this means we'll
> interpret the selectors of the policy differently from what the
> application originally provided.  This setting of xp->family therefore
> cannot make any sense, it must remain at whatever value the
> application gave us.
> 
> I really regret applying these patches, they are in a very bad shape
> and poorly designed.  Now every openswan user will get an OOPS
> when they try to bring up their tunnels with Linus's current tree.
> 
> I think instead of the patch below, I'm going to revert at least
> the xfrm_user part of these changes. :-/
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 6f97665..76c7cdc 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -857,6 +857,11 @@ static void copy_templates(struct xfrm_p
>  {
>  	int i;
>  
> +
> +	/* Backward compat for older applications.  */
> +	if (ut->family == 0)
> +		ut->family = xp->family;
> +
>  	xp->xfrm_nr = nr;
>  	xp->family = ut->family;
>  	for (i = 0; i < nr; i++, ut++) {
> 

--
Kazunori Miyazawa