netdev - RE: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op

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]

Message-ID: <HE1PR05MB3210A6E5B89A1E8EEBE0519CC43F0@HE1PR05MB3210.eurprd05.prod.outlook.com>
Date:   Sun, 3 Dec 2017 13:59:37 +0000
From:   Yossi Kuperman <yossiku@...lanox.com>
To:     Boris Pismenny <borisp@...lanox.com>,
        Steffen Klassert <steffen.klassert@...unet.com>
CC:     Aviv Heller <avivh@...lanox.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Yevgeny Kliteynik <kliteyn@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op



> -----Original Message-----
> From: Boris Pismenny
> Sent: Sunday, December 3, 2017 1:29 PM
> To: Yossi Kuperman <yossiku@...lanox.com>; Steffen Klassert
> <steffen.klassert@...unet.com>
> Cc: Aviv Heller <avivh@...lanox.com>; Herbert Xu
> <herbert@...dor.apana.org.au>; Yevgeny Kliteynik <kliteyn@...lanox.com>;
> netdev@...r.kernel.org
> Subject: RE: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op
> 
> 
> >
> > >> On 1 Dec 2017, at 9:09, Steffen Klassert
> > >> <steffen.klassert@...unet.com>
> > wrote:
> > >>
> > >> On Tue, Nov 28, 2017 at 07:55:41PM +0200, avivh@...lanox.com wrote:
> > >> From: Aviv Heller <avivh@...lanox.com>
> > >>
> > >> Adding the state to the offload device prior to replay init in
> > >> xfrm_state_construct() will result in NULL dereference if a
> > >> matching ESP packet is received in between.
> > >>
> > >> In order to inhibit driver offload logic from processing the
> > >> state's packets prior to the xfrm_state object being completely
> > >> initialized and added to the SADBs, a new activate() operation was
> > >> added to inform the driver the aforementioned conditions have been met.
> > >
> > > We discussed this already some time ago, and I still think that we
> > > should fix this by setting XFRM_STATE_VALID only after the state is
> > > fully initialized.
> >
> > An upcoming patch will refactor the if statement (encap_type < 0) in
> > xfrm_input, in order to support crypto offload with GRO disabled.
> > Currently it doesn’t work. This entails yet another check for the validity of the
> state.
> > Resulting in total of 3 copies: 1) for normal traffic, 2) GRO and 3) crypto
> offload.
> >
> > Anyway, IMO it is not right that we (the driver) allow an incoming
> > packet to be delivered while the SA is not yet ready. Rather than
> > checking for an invalid input I prefer to make sure that such a case won’t
> happen in the first place.
> >
> > To complete the picture, there is another patch to the driver which
> > simply drop incoming packets that underwent successful decryption and
> > haven’t been activated yet. Active state merely means that the SA is
> > present in the driver’s hash table.
> >
> > We can make a separate patch to set the state to valid once it is
> > fully initialized, it make sense on its own.
> >
> > What do you think?
> >
> 
> I would prefer we do not add additional checks in the data-path.
> IMO, the SA provided to the driver should be fully initialized.
> Do not insert it to HW before it is activated.

Nope, we are not adding anything new to the data-path. Currently,
xdo_dev_state_add configures the hardware with the new SA and
then stores a reference to it in a hash table. We suggest here
to split it into two parts: 1) configure the hardware 2) add to the 
hash table (activate). No change in the data-path whatsoever.