| Message ID | 20191218145136.172774-7-satyat@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Inline Encryption Support | expand |
On Wed, Dec 18, 2019 at 06:51:33AM -0800, Satya Tangirala wrote: > Wire up ufshcd.c with the UFS Crypto API, the block layer inline > encryption additions and the keyslot manager. > > Signed-off-by: Satya Tangirala <satyat@google.com> > --- > drivers/scsi/ufs/ufshcd-crypto.c | 30 ++++++++++++++++++ > drivers/scsi/ufs/ufshcd-crypto.h | 21 +++++++++++++ > drivers/scsi/ufs/ufshcd.c | 54 +++++++++++++++++++++++++++++--- > drivers/scsi/ufs/ufshcd.h | 8 +++++ > 4 files changed, 108 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd-crypto.c b/drivers/scsi/ufs/ufshcd-crypto.c > index b0aa072d9009..749c325686a7 100644 > --- a/drivers/scsi/ufs/ufshcd-crypto.c > +++ b/drivers/scsi/ufs/ufshcd-crypto.c > @@ -352,6 +352,36 @@ void ufshcd_crypto_setup_rq_keyslot_manager(struct ufs_hba *hba, > } > EXPORT_SYMBOL_GPL(ufshcd_crypto_setup_rq_keyslot_manager); > > +int ufshcd_prepare_lrbp_crypto(struct ufs_hba *hba, > + struct scsi_cmnd *cmd, > + struct ufshcd_lrb *lrbp) > +{ > + struct bio_crypt_ctx *bc; > + > + if (!bio_crypt_should_process(cmd->request)) { > + lrbp->crypto_enable = false; > + return 0; > + } > + bc = cmd->request->bio->bi_crypt_context; > + > + if (WARN_ON(!ufshcd_is_crypto_enabled(hba))) { > + /* > + * Upper layer asked us to do inline encryption > + * but that isn't enabled, so we fail this request. > + */ > + return -EINVAL; > + } > + if (!ufshcd_keyslot_valid(hba, bc->bc_keyslot)) > + return -EINVAL; > + > + lrbp->crypto_enable = true; > + lrbp->crypto_key_slot = bc->bc_keyslot; > + lrbp->data_unit_num = bc->bc_dun[0]; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(ufshcd_prepare_lrbp_crypto); The UFS driver only uses the first 64 bits of the DUN, but in this version of the patchset the DUN in the bio_crypt_ctx can be up to the real length of the algorithm's IV -- which for AES-256-XTS is 128 bits. So if the user were to specify anything nonzero in bits 64-127, the crypto would be done incorrectly. (This case isn't encountered with fscrypt. But it's still an issue with the overall approach.) So there needs to be a way for drivers to declare the max_dun_size they support, and prevent them from being used with longer DUNs. - Eric
On Wed, Dec 18, 2019 at 06:51:33AM -0800, Satya Tangirala wrote: > Wire up ufshcd.c with the UFS Crypto API, the block layer inline > encryption additions and the keyslot manager. I think this patch should be merged into the previous patch, as the previous one isn't useful without wiring it up. > +int ufshcd_prepare_lrbp_crypto(struct ufs_hba *hba, > + struct scsi_cmnd *cmd, > + struct ufshcd_lrb *lrbp) > +{ > + struct bio_crypt_ctx *bc; > + > + if (!bio_crypt_should_process(cmd->request)) { > + lrbp->crypto_enable = false; > + return 0; > + } I think this check belongs into the caller so that there is no function call overhead for the !inline crypto case. If you extend the conditional to if (!IS_ENABLED(CONFIG_SCSI_UFS_CRYPTO) || !bio_crypt_should_process(cmd->request)) you also don't need the stub for ufshcd_prepare_lrbp_crypto. > +EXPORT_SYMBOL_GPL(ufshcd_prepare_lrbp_crypto); No need to export this function. > + if (ufshcd_lrbp_crypto_enabled(lrbp)) { > +#if IS_ENABLED(CONFIG_SCSI_UFS_CRYPTO) > + dword_0 |= UTP_REQ_DESC_CRYPTO_ENABLE_CMD; > + dword_0 |= lrbp->crypto_key_slot; > + req_desc->header.dword_1 = > + cpu_to_le32(lower_32_bits(lrbp->data_unit_num)); > + req_desc->header.dword_3 = > + cpu_to_le32(upper_32_bits(lrbp->data_unit_num)); > +#endif /* CONFIG_SCSI_UFS_CRYPTO */ This can be a plain old ifdef without the IS_ENABLED obsfucation. > +#if IS_ENABLED(CONFIG_SCSI_UFS_CRYPTO) > + lrbp->crypto_enable = false; /* No crypto operations */ > +#endif Same here. > +#if IS_ENABLED(CONFIG_SCSI_UFS_CRYPTO) > + bool crypto_enable; > + u8 crypto_key_slot; > + u64 data_unit_num; > +#endif /* CONFIG_SCSI_UFS_CRYPTO */ .. and here.
On Wed, Dec 18, 2019 at 06:51:33AM -0800, Satya Tangirala wrote: > @@ -4654,6 +4686,8 @@ static int ufshcd_slave_configure(struct scsi_device *sdev) > if (ufshcd_is_rpm_autosuspend_allowed(hba)) > sdev->rpm_autosuspend = 1; > > + ufshcd_crypto_setup_rq_keyslot_manager(hba, q); > + > return 0; > } > > @@ -4664,6 +4698,7 @@ static int ufshcd_slave_configure(struct scsi_device *sdev) > static void ufshcd_slave_destroy(struct scsi_device *sdev) > { > struct ufs_hba *hba; > + struct request_queue *q = sdev->request_queue; > > hba = shost_priv(sdev->host); > /* Drop the reference as it won't be needed anymore */ > @@ -4674,6 +4709,8 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev) > hba->sdev_ufs_device = NULL; > spin_unlock_irqrestore(hba->host->host_lock, flags); > } > + > + ufshcd_crypto_destroy_rq_keyslot_manager(hba, q); > } Just noticed this --- this is still destroying the keyslot manager when a SCSI device is destroyed. The keyslot manager is associated with the host controller (which might control multiple devices), so it must not be destroyed until the ufs_hba is destroyed, i.e. in ufshcd_dealloc_host(). (I was also thinking about whether we could use devm so that the keyslot_manager doesn't need to be explicitly freed. But that wouldn't actually help because we still need to ensure that all the crypto keys get zeroed.) - Eric
On Fri, Jan 17, 2020 at 05:58:08AM -0800, Christoph Hellwig wrote: > On Wed, Dec 18, 2019 at 06:51:33AM -0800, Satya Tangirala wrote: > > Wire up ufshcd.c with the UFS Crypto API, the block layer inline > > encryption additions and the keyslot manager. > > I think this patch should be merged into the previous patch, as the > previous one isn't useful without wiring it up. > Satya actually did this originally but then one of the UFS maintainers requested the separate patches for (1) new registers, (2) ufshcd-crypto, and (3) ufshcd.c: https://lore.kernel.org/linux-block/SN6PR04MB49259F70346E2055C9E0F401FC310@SN6PR04MB4925.namprd04.prod.outlook.com/ So, he's not going to be able to make everyone happy :-) I personally would be fine with either way. - Eric
On Fri, Jan 17, 2020 at 09:27:20PM -0800, Eric Biggers wrote: > On Fri, Jan 17, 2020 at 05:58:08AM -0800, Christoph Hellwig wrote: > > On Wed, Dec 18, 2019 at 06:51:33AM -0800, Satya Tangirala wrote: > > > Wire up ufshcd.c with the UFS Crypto API, the block layer inline > > > encryption additions and the keyslot manager. > > > > I think this patch should be merged into the previous patch, as the > > previous one isn't useful without wiring it up. > > > > Satya actually did this originally but then one of the UFS maintainers requested > the separate patches for (1) new registers, (2) ufshcd-crypto, and (3) ufshcd.c: > https://lore.kernel.org/linux-block/SN6PR04MB49259F70346E2055C9E0F401FC310@SN6PR04MB4925.namprd04.prod.outlook.com/ > > So, he's not going to be able to make everyone happy :-) > > I personally would be fine with either way. Oh well, the split between adding functions and callers is highly unusual for Linux development. Adding the defines I can see, especially if they are large (which these aren't). But you'll need to get this accepted by the UFS folks, so I'll shut up now.
On Wed, Dec 18, 2019 at 06:51:33AM -0800, Satya Tangirala wrote: > @@ -2472,6 +2492,13 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) > lrbp->task_tag = tag; > lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun); > lrbp->intr_cmd = !ufshcd_is_intr_aggr_allowed(hba) ? true : false; > + > + err = ufshcd_prepare_lrbp_crypto(hba, cmd, lrbp); > + if (err) { > + lrbp->cmd = NULL; > + clear_bit_unlock(tag, &hba->lrb_in_use); > + goto out; > + } The error path here is missing a call to ufshcd_release(). - Eric
diff --git a/drivers/scsi/ufs/ufshcd-crypto.c b/drivers/scsi/ufs/ufshcd-crypto.c index b0aa072d9009..749c325686a7 100644 --- a/drivers/scsi/ufs/ufshcd-crypto.c +++ b/drivers/scsi/ufs/ufshcd-crypto.c @@ -352,6 +352,36 @@ void ufshcd_crypto_setup_rq_keyslot_manager(struct ufs_hba *hba, } EXPORT_SYMBOL_GPL(ufshcd_crypto_setup_rq_keyslot_manager); +int ufshcd_prepare_lrbp_crypto(struct ufs_hba *hba, + struct scsi_cmnd *cmd, + struct ufshcd_lrb *lrbp) +{ + struct bio_crypt_ctx *bc; + + if (!bio_crypt_should_process(cmd->request)) { + lrbp->crypto_enable = false; + return 0; + } + bc = cmd->request->bio->bi_crypt_context; + + if (WARN_ON(!ufshcd_is_crypto_enabled(hba))) { + /* + * Upper layer asked us to do inline encryption + * but that isn't enabled, so we fail this request. + */ + return -EINVAL; + } + if (!ufshcd_keyslot_valid(hba, bc->bc_keyslot)) + return -EINVAL; + + lrbp->crypto_enable = true; + lrbp->crypto_key_slot = bc->bc_keyslot; + lrbp->data_unit_num = bc->bc_dun[0]; + + return 0; +} +EXPORT_SYMBOL_GPL(ufshcd_prepare_lrbp_crypto); + void ufshcd_crypto_destroy_rq_keyslot_manager(struct ufs_hba *hba, struct request_queue *q) { diff --git a/drivers/scsi/ufs/ufshcd-crypto.h b/drivers/scsi/ufs/ufshcd-crypto.h index 61996c6520e9..7f6596d07fd3 100644 --- a/drivers/scsi/ufs/ufshcd-crypto.h +++ b/drivers/scsi/ufs/ufshcd-crypto.h @@ -36,6 +36,15 @@ static inline bool ufshcd_is_crypto_enabled(struct ufs_hba *hba) } /* Functions implementing UFSHCI v2.1 specification behaviour */ +int ufshcd_prepare_lrbp_crypto(struct ufs_hba *hba, + struct scsi_cmnd *cmd, + struct ufshcd_lrb *lrbp); + +static inline bool ufshcd_lrbp_crypto_enabled(struct ufshcd_lrb *lrbp) +{ + return lrbp->crypto_enable; +} + void ufshcd_crypto_enable(struct ufs_hba *hba); void ufshcd_crypto_disable(struct ufs_hba *hba); @@ -81,6 +90,18 @@ static inline void ufshcd_crypto_setup_rq_keyslot_manager(struct ufs_hba *hba, static inline void ufshcd_crypto_destroy_rq_keyslot_manager(struct ufs_hba *hba, struct request_queue *q) { } +static inline int ufshcd_prepare_lrbp_crypto(struct ufs_hba *hba, + struct scsi_cmnd *cmd, + struct ufshcd_lrb *lrbp) +{ + return 0; +} + +static inline bool ufshcd_lrbp_crypto_enabled(struct ufshcd_lrb *lrbp) +{ + return false; +} + #endif /* CONFIG_SCSI_UFS_CRYPTO */ #endif /* _UFSHCD_CRYPTO_H */ diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 809502c66e25..d32202f03d95 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -47,6 +47,7 @@ #include "unipro.h" #include "ufs-sysfs.h" #include "ufs_bsg.h" +#include "ufshcd-crypto.h" #define CREATE_TRACE_POINTS #include <trace/events/ufs.h> @@ -860,7 +861,14 @@ static void ufshcd_enable_run_stop_reg(struct ufs_hba *hba) */ static inline void ufshcd_hba_start(struct ufs_hba *hba) { - ufshcd_writel(hba, CONTROLLER_ENABLE, REG_CONTROLLER_ENABLE); + u32 val = CONTROLLER_ENABLE; + + if (ufshcd_hba_is_crypto_supported(hba)) { + ufshcd_crypto_enable(hba); + val |= CRYPTO_GENERAL_ENABLE; + } + + ufshcd_writel(hba, val, REG_CONTROLLER_ENABLE); } /** @@ -2214,9 +2222,23 @@ static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp, dword_0 |= UTP_REQ_DESC_INT_CMD; /* Transfer request descriptor header fields */ + if (ufshcd_lrbp_crypto_enabled(lrbp)) { +#if IS_ENABLED(CONFIG_SCSI_UFS_CRYPTO) + dword_0 |= UTP_REQ_DESC_CRYPTO_ENABLE_CMD; + dword_0 |= lrbp->crypto_key_slot; + req_desc->header.dword_1 = + cpu_to_le32(lower_32_bits(lrbp->data_unit_num)); + req_desc->header.dword_3 = + cpu_to_le32(upper_32_bits(lrbp->data_unit_num)); +#endif /* CONFIG_SCSI_UFS_CRYPTO */ + } else { + /* dword_1 and dword_3 are reserved, hence they are set to 0 */ + req_desc->header.dword_1 = 0; + req_desc->header.dword_3 = 0; + } + req_desc->header.dword_0 = cpu_to_le32(dword_0); - /* dword_1 is reserved, hence it is set to 0 */ - req_desc->header.dword_1 = 0; + /* * assigning invalid value for command status. Controller * updates OCS on command completion, with the command @@ -2224,8 +2246,6 @@ static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp, */ req_desc->header.dword_2 = cpu_to_le32(OCS_INVALID_COMMAND_STATUS); - /* dword_3 is reserved, hence it is set to 0 */ - req_desc->header.dword_3 = 0; req_desc->prd_table_length = 0; } @@ -2472,6 +2492,13 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) lrbp->task_tag = tag; lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun); lrbp->intr_cmd = !ufshcd_is_intr_aggr_allowed(hba) ? true : false; + + err = ufshcd_prepare_lrbp_crypto(hba, cmd, lrbp); + if (err) { + lrbp->cmd = NULL; + clear_bit_unlock(tag, &hba->lrb_in_use); + goto out; + } lrbp->req_abort_skip = false; ufshcd_comp_scsi_upiu(hba, lrbp); @@ -2505,6 +2532,9 @@ static int ufshcd_compose_dev_cmd(struct ufs_hba *hba, lrbp->task_tag = tag; lrbp->lun = 0; /* device management cmd is not specific to any LUN */ lrbp->intr_cmd = true; /* No interrupt aggregation */ +#if IS_ENABLED(CONFIG_SCSI_UFS_CRYPTO) + lrbp->crypto_enable = false; /* No crypto operations */ +#endif hba->dev_cmd.type = cmd_type; return ufshcd_comp_devman_upiu(hba, lrbp); @@ -4244,6 +4274,8 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep) { int err; + ufshcd_crypto_disable(hba); + ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE); err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE, CONTROLLER_ENABLE, CONTROLLER_DISABLE, @@ -4654,6 +4686,8 @@ static int ufshcd_slave_configure(struct scsi_device *sdev) if (ufshcd_is_rpm_autosuspend_allowed(hba)) sdev->rpm_autosuspend = 1; + ufshcd_crypto_setup_rq_keyslot_manager(hba, q); + return 0; } @@ -4664,6 +4698,7 @@ static int ufshcd_slave_configure(struct scsi_device *sdev) static void ufshcd_slave_destroy(struct scsi_device *sdev) { struct ufs_hba *hba; + struct request_queue *q = sdev->request_queue; hba = shost_priv(sdev->host); /* Drop the reference as it won't be needed anymore */ @@ -4674,6 +4709,8 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev) hba->sdev_ufs_device = NULL; spin_unlock_irqrestore(hba->host->host_lock, flags); } + + ufshcd_crypto_destroy_rq_keyslot_manager(hba, q); } /** @@ -8454,6 +8491,13 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) /* Reset the attached device */ ufshcd_vops_device_reset(hba); + /* Init crypto */ + err = ufshcd_hba_init_crypto(hba); + if (err) { + dev_err(hba->dev, "crypto setup failed\n"); + goto out_remove_scsi_host; + } + /* Host controller enable */ err = ufshcd_hba_enable(hba); if (err) { diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 6c8e2d04e9f8..5f5440059dd8 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -167,6 +167,9 @@ struct ufs_pm_lvl_states { * @intr_cmd: Interrupt command (doesn't participate in interrupt aggregation) * @issue_time_stamp: time stamp for debug purposes * @compl_time_stamp: time stamp for statistics + * @crypto_enable: whether or not the request needs inline crypto operations + * @crypto_key_slot: the key slot to use for inline crypto + * @data_unit_num: the data unit number for the first block for inline crypto * @req_abort_skip: skip request abort task flag */ struct ufshcd_lrb { @@ -191,6 +194,11 @@ struct ufshcd_lrb { bool intr_cmd; ktime_t issue_time_stamp; ktime_t compl_time_stamp; +#if IS_ENABLED(CONFIG_SCSI_UFS_CRYPTO) + bool crypto_enable; + u8 crypto_key_slot; + u64 data_unit_num; +#endif /* CONFIG_SCSI_UFS_CRYPTO */ bool req_abort_skip; };
Wire up ufshcd.c with the UFS Crypto API, the block layer inline encryption additions and the keyslot manager. Signed-off-by: Satya Tangirala <satyat@google.com> --- drivers/scsi/ufs/ufshcd-crypto.c | 30 ++++++++++++++++++ drivers/scsi/ufs/ufshcd-crypto.h | 21 +++++++++++++ drivers/scsi/ufs/ufshcd.c | 54 +++++++++++++++++++++++++++++--- drivers/scsi/ufs/ufshcd.h | 8 +++++ 4 files changed, 108 insertions(+), 5 deletions(-)