[2/7] security: modify PDCP xform to support SDAP
diff mbox series

Message ID 20200903160652.31654-3-akhil.goyal@nxp.com
State Superseded, archived
Delegated to: akhil goyal
Headers show
Series
  • support PDCP-SDAP for dpaa2_sec
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Akhil Goyal Sept. 3, 2020, 4:06 p.m. UTC 
From: Akhil Goyal <akhil.goyal@nxp.com>

The SDAP is a protocol in the LTE stack on top of PDCP for
QOS. A particular PDCP session may or may not have
SDAP enabled. But if it is enabled, SDAP header should be
authenticated but not encrypted if both confidentiality and
integrity is enabled. Hence, the driver should be intimated
from the xform so that it skip the SDAP header while encryption.

A new field is added in the PDCP xform to specify SDAP is enabled.
The overall size of the xform is not changed, as hfn_ovrd is just
a flag and does not need uint32. Hence, it is converted to uint8_t
and a 16 bit reserved field is added for future.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
---
 doc/guides/prog_guide/rte_security.rst | 19 ++++++++++++++++++-
 lib/librte_security/rte_security.h     | 12 ++++++++++--
 2 files changed, 28 insertions(+), 3 deletions(-)

Comments

Coyle, David Oct. 5, 2020, 6:04 p.m. UTC | #1 
Hi Akhil

> -----Original Message-----
> From: akhil.goyal@nxp.com <akhil.goyal@nxp.com>

<snip>

> diff --git a/doc/guides/prog_guide/rte_security.rst
> b/doc/guides/prog_guide/rte_security.rst
> index 127da2e4f..ab535d1cd 100644
> --- a/doc/guides/prog_guide/rte_security.rst
> +++ b/doc/guides/prog_guide/rte_security.rst
> @@ -1,5 +1,5 @@

<snip>

> @@ -693,6 +693,23 @@ PDCP related configuration parameters are defined
> in ``rte_security_pdcp_xform``
>          uint32_t hfn;
>          /** HFN Threshold for key renegotiation */
>          uint32_t hfn_threshold;
> +        /** HFN can be given as a per packet value also.
> +         * As we do not have IV in case of PDCP, and HFN is
> +         * used to generate IV. IV field can be used to get the
> +         * per packet HFN while enq/deq.
> +         * If hfn_ovrd field is set, user is expected to set the
> +         * per packet HFN in place of IV. PMDs will extract the HFN
> +         * and perform operations accordingly.
> +         */
> +         uint8_t hfn_ovrd;
> +         /** In case of 5G NR, a new protocol(SDAP) header may be set
> +          * inside PDCP payload which should be authenticated but not
> +          * encrypted. Hence, driver should be notified if SDAP is
> +          * enabled or not, so that SDAP header is not encrypted.
> +          */
> +         uint8_t sdap_enabled;
> +         /** Reserved for future */
> +         uint16_t reserved;
>      };

[DC] Should we consider removing the API code out of the security documentation?
It's a direct copy of the API code itself, and just means 2 files need to be updated for every API change.
And as with 'hfn_ovrd', sometimes it's forgotten.
From maintainability point of view, it might be better just remove it.

> 
>  DOCSIS related configuration parameters are defined in
> ``rte_security_docsis_xform`` diff --git a/lib/librte_security/rte_security.h
> b/lib/librte_security/rte_security.h
> index 16839e539..48b377b20 100644
> --- a/lib/librte_security/rte_security.h
> +++ b/lib/librte_security/rte_security.h
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright 2017,2019 NXP
> + * Copyright 2017,2019-2020 NXP
>   * Copyright(c) 2017-2020 Intel Corporation.
>   */
> 
> @@ -290,7 +290,15 @@ struct rte_security_pdcp_xform {
>  	 * per packet HFN in place of IV. PMDs will extract the HFN
>  	 * and perform operations accordingly.
>  	 */
> -	uint32_t hfn_ovrd;
> +	uint8_t hfn_ovrd;
> +	/** In case of 5G NR, a new protocol(SDAP) header may be set

[DC] Very minor thing... add space between 'protocol' and '(SDAP)' in the comment block.

And same comment for the documentation if you choose to keep the API code blocks there too.

> +	 * inside PDCP payload which should be authenticated but not
> +	 * encrypted. Hence, driver should be notified if SDAP is
> +	 * enabled or not, so that SDAP header is not encrypted.
> +	 */
> +	uint8_t sdap_enabled;
> +	/** Reserved for future */
> +	uint16_t reserved;
>  };
> 
>  /** DOCSIS direction */
> --
> 2.17.1
Akhil Goyal Oct. 8, 2020, 9:01 a.m. UTC | #2 
Hi David,
> Hi Akhil
> 
> 
> > @@ -693,6 +693,23 @@ PDCP related configuration parameters are defined
> > in ``rte_security_pdcp_xform``
> >          uint32_t hfn;
> >          /** HFN Threshold for key renegotiation */
> >          uint32_t hfn_threshold;
> > +        /** HFN can be given as a per packet value also.
> > +         * As we do not have IV in case of PDCP, and HFN is
> > +         * used to generate IV. IV field can be used to get the
> > +         * per packet HFN while enq/deq.
> > +         * If hfn_ovrd field is set, user is expected to set the
> > +         * per packet HFN in place of IV. PMDs will extract the HFN
> > +         * and perform operations accordingly.
> > +         */
> > +         uint8_t hfn_ovrd;
> > +         /** In case of 5G NR, a new protocol(SDAP) header may be set
> > +          * inside PDCP payload which should be authenticated but not
> > +          * encrypted. Hence, driver should be notified if SDAP is
> > +          * enabled or not, so that SDAP header is not encrypted.
> > +          */
> > +         uint8_t sdap_enabled;
> > +         /** Reserved for future */
> > +         uint16_t reserved;
> >      };
> 
> [DC] Should we consider removing the API code out of the security
> documentation?
> It's a direct copy of the API code itself, and just means 2 files need to be updated
> for every API change.
> And as with 'hfn_ovrd', sometimes it's forgotten.
> From maintainability point of view, it might be better just remove it.

Yes we can remove it. I will remove it in a separate patch.


> 
> >
> >  DOCSIS related configuration parameters are defined in
> > ``rte_security_docsis_xform`` diff --git a/lib/librte_security/rte_security.h
> > b/lib/librte_security/rte_security.h
> > index 16839e539..48b377b20 100644
> > --- a/lib/librte_security/rte_security.h
> > +++ b/lib/librte_security/rte_security.h
> > @@ -1,5 +1,5 @@
> >  /* SPDX-License-Identifier: BSD-3-Clause
> > - * Copyright 2017,2019 NXP
> > + * Copyright 2017,2019-2020 NXP
> >   * Copyright(c) 2017-2020 Intel Corporation.
> >   */
> >
> > @@ -290,7 +290,15 @@ struct rte_security_pdcp_xform {
> >  	 * per packet HFN in place of IV. PMDs will extract the HFN
> >  	 * and perform operations accordingly.
> >  	 */
> > -	uint32_t hfn_ovrd;
> > +	uint8_t hfn_ovrd;
> > +	/** In case of 5G NR, a new protocol(SDAP) header may be set
> 
> [DC] Very minor thing... add space between 'protocol' and '(SDAP)' in the
> comment block.

Sure. Will add it.
> 
Adding Techboard for request for approval of the change in the xform structure to
Add sdap support.


> And same comment for the documentation if you choose to keep the API code
> blocks there too.
> 
> > +	 * inside PDCP payload which should be authenticated but not
> > +	 * encrypted. Hence, driver should be notified if SDAP is
> > +	 * enabled or not, so that SDAP header is not encrypted.
> > +	 */
> > +	uint8_t sdap_enabled;
> > +	/** Reserved for future */
> > +	uint16_t reserved;
> >  };
> >
> >  /** DOCSIS direction */
> > --
> > 2.17.1

Patch
diff mbox series 

diff --git a/doc/guides/prog_guide/rte_security.rst b/doc/guides/prog_guide/rte_security.rst
index 127da2e4f..ab535d1cd 100644
--- a/doc/guides/prog_guide/rte_security.rst
+++ b/doc/guides/prog_guide/rte_security.rst
@@ -1,5 +1,5 @@ 
 ..  SPDX-License-Identifier: BSD-3-Clause
-    Copyright 2017 NXP
+    Copyright 2017,2020 NXP
 
 
 
@@ -693,6 +693,23 @@  PDCP related configuration parameters are defined in ``rte_security_pdcp_xform``
         uint32_t hfn;
         /** HFN Threshold for key renegotiation */
         uint32_t hfn_threshold;
+        /** HFN can be given as a per packet value also.
+         * As we do not have IV in case of PDCP, and HFN is
+         * used to generate IV. IV field can be used to get the
+         * per packet HFN while enq/deq.
+         * If hfn_ovrd field is set, user is expected to set the
+         * per packet HFN in place of IV. PMDs will extract the HFN
+         * and perform operations accordingly.
+         */
+         uint8_t hfn_ovrd;
+         /** In case of 5G NR, a new protocol(SDAP) header may be set
+          * inside PDCP payload which should be authenticated but not
+          * encrypted. Hence, driver should be notified if SDAP is
+          * enabled or not, so that SDAP header is not encrypted.
+          */
+         uint8_t sdap_enabled;
+         /** Reserved for future */
+         uint16_t reserved;
     };
 
 DOCSIS related configuration parameters are defined in ``rte_security_docsis_xform``
diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
index 16839e539..48b377b20 100644
--- a/lib/librte_security/rte_security.h
+++ b/lib/librte_security/rte_security.h
@@ -1,5 +1,5 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause
- * Copyright 2017,2019 NXP
+ * Copyright 2017,2019-2020 NXP
  * Copyright(c) 2017-2020 Intel Corporation.
  */
 
@@ -290,7 +290,15 @@  struct rte_security_pdcp_xform {
 	 * per packet HFN in place of IV. PMDs will extract the HFN
 	 * and perform operations accordingly.
 	 */
-	uint32_t hfn_ovrd;
+	uint8_t hfn_ovrd;
+	/** In case of 5G NR, a new protocol(SDAP) header may be set
+	 * inside PDCP payload which should be authenticated but not
+	 * encrypted. Hence, driver should be notified if SDAP is
+	 * enabled or not, so that SDAP header is not encrypted.
+	 */
+	uint8_t sdap_enabled;
+	/** Reserved for future */
+	uint16_t reserved;
 };
 
 /** DOCSIS direction */