netdev - RE: [RFC net-next 00/15] add basic PSP encryption for TCP connections

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, 20 Jun 2024 21:32:14 +0000
From: "Singhai, Anjali" <anjali.singhai@...el.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC: Paolo Abeni <pabeni@...hat.com>, Boris Pismenny <borisp@...dia.com>,
	"gal@...dia.com" <gal@...dia.com>, "cratiu@...dia.com" <cratiu@...dia.com>,
	"rrameshbabu@...dia.com" <rrameshbabu@...dia.com>,
	"steffen.klassert@...unet.com" <steffen.klassert@...unet.com>,
	"tariqt@...dia.com" <tariqt@...dia.com>, Jakub Kicinski <kuba@...nel.org>,
	"Samudrala, Sridhar" <sridhar.samudrala@...el.com>, "Acharya, Arun Kumar"
	<arun.kumar.acharya@...el.com>
Subject: RE: [RFC net-next 00/15] add basic PSP encryption for TCP connections


> > 1. Why do we need  ndo_op set_config() at device level which is setting only one version, instead the description above the psp_dev struct which had a mask for enabled versions at a  device level is better and device lets the stack know at psp_dev create time what all versions it is capable of.  Later on, version is negotiated with the peer and set per session.
> > Even the Mellanox driver does not implement this set_config ndo_op. 
>  >
Can you or Kuba comment on this?

> > 2. Where is the association_index/handle returned to the stack to be used with the packet on TX by the driver and device? ( if an SADB is in use on Tx side in the device), what we understand from Mellanox driver is, its not doing an SADB in TX in HW, but passing the key directly into the Tx descriptor? Is that right, but other devices may not support this and will have an SADB on TX and this allowed as per PSP protocol. Of course on RX there is no SADB for any device.
> > In our device we have 2 options, 
> >             1. Using SADB on TX and just passing SA_Index in the descriptor (trade off between performance and memory. 
> >             As  passing key in descriptor makes for a much larger TX descriptor which will have perf penalty.)
> >            2. Passing key in the descriptor.
> >             For us we need both these options, so please allow for enhancements.
> >
Can you or Kuba comment on this? This is critical, also in the fast path, skb needs to carry the SA_index/handle (like the tls case) instead of the key or both so that either method can be used by the device driver/device.

> > 3. About the PSP and UDP header addition, why is the driver doing it? I guess it's because the SW equivalent for PSP support in the kernel does not exist and just an offload for the device. Again in this case the assumption is either the driver does it or the device will do it.
> > Hope that is irrelevant for the stack. In our case most likely it will be the device doing it.
> > 

> This does not adhere to the spec:

> "An option must be provided that enables upper-level software to send packets that are pre-formatted to include the headers required for PSP encapsulation. In this case, the NIC will modify the contents of the headers appropriately, apply encryption/authentication, and add the PSP trailer to the packet."

> https://raw.githubusercontent.com/google/psp/main/doc/PSP_Arch_Spec.pdf

Yes I guess this is just transport mode and not tunnel mode in which the device adds the header. So agreed, it should be upper-level software that should add this, but again not the driver as this is protocol specific and not device specific and all devices using a  common code would be the right thing.

> > 4. Why is the driver adding the PSP trailer? Hoping this is between the driver and the device, in our case it's the device that will add the trailer.

This for sure is by device or driver, ideally the device. Please comment.

A few more opens that we noticed later

1. Key rotation should be triggered from the device as a master key in the device can be shared in a virtualized environment by many interfaces which would mean only the device can decide based on the following when to trigger the key rotation 
	1. Time out cannot be independent for each IKE but at a device level configuration.
	2.  SPI roll over, the SPI domain is again shared with multiple Interfaces that share the master key and only the device can trigger the rotation when this happens.

Apart from this, in a virtualized environment, a trigger from top (IKE down to device) to rotate the master key can cause unnecessary side effects to other interfaces that can be considered malicious.

Anjali