For user-key encrypted bitstream loading use case, users can encrypt
FPGA configuration Images with their own key.While decrypting the
configuration Image the user needs to provide the same key.To support
this use case with the existing FPGA manager framework is not possible
because it doesn’t have a mechanism to get the required inputs from
the user. So this patch series adds the required changes to the FPGA
manager framework to support user-key encrypted bitstream image loading
use case.
Please take a look at the changes and let us know if any improvements
are required.
Nava kishore Manne (3):
dt-bindings: fpga: Add support for user-key encrypted bitstream
loading
drivers: fpga: Add user-key encrypted FPGA Image loading support
fpga: zynqmp: Add encrypted Bitstream loading support
.../devicetree/bindings/fpga/fpga-region.txt | 32 +++++++
drivers/fpga/fpga-mgr.c | 86 +++++++++++++++++--
drivers/fpga/of-fpga-region.c | 10 +++
drivers/fpga/zynqmp-fpga.c | 53 +++++++++++-
include/linux/firmware/xlnx-zynqmp.h | 2 +
include/linux/fpga/fpga-mgr.h | 8 ++
6 files changed, 181 insertions(+), 10 deletions(-)
--
2.25.1
Adds ‘encrypted-key-name’ property to support user-key encrypted
bitstream loading use case.
Signed-off-by: Nava kishore Manne <[email protected]>
---
.../devicetree/bindings/fpga/fpga-region.txt | 32 +++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
index 528df8a0e6d8..309334558b3f 100644
--- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
+++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
@@ -177,6 +177,9 @@ Optional properties:
it indicates that the FPGA has already been programmed with this image.
If this property is in an overlay targeting an FPGA region, it is a
request to program the FPGA with that image.
+- encrypted-key-name : should contain the name of an encrypted key file located
+ on the firmware search path. It will be used to decrypt the FPGA image
+ file with user-key.
- fpga-bridges : should contain a list of phandles to FPGA Bridges that must be
controlled during FPGA programming along with the parent FPGA bridge.
This property is optional if the FPGA Manager handles the bridges.
@@ -459,6 +462,35 @@ programming is the FPGA based bridge of fpga_region1.
};
};
+Device Tree Example: Configure/Reconfigure Encrypted Image With User Key
+========================================================================
+
+Users can encrypt FPGA configuration Images with their own key. While decrypting
+the configuration Image the user needs to provide the same key.
+"encrypted-key-name" Specifies the name of the FPGA image encrypted key file on
+the firmware search path. The search path is described in the firmware class
+documentation.
+
+/dts-v1/;
+/plugin/;
+
+&fpga_region0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ firmware-name = "soc_image2.rbf";
+ encrypted-key-name = "key.nky";
+
+ gpio@10040 {
+ compatible = "altr,pio-1.0";
+ reg = <0x10040 0x20>;
+ clocks = <0x2>;
+ altr,ngpio = <0x4>;
+ #gpio-cells = <0x2>;
+ gpio-controller;
+ };
+};
+
Constraints
===========
--
2.25.1
Adds support for both Device-key and user-key encrypted bitstream
loading to the Xilinx ZynqMP Soc.
Signed-off-by: Nava kishore Manne <[email protected]>
---
drivers/fpga/zynqmp-fpga.c | 53 ++++++++++++++++++++++++++--
include/linux/firmware/xlnx-zynqmp.h | 2 ++
2 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
index f3434e2c487b..8b0e4b8b5d99 100644
--- a/drivers/fpga/zynqmp-fpga.c
+++ b/drivers/fpga/zynqmp-fpga.c
@@ -15,16 +15,44 @@
/* Constant Definitions */
#define IXR_FPGA_DONE_MASK BIT(3)
+#define ENCRYPTED_KEY_LEN 64
+#define AES_MATCH_STR_LEN 5
+
/**
* struct zynqmp_fpga_priv - Private data structure
+ * @aes_key: Pointer Aes key buffer
* @dev: Device data structure
* @flags: flags which is used to identify the bitfile type
*/
struct zynqmp_fpga_priv {
+ const char *aes_key;
struct device *dev;
u32 flags;
};
+static int zynqmp_fpga_parse_aes_key(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
+ const char *buf, size_t size)
+{
+ struct zynqmp_fpga_priv *priv = mgr->priv;
+ const char *str = "Key 0";
+
+ for (int i = 0; i < size; i++) {
+ if (!strncmp(str, &buf[i], AES_MATCH_STR_LEN)) {
+ buf += AES_MATCH_STR_LEN + 1;
+ while (buf[i] == ' ')
+ i++;
+ if (size - i < ENCRYPTED_KEY_LEN)
+ return -EINVAL;
+ priv->aes_key = &buf[i];
+
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}
+
static int zynqmp_fpga_ops_write_init(struct fpga_manager *mgr,
struct fpga_image_info *info,
const char *buf, size_t size)
@@ -43,25 +71,43 @@ static int zynqmp_fpga_ops_write(struct fpga_manager *mgr,
struct zynqmp_fpga_priv *priv;
dma_addr_t dma_addr;
u32 eemi_flags = 0;
+ size_t dma_size;
char *kbuf;
int ret;
priv = mgr->priv;
- kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr, GFP_KERNEL);
+ if (priv->flags & FPGA_MGR_USRKEY_ENCRYPTED_BITSTREAM)
+ dma_size = size + ENCRYPTED_KEY_LEN;
+ else
+ dma_size = size;
+
+ kbuf = dma_alloc_coherent(priv->dev, dma_size, &dma_addr, GFP_KERNEL);
if (!kbuf)
return -ENOMEM;
memcpy(kbuf, buf, size);
+ if (priv->flags & FPGA_MGR_USRKEY_ENCRYPTED_BITSTREAM) {
+ eemi_flags |= XILINX_ZYNQMP_PM_FPGA_ENCRYPTION_USERKEY;
+ memcpy(kbuf + size, priv->aes_key, ENCRYPTED_KEY_LEN);
+ }
+
wmb(); /* ensure all writes are done before initiate FW call */
if (priv->flags & FPGA_MGR_PARTIAL_RECONFIG)
eemi_flags |= XILINX_ZYNQMP_PM_FPGA_PARTIAL;
- ret = zynqmp_pm_fpga_load(dma_addr, size, eemi_flags);
+ if (priv->flags & FPGA_MGR_ENCRYPTED_BITSTREAM)
+ eemi_flags |= XILINX_ZYNQMP_PM_FPGA_ENCRYPTION_DEVKEY;
+
+ if (priv->flags & FPGA_MGR_USRKEY_ENCRYPTED_BITSTREAM)
+ ret = zynqmp_pm_fpga_load(dma_addr, dma_addr + size,
+ eemi_flags);
+ else
+ ret = zynqmp_pm_fpga_load(dma_addr, size, eemi_flags);
- dma_free_coherent(priv->dev, size, kbuf, dma_addr);
+ dma_free_coherent(priv->dev, dma_size, kbuf, dma_addr);
return ret;
}
@@ -99,6 +145,7 @@ ATTRIBUTE_GROUPS(zynqmp_fpga);
static const struct fpga_manager_ops zynqmp_fpga_ops = {
.state = zynqmp_fpga_ops_state,
+ .parse_aes_key = zynqmp_fpga_parse_aes_key,
.write_init = zynqmp_fpga_ops_write_init,
.write = zynqmp_fpga_ops_write,
};
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index d1ea3898564c..e88f24870a77 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -83,6 +83,8 @@
*/
#define XILINX_ZYNQMP_PM_FPGA_FULL 0x0U
#define XILINX_ZYNQMP_PM_FPGA_PARTIAL BIT(0)
+#define XILINX_ZYNQMP_PM_FPGA_ENCRYPTION_USERKEY BIT(3)
+#define XILINX_ZYNQMP_PM_FPGA_ENCRYPTION_DEVKEY BIT(4)
/* FPGA Status Reg */
#define XILINX_ZYNQMP_PM_FPGA_CONFIG_STAT_OFFSET 7U
--
2.25.1
Adds parse_aes_key() callback to fpga_manager_ops, It will read the
AES key from firmware to support user-key encrypted bitstream loading.
Signed-off-by: Nava kishore Manne <[email protected]>
---
drivers/fpga/fpga-mgr.c | 86 ++++++++++++++++++++++++++++++++---
drivers/fpga/of-fpga-region.c | 10 ++++
include/linux/fpga/fpga-mgr.h | 8 ++++
3 files changed, 97 insertions(+), 7 deletions(-)
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 06651389c592..c8e20e8f4b6f 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -83,6 +83,15 @@ static inline int fpga_mgr_parse_header(struct fpga_manager *mgr,
return 0;
}
+static inline int fpga_mgr_parse_aes_key(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
+ const char *buf, size_t count)
+{
+ if (mgr->mops->parse_aes_key)
+ return mgr->mops->parse_aes_key(mgr, info, buf, count);
+ return 0;
+}
+
static inline int fpga_mgr_write_init(struct fpga_manager *mgr,
struct fpga_image_info *info,
const char *buf, size_t count)
@@ -559,6 +568,43 @@ static int fpga_mgr_firmware_load(struct fpga_manager *mgr,
return ret;
}
+/**
+ * fpga_mgr_get_aes_key - request aes key form the firmware class
+ * @mgr: fpga manager
+ * @info: fpga image specific information
+ * @image_name: name of image file on the aes key firmware search path
+ *
+ *
+ * Request an aes key image using the firmware class, then Step the low level
+ * fpga manager through the device-specific steps. Update the state before each
+ * step to provide info on what step failed if there is a failure.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int fpga_mgr_get_aes_key(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
+ const char *image_name,
+ const struct firmware *fw)
+{
+ struct device *dev = &mgr->dev;
+ int ret;
+
+ dev_info(dev, "Get Aes-key: %s to %s\n", image_name, mgr->name);
+
+ mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
+
+ ret = request_firmware(&fw, image_name, dev);
+ if (ret) {
+ mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
+ dev_err(dev, "Error requesting firmware %s\n", image_name);
+ return ret;
+ }
+
+ ret = fpga_mgr_parse_aes_key(mgr, info, fw->data, fw->size);
+
+ return ret;
+}
+
/**
* fpga_mgr_load - load FPGA from scatter/gather table, buffer, or firmware
* @mgr: fpga manager
@@ -571,15 +617,41 @@ static int fpga_mgr_firmware_load(struct fpga_manager *mgr,
*/
int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
{
+ const struct firmware *fw;
+ int ret;
+
info->header_size = mgr->mops->initial_header_size;
- if (info->sgt)
- return fpga_mgr_buf_load_sg(mgr, info, info->sgt);
- if (info->buf && info->count)
- return fpga_mgr_buf_load(mgr, info, info->buf, info->count);
- if (info->firmware_name)
- return fpga_mgr_firmware_load(mgr, info, info->firmware_name);
- return -EINVAL;
+ if (info->encrypted_key_name) {
+ ret = fpga_mgr_get_aes_key(mgr, info,
+ info->encrypted_key_name, fw);
+ if (ret)
+ return ret;
+
+ info->flags |= FPGA_MGR_USRKEY_ENCRYPTED_BITSTREAM;
+ }
+
+ if (info->sgt) {
+ ret = fpga_mgr_buf_load_sg(mgr, info, info->sgt);
+ if (ret)
+ goto free_fw;
+ } else if (info->buf && info->count) {
+ ret = fpga_mgr_buf_load(mgr, info, info->buf, info->count);
+ if (ret)
+ goto free_fw;
+ } else if (info->firmware_name) {
+ ret = fpga_mgr_firmware_load(mgr, info, info->firmware_name);
+ if (ret)
+ goto free_fw;
+ } else {
+ ret = -EINVAL;
+ }
+
+free_fw:
+ if (info->encrypted_key_name)
+ release_firmware(fw);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(fpga_mgr_load);
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index a6affd83f275..e9ddece4b82d 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -196,6 +196,7 @@ of_fpga_region_parse_ov(struct fpga_region *region,
struct device_node *overlay)
{
struct device *dev = ®ion->dev;
+ const char *encrypted_key_name;
struct fpga_image_info *info;
const char *firmware_name;
int ret;
@@ -238,6 +239,15 @@ of_fpga_region_parse_ov(struct fpga_region *region,
return ERR_PTR(-ENOMEM);
}
+ if (!of_property_read_string(overlay, "encrypted-key-name",
+ &encrypted_key_name)) {
+ info->encrypted_key_name = devm_kstrdup(dev,
+ encrypted_key_name,
+ GFP_KERNEL);
+ if (!info->encrypted_key_name)
+ return ERR_PTR(-ENOMEM);
+ }
+
of_property_read_u32(overlay, "region-unfreeze-timeout-us",
&info->enable_timeout_us);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 54f63459efd6..303264d89922 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -71,12 +71,15 @@ enum fpga_mgr_states {
* %FPGA_MGR_BITSTREAM_LSB_FIRST: SPI bitstream bit order is LSB first
*
* %FPGA_MGR_COMPRESSED_BITSTREAM: FPGA bitstream is compressed
+ *
+ * %FPGA_MGR_USRKEY_ENCRYPTED_BITSTREAM: indicates bitstream is encrypted with user-key
*/
#define FPGA_MGR_PARTIAL_RECONFIG BIT(0)
#define FPGA_MGR_EXTERNAL_CONFIG BIT(1)
#define FPGA_MGR_ENCRYPTED_BITSTREAM BIT(2)
#define FPGA_MGR_BITSTREAM_LSB_FIRST BIT(3)
#define FPGA_MGR_COMPRESSED_BITSTREAM BIT(4)
+#define FPGA_MGR_USRKEY_ENCRYPTED_BITSTREAM BIT(5)
/**
* struct fpga_image_info - information specific to an FPGA image
@@ -86,6 +89,7 @@ enum fpga_mgr_states {
* @config_complete_timeout_us: maximum time for FPGA to switch to operating
* status in the write_complete op.
* @firmware_name: name of FPGA image firmware file
+ * @encrypted_key_name: name of the FPGA image encrypted user-key file
* @sgt: scatter/gather table containing FPGA image
* @buf: contiguous buffer containing FPGA image
* @count: size of buf
@@ -102,6 +106,7 @@ struct fpga_image_info {
u32 disable_timeout_us;
u32 config_complete_timeout_us;
char *firmware_name;
+ char *encrypted_key_name;
struct sg_table *sgt;
const char *buf;
size_t count;
@@ -172,6 +177,9 @@ struct fpga_manager_ops {
bool skip_header;
enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
u64 (*status)(struct fpga_manager *mgr);
+ int (*parse_aes_key)(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
+ const char *buf, size_t count);
int (*parse_header)(struct fpga_manager *mgr,
struct fpga_image_info *info,
const char *buf, size_t count);
--
2.25.1
On Wed, Nov 22, 2023 at 11:14:02AM +0530, Nava kishore Manne wrote:
> Adds ‘encrypted-key-name’ property to support user-key encrypted
> bitstream loading use case.
>
> Signed-off-by: Nava kishore Manne <[email protected]>
> ---
> .../devicetree/bindings/fpga/fpga-region.txt | 32 +++++++++++++++++++
Is there a reason that this has not yet been converted to yaml?
> 1 file changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> index 528df8a0e6d8..309334558b3f 100644
> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> @@ -177,6 +177,9 @@ Optional properties:
> it indicates that the FPGA has already been programmed with this image.
> If this property is in an overlay targeting an FPGA region, it is a
> request to program the FPGA with that image.
> +- encrypted-key-name : should contain the name of an encrypted key file located
> + on the firmware search path. It will be used to decrypt the FPGA image
> + file with user-key.
I might be misreading things, but your driver code seems to assume that
this is an aes key. Nothing here seems to document that this is supposed
to be a key of a particular type.
Cheers,
Conor.
> - fpga-bridges : should contain a list of phandles to FPGA Bridges that must be
> controlled during FPGA programming along with the parent FPGA bridge.
> This property is optional if the FPGA Manager handles the bridges.
> @@ -459,6 +462,35 @@ programming is the FPGA based bridge of fpga_region1.
> };
> };
>
> +Device Tree Example: Configure/Reconfigure Encrypted Image With User Key
> +========================================================================
> +
> +Users can encrypt FPGA configuration Images with their own key. While decrypting
> +the configuration Image the user needs to provide the same key.
> +"encrypted-key-name" Specifies the name of the FPGA image encrypted key file on
> +the firmware search path. The search path is described in the firmware class
> +documentation.
> +
> +/dts-v1/;
> +/plugin/;
> +
> +&fpga_region0 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + firmware-name = "soc_image2.rbf";
> + encrypted-key-name = "key.nky";
> +
> + gpio@10040 {
> + compatible = "altr,pio-1.0";
> + reg = <0x10040 0x20>;
> + clocks = <0x2>;
> + altr,ngpio = <0x4>;
> + #gpio-cells = <0x2>;
> + gpio-controller;
> + };
> +};
> +
> Constraints
> ===========
>
> --
> 2.25.1
>
Hi Conor,
Thanks for providing the review comments.
Please find my response inline.
> -----Original Message-----
> From: Conor Dooley <[email protected]>
> Sent: Wednesday, November 22, 2023 10:21 PM
> To: Manne, Nava kishore <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Simek, Michal <[email protected]>;
> [email protected]; Levinsky, Ben <[email protected]>;
> Potthuri, Sai Krishna <[email protected]>; Shah, Tanmay
> <[email protected]>; [email protected]; [email protected];
> Datta, Shubhrajyoti <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [RFC PATCH 1/3] dt-bindings: fpga: Add support for user-key
> encrypted bitstream loading
>
> On Wed, Nov 22, 2023 at 11:14:02AM +0530, Nava kishore Manne wrote:
> > Adds ‘encrypted-key-name’ property to support user-key encrypted
> > bitstream loading use case.
> >
> > Signed-off-by: Nava kishore Manne <[email protected]>
> > ---
> > .../devicetree/bindings/fpga/fpga-region.txt | 32
> > +++++++++++++++++++
>
> Is there a reason that this has not yet been converted to yaml?
>
I am not sure about the complication involved here why it's not converted to yaml format.
Due to time constraints, I couldn’t spend much time so I have used this existing legacy format
to add my changes.
> > 1 file changed, 32 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > index 528df8a0e6d8..309334558b3f 100644
> > --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > @@ -177,6 +177,9 @@ Optional properties:
> > it indicates that the FPGA has already been programmed with this
> image.
> > If this property is in an overlay targeting an FPGA region, it is a
> > request to program the FPGA with that image.
> > +- encrypted-key-name : should contain the name of an encrypted key file
> located
> > + on the firmware search path. It will be used to decrypt the FPGA
> image
> > + file with user-key.
>
> I might be misreading things, but your driver code seems to assume that this
> is an aes key. Nothing here seems to document that this is supposed to be a
> key of a particular type.
>
Yes, these changes are intended to add the support for Aes user-key encrypted bitstream loading use case.
Will fix it in v2, something like below.
aes-key-file-name : Should contain the AES key file name on the firmware search path.
The key file contains the AES key and it will be used to decrypt the FPGA image.
Regards,
Navakishore.
On Fri, Nov 24, 2023 at 06:35:19AM +0000, Manne, Nava kishore wrote:
> Hi Conor,
>
> Thanks for providing the review comments.
> Please find my response inline.
>
> > -----Original Message-----
> > From: Conor Dooley <[email protected]>
> > Sent: Wednesday, November 22, 2023 10:21 PM
> > To: Manne, Nava kishore <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Simek, Michal <[email protected]>;
> > [email protected]; Levinsky, Ben <[email protected]>;
> > Potthuri, Sai Krishna <[email protected]>; Shah, Tanmay
> > <[email protected]>; [email protected]; [email protected];
> > Datta, Shubhrajyoti <[email protected]>; linux-
> > [email protected]; [email protected]; linux-
> > [email protected]; [email protected]
> > Subject: Re: [RFC PATCH 1/3] dt-bindings: fpga: Add support for user-key
> > encrypted bitstream loading
> >
> > On Wed, Nov 22, 2023 at 11:14:02AM +0530, Nava kishore Manne wrote:
> > > Adds ‘encrypted-key-name’ property to support user-key encrypted
> > > bitstream loading use case.
> > >
> > > Signed-off-by: Nava kishore Manne <[email protected]>
> > > ---
> > > .../devicetree/bindings/fpga/fpga-region.txt | 32
> > > +++++++++++++++++++
> >
> > Is there a reason that this has not yet been converted to yaml?
> >
> I am not sure about the complication involved here why it's not converted to yaml format.
> Due to time constraints, I couldn’t spend much time so I have used this existing legacy format
> to add my changes.
>
> > > 1 file changed, 32 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > index 528df8a0e6d8..309334558b3f 100644
> > > --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > @@ -177,6 +177,9 @@ Optional properties:
> > > it indicates that the FPGA has already been programmed with this
> > image.
> > > If this property is in an overlay targeting an FPGA region, it is a
> > > request to program the FPGA with that image.
> > > +- encrypted-key-name : should contain the name of an encrypted key file
> > located
> > > + on the firmware search path. It will be used to decrypt the FPGA
> > image
> > > + file with user-key.
> >
> > I might be misreading things, but your driver code seems to assume that this
> > is an aes key. Nothing here seems to document that this is supposed to be a
> > key of a particular type.
> >
>
> Yes, these changes are intended to add the support for Aes user-key encrypted bitstream loading use case.
> Will fix it in v2, something like below.
> aes-key-file-name : Should contain the AES key file name on the firmware search path.
> The key file contains the AES key and it will be used to decrypt the FPGA image.
Then when someone comes along looking for a different type of encryption
we will end up with national-pride-foo-file-name etc. I think I'd rather
have a second property that notes what type of cipher is being used and
if that property is not present default to AES.
On 24/11/2023 13:48, Conor Dooley wrote:
> On Fri, Nov 24, 2023 at 06:35:19AM +0000, Manne, Nava kishore wrote:
>> Hi Conor,
>>
>> Thanks for providing the review comments.
>> Please find my response inline.
>>
>>> -----Original Message-----
>>> From: Conor Dooley <[email protected]>
>>> Sent: Wednesday, November 22, 2023 10:21 PM
>>> To: Manne, Nava kishore <[email protected]>
>>> Cc: [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; Simek, Michal <[email protected]>;
>>> [email protected]; Levinsky, Ben <[email protected]>;
>>> Potthuri, Sai Krishna <[email protected]>; Shah, Tanmay
>>> <[email protected]>; [email protected]; [email protected];
>>> Datta, Shubhrajyoti <[email protected]>; linux-
>>> [email protected]; [email protected]; linux-
>>> [email protected]; [email protected]
>>> Subject: Re: [RFC PATCH 1/3] dt-bindings: fpga: Add support for user-key
>>> encrypted bitstream loading
>>>
>>> On Wed, Nov 22, 2023 at 11:14:02AM +0530, Nava kishore Manne wrote:
>>>> Adds ‘encrypted-key-name’ property to support user-key encrypted
>>>> bitstream loading use case.
>>>>
>>>> Signed-off-by: Nava kishore Manne <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/fpga/fpga-region.txt | 32
>>>> +++++++++++++++++++
>>>
>>> Is there a reason that this has not yet been converted to yaml?
>>>
>> I am not sure about the complication involved here why it's not converted to yaml format.
>> Due to time constraints, I couldn’t spend much time so I have used this existing legacy format
>> to add my changes.
>>
>>>> 1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt
>>>> b/Documentation/devicetree/bindings/fpga/fpga-region.txt
>>>> index 528df8a0e6d8..309334558b3f 100644
>>>> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
>>>> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
>>>> @@ -177,6 +177,9 @@ Optional properties:
>>>> it indicates that the FPGA has already been programmed with this
>>> image.
>>>> If this property is in an overlay targeting an FPGA region, it is a
>>>> request to program the FPGA with that image.
>>>> +- encrypted-key-name : should contain the name of an encrypted key file
>>> located
>>>> + on the firmware search path. It will be used to decrypt the FPGA
>>> image
>>>> + file with user-key.
>>>
>>> I might be misreading things, but your driver code seems to assume that this
>>> is an aes key. Nothing here seems to document that this is supposed to be a
>>> key of a particular type.
>>>
>>
>> Yes, these changes are intended to add the support for Aes user-key encrypted bitstream loading use case.
>> Will fix it in v2, something like below.
>> aes-key-file-name : Should contain the AES key file name on the firmware search path.
>> The key file contains the AES key and it will be used to decrypt the FPGA image.
>
> Then when someone comes along looking for a different type of encryption
> we will end up with national-pride-foo-file-name etc. I think I'd rather
> have a second property that notes what type of cipher is being used and
> if that property is not present default to AES.
I wonder why does it need to be in DT in the first place? Why it cannot
be appended to the FPGA binary image itself? Which also points to
dubious security aspect of this approach... Shipping FPGA encrypted
image with its decryption key sounds like marvelous idea.
Even if this is suitable, why not using more arguments of firmware-name?
This would scale even for multiple FPGA firmwares with different keys
(although such need seems unlikely).
Best regards,
Krzysztof
On 22/11/2023 06:44, Nava kishore Manne wrote:
> For user-key encrypted bitstream loading use case, users can encrypt
> FPGA configuration Images with their own key.While decrypting the
> configuration Image the user needs to provide the same key.To support
> this use case with the existing FPGA manager framework is not possible
> because it doesn’t have a mechanism to get the required inputs from
> the user. So this patch series adds the required changes to the FPGA
> manager framework to support user-key encrypted bitstream image loading
Wasn't the entire point of encrypted FPGA bistreams that the key is
fused into the FPGA and the FPGA does the decrypting? Otherwise it's
like security through obscurity - the only trouble for attacker is to
decode DTB to find the filename of key, so actually not even really
obscure. Then the attacker retrieves the key and bitstream from
filesystem (by taking out the Zynq-based SoM out or booting from own
system or just accessing storage pins directly) and voila: encrypted key
is available.
Best regards,
Krzysztof
On Fri, Nov 24, 2023 at 04:46:06PM +0100, Krzysztof Kozlowski wrote:
> On 24/11/2023 13:48, Conor Dooley wrote:
> > On Fri, Nov 24, 2023 at 06:35:19AM +0000, Manne, Nava kishore wrote:
> >> Hi Conor,
> >>
> >> Thanks for providing the review comments.
> >> Please find my response inline.
> >>
> >>> -----Original Message-----
> >>> From: Conor Dooley <[email protected]>
> >>> Sent: Wednesday, November 22, 2023 10:21 PM
> >>> To: Manne, Nava kishore <[email protected]>
> >>> Cc: [email protected]; [email protected]; [email protected];
> >>> [email protected]; [email protected]; [email protected];
> >>> [email protected]; Simek, Michal <[email protected]>;
> >>> [email protected]; Levinsky, Ben <[email protected]>;
> >>> Potthuri, Sai Krishna <[email protected]>; Shah, Tanmay
> >>> <[email protected]>; [email protected]; [email protected];
> >>> Datta, Shubhrajyoti <[email protected]>; linux-
> >>> [email protected]; [email protected]; linux-
> >>> [email protected]; [email protected]
> >>> Subject: Re: [RFC PATCH 1/3] dt-bindings: fpga: Add support for user-key
> >>> encrypted bitstream loading
> >>>
> >>> On Wed, Nov 22, 2023 at 11:14:02AM +0530, Nava kishore Manne wrote:
> >>>> Adds ‘encrypted-key-name’ property to support user-key encrypted
> >>>> bitstream loading use case.
> >>>>
> >>>> Signed-off-by: Nava kishore Manne <[email protected]>
> >>>> ---
> >>>> .../devicetree/bindings/fpga/fpga-region.txt | 32
> >>>> +++++++++++++++++++
> >>>
> >>> Is there a reason that this has not yet been converted to yaml?
> >>>
> >> I am not sure about the complication involved here why it's not converted to yaml format.
> >> Due to time constraints, I couldn’t spend much time so I have used this existing legacy format
> >> to add my changes.
> >>
> >>>> 1 file changed, 32 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> >>>> b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> >>>> index 528df8a0e6d8..309334558b3f 100644
> >>>> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> >>>> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> >>>> @@ -177,6 +177,9 @@ Optional properties:
> >>>> it indicates that the FPGA has already been programmed with this
> >>> image.
> >>>> If this property is in an overlay targeting an FPGA region, it is a
> >>>> request to program the FPGA with that image.
> >>>> +- encrypted-key-name : should contain the name of an encrypted key file
> >>> located
> >>>> + on the firmware search path. It will be used to decrypt the FPGA
> >>> image
> >>>> + file with user-key.
> >>>
> >>> I might be misreading things, but your driver code seems to assume that this
> >>> is an aes key. Nothing here seems to document that this is supposed to be a
> >>> key of a particular type.
> >>>
> >>
> >> Yes, these changes are intended to add the support for Aes user-key encrypted bitstream loading use case.
> >> Will fix it in v2, something like below.
> >> aes-key-file-name : Should contain the AES key file name on the firmware search path.
> >> The key file contains the AES key and it will be used to decrypt the FPGA image.
> >
> > Then when someone comes along looking for a different type of encryption
> > we will end up with national-pride-foo-file-name etc. I think I'd rather
> > have a second property that notes what type of cipher is being used and
> > if that property is not present default to AES.
>
> I wonder why does it need to be in DT in the first place? Why it cannot
> be appended to the FPGA binary image itself? Which also points to
> dubious security aspect of this approach... Shipping FPGA encrypted
> image with its decryption key sounds like marvelous idea.
>
> Even if this is suitable, why not using more arguments of firmware-name?
> This would scale even for multiple FPGA firmwares with different keys
> (although such need seems unlikely).
In case it is not clear (given the month's delay here), this question is
for the submitter of the series to answer, not me.
Cheers,
Conor.