netdev - Re: [devel-ipsec] [PATCH ipsec-next v6] xfrm: Add Direction to the SA in or out

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: Sun, 7 Apr 2024 10:23:21 +0200
From: Antony Antony <antony@...nome.org>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Antony Antony <antony.antony@...unet.com>,
	Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org,
	devel@...ux-ipsec.org, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [devel-ipsec] [PATCH ipsec-next v6] xfrm: Add Direction to the
 SA in or out

Hi Sabrina,

On Fri, Apr 05, 2024 at 11:56:00PM +0200, Sabrina Dubroca via Devel wrote:
> Hi Antony,
> 
> 2024-04-05, 14:40:07 +0200, Antony Antony wrote:
> > This patch introduces the 'dir' attribute, 'in' or 'out', to the
> > xfrm_state, SA, enhancing usability by delineating the scope of values
> > based on direction. An input SA will now exclusively encompass values
> > pertinent to input, effectively segregating them from output-related
> > values. 
> 
> But this patch isn't doing that for existing properties (I'm thinking
> of replay window, not sure if any others are relevant [1]). Why not?

Thank you for raising this point.  I thought that introducing a patch for 
the replay window check could stir more controversy, which might delay the 
acceptance of the essential 'dir' feature. My primary goal at this stage is 
to get this basic feature  in and to convince Chris to integrate the "dir" 
attribute into IP-TFS. This patch has partly contributed to the delays in 
IP-TFS's development.

Given your input, I'm curious about the specific conditions you have in 
mind. See the attached patch.

For non-ESN scenarios, the outbound SA should have a replay window set to 0?  
And for ESN 1?

non-ESN
ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2  \
mode tunnel dir out aead 'rfc4106(gcm(aes))' \
0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10
Error: Replay-window too big > 0 for OUT SA.

The current impelementation does not replay window 0 with ESN.  Even though 
disabling replay window with ESN is a desired feature.

ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2 mode 
tunnel dir out flag esn  aead  'rfc4106(gcm(aes))'  \
0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10
Error: ESN replay-window too big > 1 for OUT SA.ww

I wonder would the attached patch get accepted quickly.

> 
> [1] that should include values passed via xfrm_usersa_info too,
>     not just XFRMA_* attributes
> 
> Adding these checks should be safe (wrt breakage of API): Old software
> would not be passing XFRMA_SA_DIR, so adding checks when it is provided
> would not break anything there. Only new software using the attribute
> would benefit from having directed SAs and restriction on which attributes
> can be used (and that's fine).
> 
> Right now the new attribute is 100% duplicate of the existing offload
> direction, so I don't see much point.

IP-TFS and Chris alreay expressed it. It started with this e-mail.
https://lore.kernel.org/netdev/ZV0BSBzNh3UIqueZ@Antony2201.local
I am trying to convince Chris to use "dir". Without direction I found IP-TFS 
confusing without direction.

> > This change aims to streamline the configuration process and
> > improve the overall clarity of SA attributes.
> > 
> > This feature sets the groundwork for future patches, including
> > the upcoming IP-TFS patch.
> > 
> > Currently, dir is only allowed when HW OFFLOAD is set.
> > 
> > ---
> 
> BTW, everything after this '---' will get cut, including your sign-off.

thanks for spotting it. I will send new version.

> 
> > v5->v6:
> >  - XFRMA_SA_DIR only allowed with HW OFFLOAD
> > 
> > v4->v5:
> >  - add details to commit message
> > 
> > v3->v4:
> >  - improve HW OFFLOAD DIR check check other direction
> > 
> > v2->v3:
> >  - delete redundant XFRM_SA_DIR_USE
> >  - use u8 for "dir"
> >  - fix HW OFFLOAD DIR check
> > 
> > v1->v2:
> >  - use .strict_start_type in struct nla_policy xfrma_policy
> >  - delete redundant XFRM_SA_DIR_MAX enum
> > ---
> > 
> > Signed-off-by: Antony Antony <antony.antony@...unet.com>
> > Reviewed-by: Leon Romanovsky <leonro@...dia.com>
> 
> nit: If I'm making non-trivial changes to the contents of the patch, I
> typically drop the review (and test) tags I got on previous versions,
> since they may no longer apply.

I agree.

-antony

View attachment "0001-xfrm-Add-Direction-to-the-SA-in-or-out.patch" of type "text/plain" (9646 bytes)