netdev - Re: [RFC ipsec-next v2 6/8] iptfs: xfrm: Add mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 01 Feb 2024 10:34:49 -0500
From: Christian Hopps <chopps@...pps.org>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Christian Hopps <chopps@...pps.org>, devel@...ux-ipsec.org, Steffen
 Klassert <steffen.klassert@...unet.com>, netdev@...r.kernel.org, Christian
 Hopps <chopps@...n.net>
Subject: Re: [RFC ipsec-next v2 6/8] iptfs: xfrm: Add mode_cbs module
 functionality


Sabrina Dubroca <sd@...asysnail.net> writes:

> 2023-11-12, 22:52:17 -0500, Christian Hopps wrote:
>> From: Christian Hopps <chopps@...n.net>
>>
>> Add a set of callbacks xfrm_mode_cbs to xfrm_state. These callbacks
>> enable the addition of new xfrm modes, such as IP-TFS to be defined
>> in modules.
>
> Not a big fan of bringing back modes in modules :(
> Florian's work made the code a lot more readable.
>
>> diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
>> index 662c83beb345..4390c111410d 100644
>> --- a/net/xfrm/xfrm_output.c
>> +++ b/net/xfrm/xfrm_output.c
>> @@ -280,7 +280,9 @@ static int xfrm4_tunnel_encap_add(struct xfrm_state *x, struct sk_buff *skb)
>>  	skb_set_inner_network_header(skb, skb_network_offset(skb));
>>  	skb_set_inner_transport_header(skb, skb_transport_offset(skb));
>>
>> -	skb_set_network_header(skb, -x->props.header_len);
>> +	/* backup to add space for the outer encap */
>> +	skb_set_network_header(skb,
>> +			       -x->props.header_len + x->props.enc_hdr_len);
>
> Since this only gets called for XFRM_MODE_TUNNEL, and only iptfs sets
> enc_hdr_len, do we need this change? (and same for xfrm6_tunnel_encap_add)

You're right, removed. This particular code actually predated the callbacks.

>>  	skb->mac_header = skb->network_header +
>>  			  offsetof(struct iphdr, protocol);
>>  	skb->transport_header = skb->network_header + sizeof(*top_iph);
>> @@ -325,7 +327,8 @@ static int xfrm6_tunnel_encap_add(struct xfrm_state *x, struct sk_buff *skb)
>>  	skb_set_inner_network_header(skb, skb_network_offset(skb));
>>  	skb_set_inner_transport_header(skb, skb_transport_offset(skb));
>>
>> -	skb_set_network_header(skb, -x->props.header_len);
>> +	skb_set_network_header(skb,
>> +			       -x->props.header_len + x->props.enc_hdr_len);
>>  	skb->mac_header = skb->network_header +
>>  			  offsetof(struct ipv6hdr, nexthdr);
>>  	skb->transport_header = skb->network_header + sizeof(*top_iph);
>> @@ -472,6 +475,8 @@ static int xfrm_outer_mode_output(struct xfrm_state *x, struct sk_buff *skb)
>>  		WARN_ON_ONCE(1);
>>  		break;
>>  	default:
>> +		if (x->mode_cbs->prepare_output)
>
> Can x->mode_cbs be NULL here? Every other use of mode_cbs does
>     if (x->mode_cbs && x->mode_cbs->FOO)
>
> (I think not at the moment since only IPTFS (and IN_TRIGGER) can reach
> this, but this inconsistency with the rest of the series struck me)

Still worth putting the guard in, fixed.

Thanks,
Chris.

Download attachment "signature.asc" of type "application/pgp-signature" (858 bytes)