[RFC 0/1] Proposal for new devlink command to enforce firmware security

[RFC 0/1] Proposal for new devlink command to enforce firmware security

[Martyna Szapar-Mudlaw]

RFC: Proposal for new devlink command to enforce firmware security

This RFC proposes addition of a new command to devlink API, providing a
mechanism to enforce secure firmware versions at the user's request. 
The primary goal is to enhance security by preventing the programming
of firmware with a lower security revision value. This feature is
particularly needed for Intel ice driver (and some other Intel drivers
next) but will be generic enough for other drivers to implement as well
for their purposes. Additionally, it proposes displaying the running
firmware security revision value and the set firmware minimum security
revision value in the output of the `devlink dev info` command for
Intel ice driver. 

Motivation

The E810 Ethernet controller provides a mechanism to prevent
downgrading to firmware versions containing known security
vulnerabilities. Specifically, the NVM components are signed
with a security revision, E810 ensures that components with
a lower security revision than the defined minimum cannot be loaded
onto the device.
Intel customers require that this action is not autonomous. Customers
typically uses this feature only in the target deployments after
finalizing testing of the new FW version. Autonomous action would
require manufacturer direct access to card to downgrade image to
previous stable version. By allowing the driver and user to manage the 
firmware security revision value, we can provide a flexible and secure
solution.
Additionally, displaying the current and minimum security revision
values in the `devlink dev info` command output will provide better
visibility and management for users.

Initial proposal for Minimum Security Revision update, via dedicated
parameter, was initially part of first devlink update deployment,
mechanism was questioned by community members [1]. However, Intel
still needs this functionality thus we are proposing a different
approach now to address the concerns raised previously. (In the last
community proposal, community proposed to make a decision based on
FW image - instead of dedicated parameter.)

Proposed design

New command, `devlink dev lock-firmware` (or `devlink dev guard-firmware`),
will be added to devlink API. Implementation in devlink will be simple
and generic, with no predefined operations, offering flexibility for drivers
to define the firmware locking mechanism appropriate to the hardware's
capabilities and security requirements. Running this command will allow
ice driver to ensure firmware with lower security value downgrades are
prevented.

Add also changes to Intel ice driver to display security values
via devlink dev info command running and set minimum. Also implement
lock-firmware devlink op callback in ice driver to update firmware
minimum security revision value.


Example usage:

$ devlink dev info pci/0000:b1:00.0

pci/0000:b1:00.0:
  driver ice
  serial_number 00-01-00-ff-ff-00-00-00
  versions:
      fixed:
        fw.mgmt.min.srev 8
        fw.undi.min.srev 8
      running:
        fw.mgmt.srev 9
        fw.undi.srev 9
        

$ devlink dev lock-firmware pci/0000:03:00.0

WARNING: This action will prevent downgrades to versions with lower the
security version Are you sure you want to lock the firmware on device %s?. (y/N)
>y

$ devlink dev info pci/0000:03:00.0

pci/0000:03:00.0:
  driver ice
  versions:
      fixed:
        fw.mgmt.min.srev 9
        fw.undi.min.srev 9


This feature is essential for the `ice` Ethernet driver (and other
Intel drivers next) but is designed to be generic for other drivers
to implement. Feedback and suggestions are welcome.

[1] https://lore.kernel.org/netdev/20210203124112.67a1e1ee@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/T/#u


Martyna Szapar-Mudlaw (1):
  devlink: add new devlink lock-firmware command

 include/net/devlink.h        |  2 ++
 include/uapi/linux/devlink.h |  2 ++
 net/devlink/dev.c            | 13 +++++++++++++
 net/devlink/netlink_gen.c    | 18 +++++++++++++++++-
 net/devlink/netlink_gen.h    |  4 +++-
 5 files changed, 37 insertions(+), 2 deletions(-)

-- 
2.47.0

[RFC 1/1] devlink: add new devlink lock-firmware command

[Martyna Szapar-Mudlaw]

Introduce a new lock-firmware command to the devlink API,
providing a mechanism to enforce secure firmware versions at the user's request.
The lock-firmware command invokes a driver-implemented callback,
allowing hardware vendors to implement a mechanism for the specific
hardware to block undesirable firmware downgrades possibilities.
This ensures that firmware downgrades, which could expose devices
to known vulnerabilities, are prevented in security-sensitive deployments.

Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
---
Note: This is only RFC draft. As expected, the code that should be
automatically generated will eventually not be manually entered here.
Documentation will be added.
---
 include/net/devlink.h        |  2 ++
 include/uapi/linux/devlink.h |  2 ++
 net/devlink/dev.c            | 13 +++++++++++++
 net/devlink/netlink_gen.c    | 18 +++++++++++++++++-
 net/devlink/netlink_gen.h    |  4 +++-
 5 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index d5da362ea321..1b19f7c4550b 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1354,6 +1354,8 @@ struct devlink_ops {
 	int (*flash_update)(struct devlink *devlink,
 			    struct devlink_flash_update_params *params,
 			    struct netlink_ext_ack *extack);
+	int (*lock_firmware)(struct devlink *devlink,
+				   struct netlink_ext_ack *extack);
 	/**
 	 * @trap_init: Trap initialization function.
 	 *
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 9401aa343673..8f37cef858e2 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -141,6 +141,8 @@ enum devlink_command {
 
 	DEVLINK_CMD_NOTIFY_FILTER_SET,
 
+	DEVLINK_CMD_LOCK_FIRMWARE,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
diff --git a/net/devlink/dev.c b/net/devlink/dev.c
index d6e3db300acb..7bb7617a8080 100644
--- a/net/devlink/dev.c
+++ b/net/devlink/dev.c
@@ -1440,3 +1440,16 @@ int devlink_nl_selftests_run_doit(struct sk_buff *skb, struct genl_info *info)
 	nlmsg_free(msg);
 	return err;
 }
+
+int devlink_nl_lock_firmware_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	int ret;
+
+	if (!devlink->ops->lock_firmware)
+		return -EOPNOTSUPP;
+
+	ret = devlink->ops->lock_firmware(devlink, info->extack);
+
+	return ret;
+}
diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c
index f9786d51f68f..2d12fb09ad48 100644
--- a/net/devlink/netlink_gen.c
+++ b/net/devlink/netlink_gen.c
@@ -567,8 +567,14 @@ static const struct nla_policy devlink_notify_filter_set_nl_policy[DEVLINK_ATTR_
 	[DEVLINK_ATTR_PORT_INDEX] = { .type = NLA_U32, },
 };
 
+/* DEVLINK_CMD_LOCK_FIRMWARE - do */
+static const struct nla_policy devlink_lock_firmware_nl_policy[DEVLINK_ATTR_DEV_NAME + 1] = {
+	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, },
+	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, },
+};
+
 /* Ops table for devlink */
-const struct genl_split_ops devlink_nl_ops[74] = {
+const struct genl_split_ops devlink_nl_ops[75] = {
 	{
 		.cmd		= DEVLINK_CMD_GET,
 		.validate	= GENL_DONT_VALIDATE_STRICT,
@@ -1247,4 +1253,14 @@ const struct genl_split_ops devlink_nl_ops[74] = {
 		.maxattr	= DEVLINK_ATTR_PORT_INDEX,
 		.flags		= GENL_CMD_CAP_DO,
 	},
+	{
+		.cmd		= DEVLINK_CMD_LOCK_FIRMWARE,
+		.validate	= GENL_DONT_VALIDATE_STRICT,
+		.pre_doit	= devlink_nl_pre_doit,
+		.doit		= devlink_nl_lock_firmware_doit,
+		.post_doit	= devlink_nl_post_doit,
+		.policy		= devlink_lock_firmware_nl_policy,
+		.maxattr	= DEVLINK_ATTR_DEV_NAME,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+	},
 };
diff --git a/net/devlink/netlink_gen.h b/net/devlink/netlink_gen.h
index 8f2bd50ddf5e..f0129da6a81b 100644
--- a/net/devlink/netlink_gen.h
+++ b/net/devlink/netlink_gen.h
@@ -16,7 +16,7 @@ extern const struct nla_policy devlink_dl_port_function_nl_policy[DEVLINK_PORT_F
 extern const struct nla_policy devlink_dl_selftest_id_nl_policy[DEVLINK_ATTR_SELFTEST_ID_FLASH + 1];
 
 /* Ops table for devlink */
-extern const struct genl_split_ops devlink_nl_ops[74];
+extern const struct genl_split_ops devlink_nl_ops[75];
 
 int devlink_nl_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
 			struct genl_info *info);
@@ -144,5 +144,7 @@ int devlink_nl_selftests_get_dumpit(struct sk_buff *skb,
 int devlink_nl_selftests_run_doit(struct sk_buff *skb, struct genl_info *info);
 int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
 				      struct genl_info *info);
+int devlink_nl_lock_firmware_doit(struct sk_buff *skb,
+				  struct genl_info *info);
 
 #endif /* _LINUX_DEVLINK_GEN_H */
-- 
2.47.0

Re: [RFC 0/1] Proposal for new devlink command to enforce firmware security

[Jakub Kicinski]

On Mon,  9 Dec 2024 14:14:50 +0100 Martyna Szapar-Mudlaw wrote:
> Proposed design
> 
> New command, `devlink dev lock-firmware` (or `devlink dev guard-firmware`),
> will be added to devlink API. Implementation in devlink will be simple
> and generic, with no predefined operations, offering flexibility for drivers
> to define the firmware locking mechanism appropriate to the hardware's
> capabilities and security requirements. Running this command will allow
> ice driver to ensure firmware with lower security value downgrades are
> prevented.
> 
> Add also changes to Intel ice driver to display security values
> via devlink dev info command running and set minimum. Also implement
> lock-firmware devlink op callback in ice driver to update firmware
> minimum security revision value.

devlink doesn't have a suitable security model. I don't think we should
be adding hacks since we're not security experts and standards like SPDM
exist.

I understand that customers ask for this but "security" is not a
checkbox, the whole certificate and version management is necessary.

Re: [RFC 0/1] Proposal for new devlink command to enforce firmware security

[Szapar-Mudlaw, Martyna]

On 12/10/2024 12:36 AM, Jakub Kicinski wrote:
> On Mon,  9 Dec 2024 14:14:50 +0100 Martyna Szapar-Mudlaw wrote:
>> Proposed design
>>
>> New command, `devlink dev lock-firmware` (or `devlink dev guard-firmware`),
>> will be added to devlink API. Implementation in devlink will be simple
>> and generic, with no predefined operations, offering flexibility for drivers
>> to define the firmware locking mechanism appropriate to the hardware's
>> capabilities and security requirements. Running this command will allow
>> ice driver to ensure firmware with lower security value downgrades are
>> prevented.
>>
>> Add also changes to Intel ice driver to display security values
>> via devlink dev info command running and set minimum. Also implement
>> lock-firmware devlink op callback in ice driver to update firmware
>> minimum security revision value.
> 
> devlink doesn't have a suitable security model. I don't think we should
> be adding hacks since we're not security experts and standards like SPDM
> exist.
> 
> I understand that customers ask for this but "security" is not a
> checkbox, the whole certificate and version management is necessary.
> 

Hi Jakub,

Thank you for your response. Apologies if any of earlier wording was 
unclear or poorly chosen.

While this feature is needed for security reasons, its implementation in 
the kernel isn't directly tied to kernel/driver security.

The E810 Ethernet controller firmware provides a certain level of 
security, which includes a mechanism to prevent firmware downgrades (to 
past, less secure versions). However, it is the driver that needs to 
initiate this mechanism. After "locking/fusing/freezing/guarding" (open 
to name suggestions) the current firmware version, upgrades would still 
be possible. The card itself handles firmware validation, including 
verifying signatures and ensuring that only properly signed and accepted 
firmware can be installed. Thus the firmware can only be upgraded to a 
validated version that the card has approved.

This patch does not aim to introduce a new security mechanism, rather, 
it enables users to utilize the controller's existing functionality. 
This feature is to provide users with a devlink interface to inform the 
device that the currently loaded firmware can become the new minimal 
version for the card. Users have specifically requested the ability to 
make this step an independent part of their firmware update process.
Leaving in-tree users without this capability exposes them to the risk 
of downgrades to older, released by Intel, but potentially compromised 
fw versions, and prevents the intended security protections of the 
device from being utilized.
On the other hand always enforcing this mechanism during firmware 
update, could lead to poor customer experiences due to unintended 
firmware behavior in specific workflows and is not accepted by Intel 
customers.

Devlink tool was proposed for this purpose, as it is designed for 
administrative/root-level tasks such as this. Perhaps it would be more 
appropriate to integrate the proposed implementation as a subcommand 
(attribute) under the devlink flash API, which was the second considered 
option, rather than keeping it as a separate devlink command?

Thank you and best regards,
Martyna

Re: [RFC 0/1] Proposal for new devlink command to enforce firmware security

[Andrew Lunn]

> The E810 Ethernet controller firmware provides a certain level of security,
> which includes a mechanism to prevent firmware downgrades (to past, less
> secure versions).

Is there anything Ethernet specific here? I assume the same API could
be used for GPU firmware, ATA controller firmware, mice firmware?

Maybe you should pull this discussion up to a higher level, covering
all drivers, not just netdev.

	Andrew

Re: [RFC 0/1] Proposal for new devlink command to enforce firmware security

[Jakub Kicinski]

On Wed, 11 Dec 2024 13:15:06 +0100 Szapar-Mudlaw, Martyna wrote:
> This patch does not aim to introduce a new security mechanism, rather, 

What I was referring to when I said "devlink doesn't have a suitable
security model" is that we have no definition of what security guarantees
we provide. Nothing in devlink is authenticated at all. 

Anti-rollback is fundamentally about preventing FW compromise.
How do you know that the FW is not compromised with devlink?

> it enables users to utilize the controller's existing functionality. 
> This feature is to provide users with a devlink interface to inform the 
> device that the currently loaded firmware can become the new minimal 
> version for the card. Users have specifically requested the ability to 
> make this step an independent part of their firmware update process.

I know, I've heard it for my internal users too. Vendors put some
"device is secure" checkbox and some SREs without security training
think that this is enough and should be supported by devlink.

> Leaving in-tree users without this capability exposes them to the risk 
> of downgrades to older, released by Intel, but potentially compromised 
> fw versions, and prevents the intended security protections of the 
> device from being utilized.
> On the other hand always enforcing this mechanism during firmware 
> update, could lead to poor customer experiences due to unintended 
> firmware behavior in specific workflows and is not accepted by Intel 
> customers.

Please point me to relevant standard that supports locking in security
revision as an action separate from FW update, and over an insecure
channel.

If you can't find one, please let's not revisit this conversation.

Re: [RFC 0/1] Proposal for new devlink command to enforce firmware security

[Przemek Kitszel]

On 12/12/24 03:11, Jakub Kicinski wrote:
> On Wed, 11 Dec 2024 13:15:06 +0100 Szapar-Mudlaw, Martyna wrote:
>> This patch does not aim to introduce a new security mechanism, rather,
> 
> What I was referring to when I said "devlink doesn't have a suitable
> security model" is that we have no definition of what security guarantees
> we provide. Nothing in devlink is authenticated at all.
> 
> Anti-rollback is fundamentally about preventing FW compromise.
> How do you know that the FW is not compromised with devlink?

our FW (on-card little CPU) will reject a spooky FW image, only images
signed by Intel's priv key are accepted

> 
>> it enables users to utilize the controller's existing functionality.
>> This feature is to provide users with a devlink interface to inform the
>> device that the currently loaded firmware can become the new minimal
>> version for the card. Users have specifically requested the ability to
>> make this step an independent part of their firmware update process.
> 
> I know, I've heard it for my internal users too. Vendors put some
> "device is secure" checkbox and some SREs without security training
> think that this is enough and should be supported by devlink.
> 
>> Leaving in-tree users without this capability exposes them to the risk
>> of downgrades to older, released by Intel, but potentially compromised
>> fw versions, and prevents the intended security protections of the
>> device from being utilized.
>> On the other hand always enforcing this mechanism during firmware
>> update, could lead to poor customer experiences due to unintended
>> firmware behavior in specific workflows and is not accepted by Intel
>> customers.
> 
> Please point me to relevant standard that supports locking in security
> revision as an action separate from FW update, and over an insecure
> channel.
> 
> If you can't find one, please let's not revisit this conversation.

It's not standard, just the design for our e810 (e82x?) FW, but we could 
achieve the goal in one step, while preserving the opt-out mechanism for
those unwilling customers. I think that this will allow at least some
customers to prevent possibility of running a known-to-be-bad FW
(prior to opening given server to the world).

We could simply add DEVLINK_FLASH_OVERWRITE_MIN_VERSION (*) flag for the
single-step flash update [1], and do the update AND bump our "minsrev"
in one step.
The worst that could happen, is that customer will get some newer
version of the firmware (but a one released by Intel).
We preserve the simplicity of one .bin file with that too.

We will prepare a proper v2 of the RFC, reaching broader community,
but will check locally to see if there is a similar scheme in some
other intel (non eth) device first.
But please stop us if you believe that this idea is also
a phantasmagoria.

[1] https://docs.kernel.org/networking/devlink/devlink-flash.html
(*) name suggestions, as always, much welcomed

Re: [RFC 0/1] Proposal for new devlink command to enforce firmware security

[Jakub Kicinski]

On Mon, 16 Dec 2024 16:09:12 +0100 Przemek Kitszel wrote:
> > Please point me to relevant standard that supports locking in security
> > revision as an action separate from FW update, and over an insecure
> > channel.
> > 
> > If you can't find one, please let's not revisit this conversation.  
> 
> It's not standard, just the design for our e810 (e82x?) FW, but we could 
> achieve the goal in one step, while preserving the opt-out mechanism for
> those unwilling customers. I think that this will allow at least some
> customers to prevent possibility of running a known-to-be-bad FW
> (prior to opening given server to the world).
> 
> We could simply add DEVLINK_FLASH_OVERWRITE_MIN_VERSION (*) flag for the
> single-step flash update [1], and do the update AND bump our "minsrev"
> in one step.
> The worst that could happen, is that customer will get some newer
> version of the firmware (but a one released by Intel).
> We preserve the simplicity of one .bin file with that too.

Please explain to me how this will all fit into the existing standards
like SPDM. Please take security seriously.. this is not just another
knob. How will attestation of the fact that someone flipped the knob
work?

A much better workaround would be for you to build multiple FW images
and give customers an image which locks in the min version and one
which doesn't.