| Message ID | 20200814130426.2741171-1-sreekanth.reddy@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] mpt3sas: Add support for Non-secure Aero and Sea PCI IDs | expand |
Sreekanth, > This patch will add support for Non-secure Aero & Sea adapters' PCI > IDs. That wording is misleading since you're effectively removing support for these controllers. > Driver will throw an warning message when a non-secure type controller > is detected. Purpose of this interface is to avoid interacting with > any firmware which is not secured/signed by Broadcom. The request was to log a warning. Why would we want to disable support for these devices in the driver?
On Fri, Aug 21, 2020 at 7:57 AM Martin K. Petersen <martin.petersen@oracle.com> wrote: > > > Sreekanth, > > > This patch will add support for Non-secure Aero & Sea adapters' PCI > > IDs. > > That wording is misleading since you're effectively removing support for > these controllers. Actually we want the driver to claim the non-secure (i.e. invalid & tempered) Aero & Sea devices and print the error message to the user that he is using invalid/tempered Devices. So that user can change to 'Hard Secure'/'config secure' cards. We will rephrase above line as below, "This patch will claim non-secure Aero & Sea adapter's PCI IDs & quit the device initialization (without enabling the HBA firmware) with a Error message" > > > Driver will throw an warning message when a non-secure type controller > > is detected. Purpose of this interface is to avoid interacting with > > any firmware which is not secured/signed by Broadcom. > > The request was to log a warning. Why would we want to disable support > for these devices in the driver? Actually the request is to log an error message but I was confused with your previous comment and made changes to log a warning message. As explained in description the purpose of disabling support for these devices in the driver is to avoid interacting with any firmware which is not secured/signed by Broadcom. Thanks, Sreekanth > > -- > Martin K. Petersen Oracle Linux Engineering
Sreekanth, > As explained in description the purpose of disabling support for these > devices in the driver is to avoid interacting with any firmware which > is not secured/signed by Broadcom. I understand, but that should be a user decision. What are these devices you want to disable support for? Why is their firmware not signed?
On Tue, Aug 25, 2020 at 7:45 AM Martin K. Petersen <martin.petersen@oracle.com> wrote: > > > Sreekanth, > > > As explained in description the purpose of disabling support for these > > devices in the driver is to avoid interacting with any firmware which > > is not secured/signed by Broadcom. > > I understand, but that should be a user decision. > > What are these devices you want to disable support for? Why is their > firmware not signed? The scenario that we are discussing here is a scenario where the device is showing evidence that someone has attempted to physically tamper with the device and has attempted to put it into a state where security could be compromised. Broadcom adapters participate in a Secure Boot process, where every piece of FW is digitally signed by Broadcom and is checked for a valid signature. If any piece of our adapter FW fails this signature check, it is possible the FW has been tampered with and the adapter should not be used. Our driver should not make any additional access to the “invalid/tampered” adapter because the FW is not valid (could be malicious FW). This type of detection is added into latest Aero and Sea family adapters h/w. Thanks, Sreekanth > > -- > Martin K. Petersen Oracle Linux Engineering
Hi Martin, Please let us know if any further information is needed for acceptance of this patch. Thanks, Sreekanth On Wed, Aug 26, 2020 at 9:53 PM Sreekanth Reddy <sreekanth.reddy@broadcom.com> wrote: > > On Tue, Aug 25, 2020 at 7:45 AM Martin K. Petersen > <martin.petersen@oracle.com> wrote: > > > > > > Sreekanth, > > > > > As explained in description the purpose of disabling support for these > > > devices in the driver is to avoid interacting with any firmware which > > > is not secured/signed by Broadcom. > > > > I understand, but that should be a user decision. > > > > What are these devices you want to disable support for? Why is their > > firmware not signed? > > The scenario that we are discussing here is a scenario where the > device is showing evidence that someone has attempted to physically > tamper with the device and has attempted to put it into a state where > security could be compromised. > > Broadcom adapters participate in a Secure Boot process, where every > piece of FW is digitally signed by Broadcom and is checked for a valid > signature. If any piece of our adapter FW fails this signature check, > it is possible the FW has been tampered with and the adapter should > not be used. Our driver should not make any additional access to the > “invalid/tampered” adapter because the FW is not valid (could be > malicious FW). This type of detection is added into latest Aero and > Sea family adapters h/w. > > Thanks, > Sreekanth > > > > > > -- > > Martin K. Petersen Oracle Linux Engineering
Sreekanth, > Broadcom adapters participate in a Secure Boot process, where every > piece of FW is digitally signed by Broadcom and is checked for a valid > signature. If any piece of our adapter FW fails this signature check, > it is possible the FW has been tampered with and the adapter should > not be used. Our driver should not make any additional access to the > “invalid/tampered” adapter because the FW is not valid (could be > malicious FW). This type of detection is added into latest Aero and > Sea family adapters h/w. While I appreciate the intent, I would still like there to be an option to permit using the adapter. I am concerned about users being unable to boot their system due to this if, for whatever reason, these validation checks fail. Maybe there is limited risk of that happening since this is restricted to Aero and Sea adapters. But I am still concerned about enforcing policy decisions like this in the kernel.
On Wed, Sep 9, 2020 at 8:32 AM Martin K. Petersen <martin.petersen@oracle.com> wrote: > > > Sreekanth, > > > Broadcom adapters participate in a Secure Boot process, where every > > piece of FW is digitally signed by Broadcom and is checked for a valid > > signature. If any piece of our adapter FW fails this signature check, > > it is possible the FW has been tampered with and the adapter should > > not be used. Our driver should not make any additional access to the > > “invalid/tampered” adapter because the FW is not valid (could be > > malicious FW). This type of detection is added into latest Aero and > > Sea family adapters h/w. > > While I appreciate the intent, I would still like there to be an option > to permit using the adapter. I am concerned about users being unable to > boot their system due to this if, for whatever reason, these validation > checks fail. Maybe there is limited risk of that happening since this is > restricted to Aero and Sea adapters. But I am still concerned about > enforcing policy decisions like this in the kernel. These non-secure PCI Ids are very unlikely to happen as they are not actual IDs instead they get exposed when something wrong happens in the controller and it is good to prevent boot instead of giving an ability for an user to run malicious code. Also, This is an extremely unlikely event, and is evidence of physical tampering at the ASIC level. - If the executing firmware is authentic, signed Broadcom firmware, it will halt due to the evidence of physical tampering and you won't have a working controller anyway. - If the executing firmware continues to run, then that means that the physical attack has somehow succeeded and allowed the firmware to bypass some part of the signature check, since authentic firmware would have halted. In this case, the driver should reject the controller/firmware combination Thanks, Sreekanth > > -- > Martin K. Petersen Oracle Linux Engineering
Sreekanth, > If the executing firmware continues to run, then that means that the > physical attack has somehow succeeded and allowed the firmware to > bypass some part of the signature check, since authentic firmware > would have halted. In this case, the driver should reject the > controller/firmware combination Applied to 5.10/scsi-staging, thanks!
On Fri, 14 Aug 2020 13:04:26 +0000, Sreekanth Reddy wrote: > This patch will add support for Non-secure Aero & Sea adapters' PCI IDs. > Driver will throw an warning message when a non-secure type controller > is detected. Purpose of this interface is to avoid interacting with > any firmware which is not secured/signed by Broadcom. Any tampering on > Firmware component will be detected by hardware and it will be > communicated to the driver to avoid any further interaction with > that component. Applied to 5.10/scsi-queue, thanks! [1/1] scsi: mpt3sas: Detect tampered Aero and Sea adapters https://git.kernel.org/mkp/scsi/c/f38c43a0e900
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c index 67270e0..cb76b7c 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c @@ -10092,6 +10092,34 @@ void mpt3sas_scsih_pre_reset_handler(struct MPT3SAS_ADAPTER *ioc) } /** + * _scsih_get_shost_and_ioc - get shost and ioc + * and verify whether they are NULL or not + * @pdev: PCI device struct + * @shost: address of scsi host pointer + * @ioc: address of HBA adapter pointer + * + * Return zero if *shost and *ioc are not NULL otherwise return error number. + */ +static int +_scsih_get_shost_and_ioc(struct pci_dev *pdev, + struct Scsi_Host **shost, struct MPT3SAS_ADAPTER **ioc) +{ + *shost = pci_get_drvdata(pdev); + if (*shost == NULL) { + dev_err(&pdev->dev, "pdev's driver data is null\n"); + return -ENXIO; + } + + *ioc = shost_priv(*shost); + if (*ioc == NULL) { + dev_err(&pdev->dev, "shost's private data is null\n"); + return -ENXIO; + } + + return 0; +} + +/** * scsih_remove - detach and remove add host * @pdev: PCI device struct * @@ -10099,8 +10127,8 @@ void mpt3sas_scsih_pre_reset_handler(struct MPT3SAS_ADAPTER *ioc) */ static void scsih_remove(struct pci_dev *pdev) { - struct Scsi_Host *shost = pci_get_drvdata(pdev); - struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); + struct Scsi_Host *shost; + struct MPT3SAS_ADAPTER *ioc; struct _sas_port *mpt3sas_port, *next_port; struct _raid_device *raid_device, *next; struct MPT3SAS_TARGET *sas_target_priv_data; @@ -10109,6 +10137,9 @@ static void scsih_remove(struct pci_dev *pdev) unsigned long flags; Mpi2ConfigReply_t mpi_reply; + if (_scsih_get_shost_and_ioc(pdev, &shost, &ioc)) + return; + ioc->remove_host = 1; if (!pci_device_is_present(pdev)) @@ -10188,12 +10219,15 @@ static void scsih_remove(struct pci_dev *pdev) static void scsih_shutdown(struct pci_dev *pdev) { - struct Scsi_Host *shost = pci_get_drvdata(pdev); - struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); + struct Scsi_Host *shost; + struct MPT3SAS_ADAPTER *ioc; struct workqueue_struct *wq; unsigned long flags; Mpi2ConfigReply_t mpi_reply; + if (_scsih_get_shost_and_ioc(pdev, &shost, &ioc)) + return; + ioc->remove_host = 1; if (!pci_device_is_present(pdev)) @@ -10763,6 +10797,10 @@ static void pcie_device_make_active(struct MPT3SAS_ADAPTER *ioc, case MPI26_MFGPAGE_DEVID_HARD_SEC_3916: case MPI26_MFGPAGE_DEVID_CFG_SEC_3816: case MPI26_MFGPAGE_DEVID_HARD_SEC_3816: + case MPI26_MFGPAGE_DEVID_INVALID0_3916: + case MPI26_MFGPAGE_DEVID_INVALID1_3916: + case MPI26_MFGPAGE_DEVID_INVALID0_3816: + case MPI26_MFGPAGE_DEVID_INVALID1_3816: return MPI26_VERSION; } return 0; @@ -10852,6 +10890,20 @@ static void pcie_device_make_active(struct MPT3SAS_ADAPTER *ioc, case MPI26_ATLAS_PCIe_SWITCH_DEVID: ioc->is_gen35_ioc = 1; break; + case MPI26_MFGPAGE_DEVID_INVALID0_3816: + case MPI26_MFGPAGE_DEVID_INVALID0_3916: + dev_warn(&pdev->dev, + "HBA with DeviceId 0x%04x, sub VendorId 0x%04x, sub DeviceId 0x%04x is Invalid", + pdev->device, pdev->subsystem_vendor, + pdev->subsystem_device); + return 1; + case MPI26_MFGPAGE_DEVID_INVALID1_3816: + case MPI26_MFGPAGE_DEVID_INVALID1_3916: + dev_warn(&pdev->dev, + "HBA with DeviceId 0x%04x, sub VendorId 0x%04x, sub DeviceId 0x%04x is Tampered", + pdev->device, pdev->subsystem_vendor, + pdev->subsystem_device); + return 1; case MPI26_MFGPAGE_DEVID_CFG_SEC_3816: case MPI26_MFGPAGE_DEVID_CFG_SEC_3916: dev_info(&pdev->dev, @@ -11043,9 +11095,14 @@ static void pcie_device_make_active(struct MPT3SAS_ADAPTER *ioc, static int scsih_suspend(struct pci_dev *pdev, pm_message_t state) { - struct Scsi_Host *shost = pci_get_drvdata(pdev); - struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); + struct Scsi_Host *shost; + struct MPT3SAS_ADAPTER *ioc; pci_power_t device_state; + int rc; + + rc = _scsih_get_shost_and_ioc(pdev, &shost, &ioc); + if (rc) + return rc; mpt3sas_base_stop_watchdog(ioc); flush_scheduled_work(); @@ -11070,11 +11127,15 @@ static void pcie_device_make_active(struct MPT3SAS_ADAPTER *ioc, static int scsih_resume(struct pci_dev *pdev) { - struct Scsi_Host *shost = pci_get_drvdata(pdev); - struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); + struct Scsi_Host *shost; + struct MPT3SAS_ADAPTER *ioc; pci_power_t device_state = pdev->current_state; int r; + r = _scsih_get_shost_and_ioc(pdev, &shost, &ioc); + if (r) + return r; + ioc_info(ioc, "pdev=0x%p, slot=%s, previous operating state [D%d]\n", pdev, pci_name(pdev), device_state); @@ -11105,8 +11166,11 @@ static void pcie_device_make_active(struct MPT3SAS_ADAPTER *ioc, static pci_ers_result_t scsih_pci_error_detected(struct pci_dev *pdev, pci_channel_state_t state) { - struct Scsi_Host *shost = pci_get_drvdata(pdev); - struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); + struct Scsi_Host *shost; + struct MPT3SAS_ADAPTER *ioc; + + if (_scsih_get_shost_and_ioc(pdev, &shost, &ioc)) + return PCI_ERS_RESULT_DISCONNECT; ioc_info(ioc, "PCI error: detected callback, state(%d)!!\n", state); @@ -11141,10 +11205,13 @@ static void pcie_device_make_active(struct MPT3SAS_ADAPTER *ioc, static pci_ers_result_t scsih_pci_slot_reset(struct pci_dev *pdev) { - struct Scsi_Host *shost = pci_get_drvdata(pdev); - struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); + struct Scsi_Host *shost; + struct MPT3SAS_ADAPTER *ioc; int rc; + if (_scsih_get_shost_and_ioc(pdev, &shost, &ioc)) + return PCI_ERS_RESULT_DISCONNECT; + ioc_info(ioc, "PCI error: slot reset callback!!\n"); ioc->pci_error_recovery = 0; @@ -11177,8 +11244,11 @@ static void pcie_device_make_active(struct MPT3SAS_ADAPTER *ioc, static void scsih_pci_resume(struct pci_dev *pdev) { - struct Scsi_Host *shost = pci_get_drvdata(pdev); - struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); + struct Scsi_Host *shost; + struct MPT3SAS_ADAPTER *ioc; + + if (_scsih_get_shost_and_ioc(pdev, &shost, &ioc)) + return; ioc_info(ioc, "PCI error: resume callback!!\n"); @@ -11193,8 +11263,11 @@ static void pcie_device_make_active(struct MPT3SAS_ADAPTER *ioc, static pci_ers_result_t scsih_pci_mmio_enabled(struct pci_dev *pdev) { - struct Scsi_Host *shost = pci_get_drvdata(pdev); - struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); + struct Scsi_Host *shost; + struct MPT3SAS_ADAPTER *ioc; + + if (_scsih_get_shost_and_ioc(pdev, &shost, &ioc)) + return PCI_ERS_RESULT_DISCONNECT; ioc_info(ioc, "PCI error: mmio enabled callback!!\n"); @@ -11342,6 +11415,14 @@ bool scsih_ncq_prio_supp(struct scsi_device *sdev) { MPI2_MFGPAGE_VENDORID_LSI, MPI26_MFGPAGE_DEVID_HARD_SEC_3916, PCI_ANY_ID, PCI_ANY_ID }, + /* + * Aero SI –> 0x00E0 Invalid, 0x00E3 Tampered + */ + { MPI2_MFGPAGE_VENDORID_LSI, MPI26_MFGPAGE_DEVID_INVALID0_3916, + PCI_ANY_ID, PCI_ANY_ID }, + { MPI2_MFGPAGE_VENDORID_LSI, MPI26_MFGPAGE_DEVID_INVALID1_3916, + PCI_ANY_ID, PCI_ANY_ID }, + /* Atlas PCIe Switch Management Port */ { MPI2_MFGPAGE_VENDORID_LSI, MPI26_ATLAS_PCIe_SWITCH_DEVID, PCI_ANY_ID, PCI_ANY_ID }, @@ -11354,6 +11435,14 @@ bool scsih_ncq_prio_supp(struct scsi_device *sdev) { MPI2_MFGPAGE_VENDORID_LSI, MPI26_MFGPAGE_DEVID_HARD_SEC_3816, PCI_ANY_ID, PCI_ANY_ID }, + /* + * Sea SI –> 0x00E4 Invalid, 0x00E7 Tampered + */ + { MPI2_MFGPAGE_VENDORID_LSI, MPI26_MFGPAGE_DEVID_INVALID0_3816, + PCI_ANY_ID, PCI_ANY_ID }, + { MPI2_MFGPAGE_VENDORID_LSI, MPI26_MFGPAGE_DEVID_INVALID1_3816, + PCI_ANY_ID, PCI_ANY_ID }, + {0} /* Terminating entry */ }; MODULE_DEVICE_TABLE(pci, mpt3sas_pci_table);
This patch will add support for Non-secure Aero & Sea adapters' PCI IDs. Driver will throw an warning message when a non-secure type controller is detected. Purpose of this interface is to avoid interacting with any firmware which is not secured/signed by Broadcom. Any tampering on Firmware component will be detected by hardware and it will be communicated to the driver to avoid any further interaction with that component. Signed-off-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com> --- v1: Print warning message instead of error message. drivers/scsi/mpt3sas/mpt3sas_scsih.c | 121 ++++++++++++++++++++++++++++++----- 1 file changed, 105 insertions(+), 16 deletions(-)