Re: [PATCH 6/6] scsi: ufs: exynos: Add support for Flash Memory Protector (FMP)

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Jun 14, 2024 at 06:00:57PM -0500, Sam Protsenko wrote:
> On Tue, Jun 11, 2024 at 5:36 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > From: Eric Biggers <ebiggers@google.com>
> >
> > Add support for Flash Memory Protector (FMP), which is the inline
> > encryption hardware on Exynos and Exynos-based SoCs.
> >
> > Specifically, add support for the "traditional FMP mode" that works on
> > many Exynos-based SoCs including gs101.  This is the mode that uses
> > "software keys" and is compatible with the upstream kernel's existing
> > inline encryption framework in the block and filesystem layers.  I plan
> > to add support for the wrapped key support on gs101 at a later time.
> >
> > Tested on gs101 (specifically Pixel 6) by running the 'encrypt' group of
> > xfstests on a filesystem mounted with the 'inlinecrypt' mount option.
> >
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  drivers/ufs/host/ufs-exynos.c | 219 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 218 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
> > index 88d125d1ee3c..969c4eedbe2d 100644
> > --- a/drivers/ufs/host/ufs-exynos.c
> > +++ b/drivers/ufs/host/ufs-exynos.c
> > @@ -6,10 +6,13 @@
> >   * Author: Seungwon Jeon  <essuuj@gmail.com>
> >   * Author: Alim Akhtar <alim.akhtar@samsung.com>
> >   *
> >   */
> >
> > +#include <asm/unaligned.h>
> > +#include <crypto/aes.h>
> > +#include <linux/arm-smccc.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_address.h>
> > @@ -1149,10 +1152,221 @@ static inline void exynos_ufs_priv_init(struct ufs_hba *hba,
> >                 ufs->rx_sel_idx = 0;
> >         hba->priv = (void *)ufs;
> >         hba->quirks = ufs->drv_data->quirks;
> >  }
> >
> > +#ifdef CONFIG_SCSI_UFS_CRYPTO
> > +
> > +/*
> > + * Support for Flash Memory Protector (FMP), which is the inline encryption
> > + * hardware on Exynos and Exynos-based SoCs.  The interface to this hardware is
> > + * not compatible with the standard UFS crypto.  It requires that encryption be
> > + * configured in the PRDT using a nonstandard extension.
> > + */
> > +
> > +enum fmp_crypto_algo_mode {
> > +       FMP_BYPASS_MODE = 0,
> > +       FMP_ALGO_MODE_AES_CBC = 1,
> > +       FMP_ALGO_MODE_AES_XTS = 2,
> > +};
> > +enum fmp_crypto_key_length {
> > +       FMP_KEYLEN_256BIT = 1,
> > +};
> > +#define FMP_DATA_UNIT_SIZE     SZ_4K
> > +
> > +/* This is the nonstandard format of PRDT entries when FMP is enabled. */
> > +struct fmp_sg_entry {
> > +
> > +       /*
> > +        * This is the standard PRDT entry, but with nonstandard bitfields in
> > +        * the high bits of the 'size' field, i.e. the last 32-bit word.  When
> > +        * these nonstandard bitfields are zero, the data segment won't be
> > +        * encrypted or decrypted.  Otherwise they specify the algorithm and key
> > +        * length with which the data segment will be encrypted or decrypted.
> > +        */
> 
> Minor suggestion: create a kernel-doc comment for the structure and
> pull all fields documentation there.
> 
> > +       struct ufshcd_sg_entry base;
> > +
> > +       /* The initialization vector (IV) with all bytes reversed */
> > +       __be64 file_iv[2];
> > +
> > +       /*
> > +        * The key with all bytes reversed.  For XTS, the two halves of the key
> > +        * are given separately and are byte-reversed separately.
> > +        */
> > +       __be64 file_enckey[4];
> > +       __be64 file_twkey[4];
> > +
> > +       /* Unused */
> > +       __be64 disk_iv[2];
> > +       __be64 reserved[2];
> > +};
> > +
> > +#define SMC_CMD_FMP_SECURITY           0xC2001810
> > +#define SMC_CMD_SMU                    0xC2001850
> > +#define SMC_CMD_FMP_SMU_RESUME         0xC2001860
> 
> Suggest to use ARM_SMCCC_CALL_VAL() macro to define above values.
> 
> > +#define SMU_EMBEDDED                   0
> > +#define SMU_INIT                       0
> > +#define CFG_DESCTYPE_3                 3
> > +
> > +static inline long exynos_smc(unsigned long cmd, unsigned long arg0,
> > +                             unsigned long arg1, unsigned long arg2)
> > +{
> > +       struct arm_smccc_res res;
> > +
> > +       arm_smccc_smc(cmd, arg0, arg1, arg2, 0, 0, 0, 0, &res);
> > +       return res.a0;
> > +}
> 
> This wrapper looks like it was borrowed from the downstream Samsung
> code. Not sure if it brings any value nowadays. Maybe it would be
> clearer to just use arm_smccc_smc() directly and remove this wrapper?
> 

All done in v2.  Thanks.

- Eric