| Message ID | 20210129004332.3004826-5-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | 100GbE Intel Wired LAN Driver Updates 2021-01-28 | expand |
On Thu, 28 Jan 2021 16:43:21 -0800 Tony Nguyen wrote: > From: Jacob Keller <jacob.e.keller@intel.com> > > The ice NVM flash has a security revision field for the main NVM bank > and the Option ROM bank. In addition to the revision within the module, > the device also has a minimum security revision TLV area. This minimum > security revision field indicates the minimum value that will be > accepted for the associated security revision when loading the NVM bank. > > Add functions to read and update the minimum security revisions. Use > these functions to implement devlink parameters, "fw.undi.minsrev" and > "fw.mgmt.minsrev". > > These parameters are permanent (i.e. stored in flash), and are used to > indicate the minimum security revision of the associated NVM bank. If > the image in the bank has a lower security revision, then the flash > loader will not continue loading that flash bank. > > The new parameters allow for opting in to update the minimum security > revision to ensure that a flash image with a known security flaw cannot > be loaded. > > Note that the minimum security revision cannot be reduced, only > increased. The driver also refuses to allow an update if the currently > active image revision is lower than the requested value. This is done to > avoid potentially updating the value such that the device can no longer > start. Hi Jake, I had a couple of conversations with people from operations and I'm struggling to find interest in writing this parameter. It seems like the expectation is that the min sec revision will go up automatically after a new firmware with a higher number is flashed. Do you have a user scenario where the manual bumping is needed?
On 2/3/2021 12:41 PM, Jakub Kicinski wrote: > On Thu, 28 Jan 2021 16:43:21 -0800 Tony Nguyen wrote: >> From: Jacob Keller <jacob.e.keller@intel.com> >> >> The ice NVM flash has a security revision field for the main NVM bank >> and the Option ROM bank. In addition to the revision within the module, >> the device also has a minimum security revision TLV area. This minimum >> security revision field indicates the minimum value that will be >> accepted for the associated security revision when loading the NVM bank. >> >> Add functions to read and update the minimum security revisions. Use >> these functions to implement devlink parameters, "fw.undi.minsrev" and >> "fw.mgmt.minsrev". >> >> These parameters are permanent (i.e. stored in flash), and are used to >> indicate the minimum security revision of the associated NVM bank. If >> the image in the bank has a lower security revision, then the flash >> loader will not continue loading that flash bank. >> >> The new parameters allow for opting in to update the minimum security >> revision to ensure that a flash image with a known security flaw cannot >> be loaded. >> >> Note that the minimum security revision cannot be reduced, only >> increased. The driver also refuses to allow an update if the currently >> active image revision is lower than the requested value. This is done to >> avoid potentially updating the value such that the device can no longer >> start. Hi Jakub, > > Hi Jake, I had a couple of conversations with people from operations > and I'm struggling to find interest in writing this parameter. >> It seems like the expectation is that the min sec revision will go up > automatically after a new firmware with a higher number is flashed. > I believe the intention is that the update is not automatic, and requires the user to opt-in to enforcing the new minimum value. This is because once you update this value it is not possible to lower it without physical access to reflash the chip directly. It's intended as a mechanism to allow a system administrator to ensure that the board is unable to downgrade below a given minimum security revision. > Do you have a user scenario where the manual bumping is needed? > In our case, we have tools which would use this interface and would perform the update upon request i.e. because the tool is configured to perform the update. We don't want this field to be updated every time the board is flashed, as it is supposed to be an optional "opt-in", and not forced. The flow is something like: a) device is as firmware version with SREV of 1 b) new firmware is flashed with SREV 2 c) system administrator confirms that new firmware is working and that no issues have occurred d) system administrator then decides to enforce new srev by updating the minimum srev value. If there was an issue at step (c), we want to still be able to roll back to the old firmware. If the minimum srev is updated automatically, this would not be possible. I've asked for further details from some of our firmware folks, and can try to provide further information. The key is that making it automatic is bad because it prevents rollback, so we want to ensure that it is only updated after the system administrator is ready to opt-in. Ofcourse, it is plausible that most won't actually update this ever, because preventing the ability to use an old firmware might not be desired. The goal with this series was to provide a mechanism to allow the update, because existing tools based on direct flash access have support for this, and we want to ensure that these tools can be ported to devlink without the direct flash access that we were (ab)using before. Thanks, Jake
On Wed, 3 Feb 2021 17:34:24 -0800 Jacob Keller wrote: > On 2/3/2021 12:41 PM, Jakub Kicinski wrote: > > On Thu, 28 Jan 2021 16:43:21 -0800 Tony Nguyen wrote: > >> From: Jacob Keller <jacob.e.keller@intel.com> > >> > >> The ice NVM flash has a security revision field for the main NVM bank > >> and the Option ROM bank. In addition to the revision within the module, > >> the device also has a minimum security revision TLV area. This minimum > >> security revision field indicates the minimum value that will be > >> accepted for the associated security revision when loading the NVM bank. > >> > >> Add functions to read and update the minimum security revisions. Use > >> these functions to implement devlink parameters, "fw.undi.minsrev" and > >> "fw.mgmt.minsrev". > >> > >> These parameters are permanent (i.e. stored in flash), and are used to > >> indicate the minimum security revision of the associated NVM bank. If > >> the image in the bank has a lower security revision, then the flash > >> loader will not continue loading that flash bank. > >> > >> The new parameters allow for opting in to update the minimum security > >> revision to ensure that a flash image with a known security flaw cannot > >> be loaded. > >> > >> Note that the minimum security revision cannot be reduced, only > >> increased. The driver also refuses to allow an update if the currently > >> active image revision is lower than the requested value. This is done to > >> avoid potentially updating the value such that the device can no longer > >> start. > > > > Hi Jake, I had a couple of conversations with people from operations > > and I'm struggling to find interest in writing this parameter. > >> It seems like the expectation is that the min sec revision will go up > > automatically after a new firmware with a higher number is flashed. > > I believe the intention is that the update is not automatic, and > requires the user to opt-in to enforcing the new minimum value. This is > because once you update this value it is not possible to lower it > without physical access to reflash the chip directly. It's intended as a > mechanism to allow a system administrator to ensure that the board is > unable to downgrade below a given minimum security revision. > > > Do you have a user scenario where the manual bumping is needed? > > > > In our case, we have tools which would use this interface and would > perform the update upon request i.e. because the tool is configured to > perform the update. > > We don't want this field to be updated every time the board is flashed, > as it is supposed to be an optional "opt-in", and not forced. > > The flow is something like: > > a) device is as firmware version with SREV of 1 > b) new firmware is flashed with SREV 2 > c) system administrator confirms that new firmware is working and that > no issues have occurred > d) system administrator then decides to enforce new srev by updating the > minimum srev value. Dunno, seems to me secure by default is a better approach. If admin is worried you can always ship an eval build which does not bump the version. Or address the issues with the next release rather than roll back. > If there was an issue at step (c), we want to still be able to roll back > to the old firmware. If the minimum srev is updated automatically, this > would not be possible. > > I've asked for further details from some of our firmware folks, and can > try to provide further information. The key is that making it automatic > is bad because it prevents rollback, so we want to ensure that it is > only updated after the system administrator is ready to opt-in. > > Ofcourse, it is plausible that most won't actually update this ever, > because preventing the ability to use an old firmware might not be desired. Well, if there is a point to secure boot w/ NICs people better prevent replay attacks. Not every FW update is a security update, tho, so it's not like "going to the old version" would never be possible. > The goal with this series was to provide a mechanism to allow the > update, because existing tools based on direct flash access have support > for this, and we want to ensure that these tools can be ported to > devlink without the direct flash access that we were (ab)using before. I'm not completely opposed to this mechanism (although you may want to communicate the approach to your customers clearly, because many may be surprised) - but let's be clear - the goal of devlink is to create a replacement for vendor tooling, not be their underlying mechanism.
On 2/3/2021 6:08 PM, Jakub Kicinski wrote: > On Wed, 3 Feb 2021 17:34:24 -0800 Jacob Keller wrote: >> On 2/3/2021 12:41 PM, Jakub Kicinski wrote: >>> On Thu, 28 Jan 2021 16:43:21 -0800 Tony Nguyen wrote: >>>> From: Jacob Keller <jacob.e.keller@intel.com> >>>> >>>> The ice NVM flash has a security revision field for the main NVM bank >>>> and the Option ROM bank. In addition to the revision within the module, >>>> the device also has a minimum security revision TLV area. This minimum >>>> security revision field indicates the minimum value that will be >>>> accepted for the associated security revision when loading the NVM bank. >>>> >>>> Add functions to read and update the minimum security revisions. Use >>>> these functions to implement devlink parameters, "fw.undi.minsrev" and >>>> "fw.mgmt.minsrev". >>>> >>>> These parameters are permanent (i.e. stored in flash), and are used to >>>> indicate the minimum security revision of the associated NVM bank. If >>>> the image in the bank has a lower security revision, then the flash >>>> loader will not continue loading that flash bank. >>>> >>>> The new parameters allow for opting in to update the minimum security >>>> revision to ensure that a flash image with a known security flaw cannot >>>> be loaded. >>>> >>>> Note that the minimum security revision cannot be reduced, only >>>> increased. The driver also refuses to allow an update if the currently >>>> active image revision is lower than the requested value. This is done to >>>> avoid potentially updating the value such that the device can no longer >>>> start. >>> >>> Hi Jake, I had a couple of conversations with people from operations >>> and I'm struggling to find interest in writing this parameter. >>>> It seems like the expectation is that the min sec revision will go up >>> automatically after a new firmware with a higher number is flashed. >> >> I believe the intention is that the update is not automatic, and >> requires the user to opt-in to enforcing the new minimum value. This is >> because once you update this value it is not possible to lower it >> without physical access to reflash the chip directly. It's intended as a >> mechanism to allow a system administrator to ensure that the board is >> unable to downgrade below a given minimum security revision. >> >>> Do you have a user scenario where the manual bumping is needed? >>> >> >> In our case, we have tools which would use this interface and would >> perform the update upon request i.e. because the tool is configured to >> perform the update. >> >> We don't want this field to be updated every time the board is flashed, >> as it is supposed to be an optional "opt-in", and not forced. >> >> The flow is something like: >> >> a) device is as firmware version with SREV of 1 >> b) new firmware is flashed with SREV 2 >> c) system administrator confirms that new firmware is working and that >> no issues have occurred >> d) system administrator then decides to enforce new srev by updating the >> minimum srev value. > > Dunno, seems to me secure by default is a better approach. If admin > is worried you can always ship an eval build which does not bump the > version. Or address the issues with the next release rather than roll > back. > >> If there was an issue at step (c), we want to still be able to roll back >> to the old firmware. If the minimum srev is updated automatically, this >> would not be possible. >> >> I've asked for further details from some of our firmware folks, and can >> try to provide further information. The key is that making it automatic >> is bad because it prevents rollback, so we want to ensure that it is >> only updated after the system administrator is ready to opt-in. >> >> Ofcourse, it is plausible that most won't actually update this ever, >> because preventing the ability to use an old firmware might not be desired. > > Well, if there is a point to secure boot w/ NICs people better prevent > replay attacks. Not every FW update is a security update, tho, so it's > not like "going to the old version" would never be possible. > Correct. The issue only comes up for firmware versions which have an updated security revision, which we do not expect to occur every release. >> The goal with this series was to provide a mechanism to allow the >> update, because existing tools based on direct flash access have support >> for this, and we want to ensure that these tools can be ported to >> devlink without the direct flash access that we were (ab)using before. > > I'm not completely opposed to this mechanism (although you may want > to communicate the approach to your customers clearly, because many > may be surprised) I do admit the primary motivation for this implementation was to enable the existing tooling to function. I'd rather see the right solution designed here, so if this isn't the right direction I want to work with the list to figure out what makes the most sense. (Even if that's "minimum security should update automatically"). Perhaps some extension of the info mechanism or other solution is also preferable so that it is more generic? But I don't have any example from other vendors that matches this so I'm not sure what makes the most sense here. > but let's be clear - the goal of devlink is to > create a replacement for vendor tooling, not be their underlying > mechanism. > The intention of modifying our tools is to help ensure existing practices and tools can continue to work. This is primarily being done to appease folks who "just want things to keep working as is". I agree with the goal of devlink. But convincing everyone here to move is a slow process that mostly involves educating folks one by one.
On 2/3/2021 6:08 PM, Jakub Kicinski wrote: > On Wed, 3 Feb 2021 17:34:24 -0800 Jacob Keller wrote: >> On 2/3/2021 12:41 PM, Jakub Kicinski wrote: >>> On Thu, 28 Jan 2021 16:43:21 -0800 Tony Nguyen wrote: >>>> From: Jacob Keller <jacob.e.keller@intel.com> >>>> >>>> The ice NVM flash has a security revision field for the main NVM bank >>>> and the Option ROM bank. In addition to the revision within the module, >>>> the device also has a minimum security revision TLV area. This minimum >>>> security revision field indicates the minimum value that will be >>>> accepted for the associated security revision when loading the NVM bank. >>>> >>>> Add functions to read and update the minimum security revisions. Use >>>> these functions to implement devlink parameters, "fw.undi.minsrev" and >>>> "fw.mgmt.minsrev". >>>> >>>> These parameters are permanent (i.e. stored in flash), and are used to >>>> indicate the minimum security revision of the associated NVM bank. If >>>> the image in the bank has a lower security revision, then the flash >>>> loader will not continue loading that flash bank. >>>> >>>> The new parameters allow for opting in to update the minimum security >>>> revision to ensure that a flash image with a known security flaw cannot >>>> be loaded. >>>> >>>> Note that the minimum security revision cannot be reduced, only >>>> increased. The driver also refuses to allow an update if the currently >>>> active image revision is lower than the requested value. This is done to >>>> avoid potentially updating the value such that the device can no longer >>>> start. >>> >>> Hi Jake, I had a couple of conversations with people from operations >>> and I'm struggling to find interest in writing this parameter. >>>> It seems like the expectation is that the min sec revision will go up >>> automatically after a new firmware with a higher number is flashed. >> >> I believe the intention is that the update is not automatic, and >> requires the user to opt-in to enforcing the new minimum value. This is >> because once you update this value it is not possible to lower it >> without physical access to reflash the chip directly. It's intended as a >> mechanism to allow a system administrator to ensure that the board is >> unable to downgrade below a given minimum security revision. >> >>> Do you have a user scenario where the manual bumping is needed? >>> I've spoken with some of our customer support engineers. Feedback I've received is that this was implemented as an automatic/default/enforced update in past products. Several customers have indicated that they want to be in control of when this update happens, and not to have it happen automatically. Specifically I've been asked to ensure this update is something that must be "opt in" and not by default, because of the issues we've received from vendors. > > Dunno, seems to me secure by default is a better approach. If admin > is worried you can always ship an eval build which does not bump the > version. Or address the issues with the next release rather than roll > back. > This is how we had it implemented in previous products, but we got significant feedback that it should be a step that can be controlled by the admin, so that they can decide when it is appropriate. Making this the default behavior that the driver automatically occurs after an update is not something we want, based on the feedback we've received in our previous products. The feedback that we've received is that a "one size fits all" automatic update of the minimum value is not acceptable to all of our customers. >> >> Ofcourse, it is plausible that most won't actually update this ever, >> because preventing the ability to use an old firmware might not be desired. > > Well, if there is a point to secure boot w/ NICs people better prevent > replay attacks. Not every FW update is a security update, tho, so it's > not like "going to the old version" would never be possible. > After I spoke to some folks internally I believe I misstated above: it's not about never updating this, but about being in control of when to perform the update. >> The goal with this series was to provide a mechanism to allow the >> update, because existing tools based on direct flash access have support >> for this, and we want to ensure that these tools can be ported to >> devlink without the direct flash access that we were (ab)using before. > > I'm not completely opposed to this mechanism (although you may want > to communicate the approach to your customers clearly, because many > may be surprised) - but let's be clear - the goal of devlink is to > create a replacement for vendor tooling, not be their underlying > mechanism. > We want some mechanism to allow administrators to decide when to update this value. We do not want to have a "default" be to update because that means the system administrators who want control over the process might accidentally perform a non-reversible operation. (Once you update the value you can't lower it without physically accessing the flash chip). I understand if the use of vendor-specific parameters here isn't acceptable. I'm happy to help design a more generic solution that avoids these and potentially integrates better into the flash update process. There is also the technical question of when the update can be performed. As is, it's a software controlled update, and must occur after the new flash image is booted, in order to ensure we do not update the value in a way that bricks the currently running firmware from booting.
On 2/4/2021 11:10 AM, Jacob Keller wrote: > I'd rather see the right solution designed here, so if this isn't the > right direction I want to work with the list to figure out what makes > the most sense. (Even if that's "minimum security should update > automatically"). > I want to clarify here based on feedback I received from customer support engineers: We believe it is not acceptable to update this automatically, because not all customers want that behavior and would prefer to have control over when to lock in the minimum security revision. Previous products have behaved this way and we had significant feedback when this occurred that many of our customers were unhappy about this, even after we explained the reasoning. I do not believe that we can accept an automatic/default update of minimum security revision.
From: Jacob Keller <jacob.e.keller@intel.com> Sent: Thursday, February 4, 2021 1:54 PM To: Jakub Kicinski <kuba@kernel.org> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; netdev@vger.kernel.org; sassmann@redhat.com; Brelinski, TonyX <tonyx.brelinski@intel.com> Subject: Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision On 2/4/2021 11:10 AM, Jacob Keller wrote: > I'd rather see the right solution designed here, so if this isn't the > right direction I want to work with the list to figure out what makes > the most sense. (Even if that's "minimum security should update > automatically"). > I want to clarify here based on feedback I received from customer support engineers: We believe it is not acceptable to update this automatically, because not all customers want that behavior and would prefer to have control over when to lock in the minimum security revision. Previous products have behaved this way and we had significant feedback when this occurred that many of our customers were unhappy about this, even after we explained the reasoning. I do not believe that we can accept an automatic/default update of minimum security revision. ---------------------------------------------- I tested this revision: Tested-by: Tony Brelinski <tonyx.brelinski@intel.com> A Contingent Worker at Intel
From: Brelinski, TonyX Sent: Friday, February 5, 2021 6:32 PM To: Jacob Keller <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; netdev@vger.kernel.org; sassmann@redhat.com Subject: RE: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision From: Jacob Keller <jacob.e.keller@intel.com> Sent: Thursday, February 4, 2021 1:54 PM To: Jakub Kicinski <kuba@kernel.org> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; netdev@vger.kernel.org; sassmann@redhat.com; Brelinski, TonyX <tonyx.brelinski@intel.com> Subject: Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision On 2/4/2021 11:10 AM, Jacob Keller wrote: > I'd rather see the right solution designed here, so if this isn't the > right direction I want to work with the list to figure out what makes > the most sense. (Even if that's "minimum security should update > automatically"). > I want to clarify here based on feedback I received from customer support engineers: We believe it is not acceptable to update this automatically, because not all customers want that behavior and would prefer to have control over when to lock in the minimum security revision. Previous products have behaved this way and we had significant feedback when this occurred that many of our customers were unhappy about this, even after we explained the reasoning. I do not believe that we can accept an automatic/default update of minimum security revision. ---------------------------------------------- Scratch that. I replied to the wrong email. Sorry about that.
On Thu, 4 Feb 2021 13:53:34 -0800 Jacob Keller wrote: > On 2/4/2021 11:10 AM, Jacob Keller wrote: > > I'd rather see the right solution designed here, so if this isn't the > > right direction I want to work with the list to figure out what makes > > the most sense. (Even if that's "minimum security should update > > automatically"). > > > I want to clarify here based on feedback I received from customer > support engineers: We believe it is not acceptable to update this > automatically, because not all customers want that behavior and would > prefer to have control over when to lock in the minimum security revision. > > Previous products have behaved this way and we had significant feedback > when this occurred that many of our customers were unhappy about this, > even after we explained the reasoning. > > I do not believe that we can accept an automatic/default update of > minimum security revision. I spent some time reading through various docs, and my main concern now is introduction of an API which does not have any cryptographic guarantees. An attacker who has infiltrated the OS but did not manage to crack the device yet, can fake the SEV responses and keep the counter from ever being bumped until they successfully expoit the device. Is the min SEV counter included in device measurements? I'm starting to think that distributing separate FW builds with and without auto-SEV bump is the best way to fit into the SecBoot infra, without additional wrinkles and attack vectors. WDYT?
diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst index 78707970ee62..efa366ae633a 100644 --- a/Documentation/networking/devlink/ice.rst +++ b/Documentation/networking/devlink/ice.rst @@ -96,6 +96,40 @@ The ``ice`` driver reports the following versions - 0xee16ced7 - The first 4 bytes of the hash of the netlist module contents. +Parameters +========== + +The minimum security revision fields of the ice device control whether the +associated flash section can be loaded. If the security revision field of +the section -- ``fw.mgmt.srev`` for the main firmware section and +``fw.undi.srev`` for the Option ROM -- is lower than the associated minimum +security revision, then the device will not load that section of firmware. + +The ``ice`` driver implements driver-specific parameters for updating the +minimum security revision fields associated those two sections of the device +flash. Note that the device will not allow lowering a minimum security +revision, nor will it allow increasing the security revision higher than the +associated security revision of the active flash image. + +.. list-table:: Minimum security revision parameters + :widths: 5 5 5 85 + + * - Name + - Type + - Mode + - Description + * - ``fw.undi.minsrev`` + - u32 + - permanent + - The device's minimum security revision for the ``fw.undi`` section of + the flash. + * - ``fw.mgmt.minsrev`` + - u32 + - permanent + - The device's minimum security revision for the ``fw.mgmt`` section of + the flash. + + Flash Update ============ diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h index b06fbe99d8e9..40c96662458a 100644 --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h @@ -1334,6 +1334,8 @@ struct ice_aqc_nvm_checksum { u8 rsvd2[12]; }; +#define ICE_AQC_NVM_MINSREV_MOD_ID 0x130 + /* The result of netlist NVM read comes in a TLV format. The actual data * (netlist header) starts from word offset 1 (byte 2). The FW strips * out the type field from the TLV header so all the netlist fields @@ -1361,6 +1363,21 @@ struct ice_aqc_nvm_checksum { #define ICE_AQC_NVM_NETLIST_ID_BLK_SHA_HASH 0xA #define ICE_AQC_NVM_NETLIST_ID_BLK_CUST_VER 0x2F +/* Used for reading and writing MinSRev using 0x0701 and 0x0703. Note that the + * type field is excluded from the section when reading and writing from + * a module using the module_typeid field with these AQ commands. + */ +struct ice_aqc_nvm_minsrev { + __le16 length; + __le16 validity; +#define ICE_AQC_NVM_MINSREV_NVM_VALID BIT(0) +#define ICE_AQC_NVM_MINSREV_OROM_VALID BIT(1) + __le16 nvm_minsrev_l; + __le16 nvm_minsrev_h; + __le16 orom_minsrev_l; + __le16 orom_minsrev_h; +}; + /* Used for NVM Set Package Data command - 0x070A */ struct ice_aqc_nvm_pkg_data { u8 reserved[3]; diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c index 4b08bf6dd0b0..8af33ef70f76 100644 --- a/drivers/net/ethernet/intel/ice/ice_devlink.c +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c @@ -250,6 +250,193 @@ static int ice_devlink_info_get(struct devlink *devlink, return 0; } +enum ice_devlink_param_id { + ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, + ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV, + ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV, +}; + +/** + * ice_devlink_minsrev_get - Get the current minimum security revision + * @devlink: pointer to the devlink instance + * @id: the parameter ID to get + * @ctx: context to return the parameter value + * + * Returns: zero on success, or an error code on failure. + */ +static int +ice_devlink_minsrev_get(struct devlink *devlink, u32 id, struct devlink_param_gset_ctx *ctx) +{ + struct ice_pf *pf = devlink_priv(devlink); + struct device *dev = ice_pf_to_dev(pf); + struct ice_minsrev_info minsrevs = {}; + enum ice_status status; + + if (id != ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV && + id != ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV) + return -EINVAL; + + status = ice_get_nvm_minsrevs(&pf->hw, &minsrevs); + if (status) { + dev_warn(dev, "Failed to read minimum security revision data from flash\n"); + return -EIO; + } + + /* We report zero if the device has not yet had a valid minimum + * security revision programmed for the associated module. This makes + * sense because it is not possible to have a security revision of + * less than zero. Thus, all images will be able to load if the + * minimum security revision is zero, the same as the case where the + * minimum value is indicated as invalid. + */ + switch (id) { + case ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV: + if (minsrevs.nvm_valid) + ctx->val.vu32 = minsrevs.nvm; + else + ctx->val.vu32 = 0; + break; + case ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV: + if (minsrevs.orom_valid) + ctx->val.vu32 = minsrevs.orom; + else + ctx->val.vu32 = 0; + break; + } + + return 0; +} + +/** + * ice_devlink_minsrev_set - Set the minimum security revision + * @devlink: pointer to the devlink instance + * @id: the parameter ID to set + * @ctx: context to return the parameter value + * + * Set the minimum security revision value for fw.mgmt or fw.undi. The kernel + * calls the validate handler before calling this, so we do not need to + * duplicate those checks here. + * + * Returns: zero on success, or an error code on failure. + */ +static int +ice_devlink_minsrev_set(struct devlink *devlink, u32 id, struct devlink_param_gset_ctx *ctx) +{ + struct ice_pf *pf = devlink_priv(devlink); + struct device *dev = ice_pf_to_dev(pf); + struct ice_minsrev_info minsrevs = {}; + enum ice_status status; + + switch (id) { + case ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV: + minsrevs.nvm_valid = true; + minsrevs.nvm = ctx->val.vu32; + break; + case ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV: + minsrevs.orom_valid = true; + minsrevs.orom = ctx->val.vu32; + break; + default: + return -EINVAL; + } + + status = ice_update_nvm_minsrevs(&pf->hw, &minsrevs); + if (status) { + dev_warn(dev, "Failed to update minimum security revision data\n"); + return -EIO; + } + + return 0; +} + +/** + * ice_devlink_minsrev_validate - Validate a minimum security revision update + * @devlink: unused pointer to devlink instance + * @id: the parameter ID to validate + * @val: value to validate + * @extack: netlink extended ACK structure + * + * Check that a proposed update to a minimum security revision field is valid. + * Each minimum security revision can only be increased, not decreased. + * Additionally, we verify that the value is never set higher than the + * security revision of the active flash component. + * + * Returns: zero if the value is valid, -ERANGE if it is out of range, and + * -EINVAL if this function is called with the wrong ID. + */ +static int +ice_devlink_minsrev_validate(struct devlink *devlink, u32 id, union devlink_param_value val, + struct netlink_ext_ack *extack) +{ + struct ice_pf *pf = devlink_priv(devlink); + struct device *dev = ice_pf_to_dev(pf); + struct ice_minsrev_info minsrevs = {}; + enum ice_status status; + + if (id != ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV && + id != ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV) + return -EINVAL; + + status = ice_get_nvm_minsrevs(&pf->hw, &minsrevs); + if (status) { + NL_SET_ERR_MSG_MOD(extack, "Failed to read minimum security revision data from flash"); + return -EIO; + } + + switch (id) { + case ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV: + if (val.vu32 > pf->hw.flash.nvm.srev) { + NL_SET_ERR_MSG_MOD(extack, "Cannot update fw.mgmt minimum security revision higher than the currently running firmware"); + dev_dbg(dev, "Attempted to set fw.mgmt.minsrev to %u, but running firmware has srev %u\n", + val.vu32, pf->hw.flash.nvm.srev); + return -EPERM; + } + + if (minsrevs.nvm_valid && val.vu32 < minsrevs.nvm) { + NL_SET_ERR_MSG_MOD(extack, "Cannot lower the minimum security revision for fw.mgmt flash section"); + dev_dbg(dev, "Attempted to set fw.mgmt.minsrev to %u, but current minsrev is %u\n", + val.vu32, minsrevs.nvm); + return -EPERM; + } + break; + case ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV: + if (val.vu32 > pf->hw.flash.orom.srev) { + NL_SET_ERR_MSG_MOD(extack, "Cannot update fw.undi minimum security revision higher than the currently running firmware"); + dev_dbg(dev, "Attempted to set fw.undi.minsrev to %u, but running firmware has srev %u\n", + val.vu32, pf->hw.flash.orom.srev); + return -EPERM; + } + + if (minsrevs.orom_valid && val.vu32 < minsrevs.orom) { + NL_SET_ERR_MSG_MOD(extack, "Cannot lower the minimum security revision for fw.undi flash section"); + dev_dbg(dev, "Attempted to set fw.undi.minsrev to %u, but current minsrev is %u\n", + val.vu32, minsrevs.orom); + return -EPERM; + } + break; + } + + return 0; +} + +/* devlink parameters for the ice driver */ +static const struct devlink_param ice_devlink_params[] = { + DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV, + "fw.mgmt.minsrev", + DEVLINK_PARAM_TYPE_U32, + BIT(DEVLINK_PARAM_CMODE_PERMANENT), + ice_devlink_minsrev_get, + ice_devlink_minsrev_set, + ice_devlink_minsrev_validate), + DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV, + "fw.undi.minsrev", + DEVLINK_PARAM_TYPE_U32, + BIT(DEVLINK_PARAM_CMODE_PERMANENT), + ice_devlink_minsrev_get, + ice_devlink_minsrev_set, + ice_devlink_minsrev_validate), +}; + /** * ice_devlink_flash_update - Update firmware stored in flash on the device * @devlink: pointer to devlink associated with device to update @@ -356,6 +543,13 @@ int ice_devlink_register(struct ice_pf *pf) return err; } + err = devlink_params_register(devlink, ice_devlink_params, + ARRAY_SIZE(ice_devlink_params)); + if (err) { + dev_err(dev, "devlink params registration failed: %d\n", err); + return err; + } + return 0; } @@ -367,7 +561,29 @@ int ice_devlink_register(struct ice_pf *pf) */ void ice_devlink_unregister(struct ice_pf *pf) { - devlink_unregister(priv_to_devlink(pf)); + struct devlink *devlink = priv_to_devlink(pf); + + devlink_params_unregister(devlink, ice_devlink_params, + ARRAY_SIZE(ice_devlink_params)); + devlink_unregister(devlink); +} + +/** + * ice_devlink_params_publish - Publish parameters to allow user access. + * @pf: the PF structure pointer + */ +void ice_devlink_params_publish(struct ice_pf *pf) +{ + devlink_params_publish(priv_to_devlink(pf)); +} + +/** + * ice_devlink_params_unpublish - Unpublish parameters to prevent user access. + * @pf: the PF structure pointer + */ +void ice_devlink_params_unpublish(struct ice_pf *pf) +{ + devlink_params_unpublish(priv_to_devlink(pf)); } /** diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.h b/drivers/net/ethernet/intel/ice/ice_devlink.h index e07e74426bde..e3363ea5c7ac 100644 --- a/drivers/net/ethernet/intel/ice/ice_devlink.h +++ b/drivers/net/ethernet/intel/ice/ice_devlink.h @@ -8,6 +8,8 @@ struct ice_pf *ice_allocate_pf(struct device *dev); int ice_devlink_register(struct ice_pf *pf); void ice_devlink_unregister(struct ice_pf *pf); +void ice_devlink_params_publish(struct ice_pf *pf); +void ice_devlink_params_unpublish(struct ice_pf *pf); int ice_devlink_create_port(struct ice_vsi *vsi); void ice_devlink_destroy_port(struct ice_vsi *vsi); diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 6e251dfffc91..66a40dfadb6a 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -4071,6 +4071,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent) } ice_devlink_init_regions(pf); + ice_devlink_params_publish(pf); pf->hw.udp_tunnel_nic.set_port = ice_udp_tunnel_set_port; pf->hw.udp_tunnel_nic.unset_port = ice_udp_tunnel_unset_port; @@ -4259,6 +4260,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent) devm_kfree(dev, pf->vsi); err_init_pf_unroll: ice_deinit_pf(pf); + ice_devlink_params_unpublish(pf); ice_devlink_destroy_regions(pf); ice_deinit_hw(hw); err_exit_unroll: @@ -4371,6 +4373,7 @@ static void ice_remove(struct pci_dev *pdev) ice_vsi_free_q_vectors(pf->vsi[i]); } ice_deinit_pf(pf); + ice_devlink_params_unpublish(pf); ice_devlink_destroy_regions(pf); ice_deinit_hw(&pf->hw); ice_devlink_unregister(pf); diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c index 21eef3d037d6..ed4d6058a90d 100644 --- a/drivers/net/ethernet/intel/ice/ice_nvm.c +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c @@ -1038,6 +1038,118 @@ enum ice_status ice_nvm_write_activate(struct ice_hw *hw, u8 cmd_flags) return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL); } +/** + * ice_get_nvm_minsrevs - Get the Minimum Security Revision values from flash + * @hw: pointer to the HW struct + * @minsrevs: structure to store NVM and OROM minsrev values + * + * Read the Minimum Security Revision TLV and extract the revision values from + * the flash image into a readable structure for processing. + */ +enum ice_status +ice_get_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs) +{ + struct ice_aqc_nvm_minsrev data; + enum ice_status status; + u16 valid; + + status = ice_acquire_nvm(hw, ICE_RES_READ); + if (status) + return status; + + status = ice_aq_read_nvm(hw, ICE_AQC_NVM_MINSREV_MOD_ID, 0, sizeof(data), + &data, true, false, NULL); + + ice_release_nvm(hw); + + if (status) + return status; + + valid = le16_to_cpu(data.validity); + + /* Extract NVM minimum security revision */ + if (valid & ICE_AQC_NVM_MINSREV_NVM_VALID) { + u16 minsrev_l, minsrev_h; + + minsrev_l = le16_to_cpu(data.nvm_minsrev_l); + minsrev_h = le16_to_cpu(data.nvm_minsrev_h); + + minsrevs->nvm = minsrev_h << 16 | minsrev_l; + minsrevs->nvm_valid = true; + } + + /* Extract the OROM minimum security revision */ + if (valid & ICE_AQC_NVM_MINSREV_OROM_VALID) { + u16 minsrev_l, minsrev_h; + + minsrev_l = le16_to_cpu(data.orom_minsrev_l); + minsrev_h = le16_to_cpu(data.orom_minsrev_h); + + minsrevs->orom = minsrev_h << 16 | minsrev_l; + minsrevs->orom_valid = true; + } + + return 0; +} + +/** + * ice_update_nvm_minsrevs - Update minimum security revision TLV data in flash + * @hw: pointer to the HW struct + * @minsrevs: minimum security revision information + * + * Update the NVM or Option ROM minimum security revision fields in the PFA + * area of the flash. Reads the minsrevs->nvm_valid and minsrevs->orom_valid + * fields to determine what update is being requested. If the valid bit is not + * set for that module, then the associated minsrev will be left as is. + */ +enum ice_status +ice_update_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs) +{ + struct ice_aqc_nvm_minsrev data; + enum ice_status status; + + if (!minsrevs->nvm_valid && !minsrevs->orom_valid) { + ice_debug(hw, ICE_DBG_NVM, "At least one of NVM and OROM MinSrev must be valid"); + return ICE_ERR_PARAM; + } + + status = ice_acquire_nvm(hw, ICE_RES_WRITE); + if (status) + return status; + + /* Get current data */ + status = ice_aq_read_nvm(hw, ICE_AQC_NVM_MINSREV_MOD_ID, 0, sizeof(data), + &data, true, false, NULL); + if (status) + goto exit_release_res; + + if (minsrevs->nvm_valid) { + data.nvm_minsrev_l = cpu_to_le16(minsrevs->nvm & 0xFFFF); + data.nvm_minsrev_h = cpu_to_le16(minsrevs->nvm >> 16); + data.validity |= cpu_to_le16(ICE_AQC_NVM_MINSREV_NVM_VALID); + } + + if (minsrevs->orom_valid) { + data.orom_minsrev_l = cpu_to_le16(minsrevs->orom & 0xFFFF); + data.orom_minsrev_h = cpu_to_le16(minsrevs->orom >> 16); + data.validity |= cpu_to_le16(ICE_AQC_NVM_MINSREV_OROM_VALID); + } + + /* Update flash data */ + status = ice_aq_update_nvm(hw, ICE_AQC_NVM_MINSREV_MOD_ID, 0, sizeof(data), &data, + true, ICE_AQC_NVM_SPECIAL_UPDATE, NULL); + if (status) + goto exit_release_res; + + /* Dump the Shadow RAM to the flash */ + status = ice_nvm_write_activate(hw, 0); + +exit_release_res: + ice_release_nvm(hw); + + return status; +} + /** * ice_aq_nvm_update_empr * @hw: pointer to the HW struct diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h index 8d430909f846..8cfb9b9ac638 100644 --- a/drivers/net/ethernet/intel/ice/ice_nvm.h +++ b/drivers/net/ethernet/intel/ice/ice_nvm.h @@ -14,6 +14,10 @@ enum ice_status ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len, u16 module_type); enum ice_status +ice_get_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs); +enum ice_status +ice_update_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs); +enum ice_status ice_read_pba_string(struct ice_hw *hw, u8 *pba_num, u32 pba_num_size); enum ice_status ice_init_nvm(struct ice_hw *hw); enum ice_status ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data); diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index 0e0cbf90c431..f387641195a9 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -322,6 +322,14 @@ struct ice_nvm_info { u8 minor; }; +/* Minimum Security Revision information */ +struct ice_minsrev_info { + u32 nvm; + u32 orom; + u8 nvm_valid : 1; + u8 orom_valid : 1; +}; + /* netlist version information */ struct ice_netlist_info { u32 major; /* major high/low */