Re: [PATCH v7 2/2] crypto: s5p-sss: Add HASH support for Exynos
Hi Kamil,
I'll just answer to your question, all the comments from you are accepted,
please send a new version with the minor fixes, hopefully the change will
be included into v4.15-rc.
On 10/24/2017 02:27 PM, Kamil Konieczny wrote:
> Hi Vladimir,
>
> Thank you for review, I will apply almost all of your remarks,
> see answers below.
>
> On 22.10.2017 12:18, Vladimir Zapolskiy wrote:
>> Hi Kamil,
>>
>> thank you for updates, I have just a few more comments.
>>
[snip]
>>> +/**
>>> + * s5p_hash_import - import hash state
>>> + * @req: AHASH request
>>> + * @in:buffer with state to be imported from
>>> + */
>>> +static int s5p_hash_import(struct ahash_request *req, const void *in)
>>> +{
>>> + struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
>>> + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
>>> + struct s5p_hash_ctx *tctx = crypto_ahash_ctx(tfm);
>>> + const struct s5p_hash_reqctx *ctx_in = in;
>>> +
>>> + memcpy(ctx, in, sizeof(*ctx) + BUFLEN);
>>> + if (ctx_in->bufcnt < 0 || ctx_in->bufcnt > BUFLEN) {
>>
>> Now "ctx_in->bufcnt < 0" condition can be removed, moreover it
>> will eliminate a warning reproted by the compiler:
>>
>> drivers/crypto/s5p-sss.c:1748:21: warning: comparison of unsigned expression
>> < 0 is always false [-Wtype-limits]
>> if (ctx_in->bufcnt < 0 || ctx_in->bufcnt > BUFLEN) {
>> ^
>
> You are right, I will drop first condition. BTW what compiler options are you
> using ?
> This one was not reported by my compilation process.
>
Regularly I specify 'make C=1 W=1' options to build a kernel with changes,
some of the new reported warnings are false positives, but in general it
makes sense to check the output to catch potential issues like this one.
--
With best wishes,
Vladimir
Re: [PATCH v7 1/2] crypto: s5p-sss: change spaces into tabs in defines
Hi Kamil, On 10/24/2017 01:19 PM, Kamil Konieczny wrote: > Hi Vladimir, > > Thank you for review. > > On 22.10.2017 12:18, Vladimir Zapolskiy wrote: >> Hi Kamil, >> >> On 10/17/2017 02:28 PM, Kamil Konieczny wrote: >>> change spaces into tabs in defines >> >> Here a grammatically correct sentence in English is welcome. > > What about: "Change spaces to tabs" ? > yes, it'll be fine, sorry if I was excessively pedantic. -- With best wishes, Vladimir
Re: [PATCH] tpm: Move Linux RNG connection to hwrng
On Tue, Oct 24, 2017 at 03:34:49PM -0600, Jason Gunthorpe wrote: > On Tue, Oct 24, 2017 at 12:42:35PM -0600, Jason Gunthorpe wrote: > > > This is compile tested only. > > 0day says the kconfig has a problem when randomized, here is the fix I > will roll into a v2 in a few days: I will probably have to postpone the review to next week anyway so take your time :-) /Jarkko > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig > index a95725fa77789e..ca89da3e4b2ae9 100644 > --- a/drivers/char/tpm/Kconfig > +++ b/drivers/char/tpm/Kconfig > @@ -27,16 +27,13 @@ menuconfig TCG_TPM > if TCG_TPM > > config HW_RANDOM_TPM > - tristate "TPM HW Random Number Generator support" > - depends on TCG_TPM && HW_RANDOM > - default HW_RANDOM > + bool "TPM HW Random Number Generator support" > + depends on TCG_TPM && HW_RANDOM && !(TCG_TPM=y && HW_RANDOM=m) > + default y > ---help--- > This driver provides kernel-side support for the Random Number > Generator in the Trusted Platform Module > > - To compile this driver as a module, choose M here: the > - module will be called tpm-rng. > - > If unsure, say Y. > > config TCG_TIS_CORE
Re: [PATCH] tpm: remove chip_num parameter from in-kernel API
On Tue, Oct 24, 2017 at 12:52:08PM -0600, Jason Gunthorpe wrote: > On Mon, Oct 23, 2017 at 02:38:14PM +0200, Jarkko Sakkinen wrote: > > The reasoning is simple and obvious. Since every call site passes the > > value TPM_ANY_NUM (0x) the parameter does not have right to exist. > > Refined the documentation of the corresponding functions. > > I like this patch, but how about a slightly different take, make this > change instead: > > -struct tpm_chip *tpm_chip_find_get(int chip_num) > +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip); > > Where chip == NULL means the default TPM. > > And then at all call sites swap TPM_ANY_NUM to NULL and instead of > flowing an 'int chip_num' to tpm_chip_find_get, flow the 'struct > tpm_chip *' directly. > > This gets us much closer to the desired API with about the same amount > of churn as this patch has. > > Jason I can do that. I will rework this as a patch set that includes this proposal. Gives us more clean position to continue. Thank you. /Jarkko
Re: [PATCH] tpm: Move Linux RNG connection to hwrng
On Tue, Oct 24, 2017 at 12:42:35PM -0600, Jason Gunthorpe wrote: > This is compile tested only. 0day says the kconfig has a problem when randomized, here is the fix I will roll into a v2 in a few days: diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index a95725fa77789e..ca89da3e4b2ae9 100644 --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -27,16 +27,13 @@ menuconfig TCG_TPM if TCG_TPM config HW_RANDOM_TPM - tristate "TPM HW Random Number Generator support" - depends on TCG_TPM && HW_RANDOM - default HW_RANDOM + bool "TPM HW Random Number Generator support" + depends on TCG_TPM && HW_RANDOM && !(TCG_TPM=y && HW_RANDOM=m) + default y ---help--- This driver provides kernel-side support for the Random Number Generator in the Trusted Platform Module - To compile this driver as a module, choose M here: the - module will be called tpm-rng. - If unsure, say Y. config TCG_TIS_CORE
Re: [PATCH] tpm: remove chip_num parameter from in-kernel API
On Mon, Oct 23, 2017 at 02:38:14PM +0200, Jarkko Sakkinen wrote: > The reasoning is simple and obvious. Since every call site passes the > value TPM_ANY_NUM (0x) the parameter does not have right to exist. > Refined the documentation of the corresponding functions. I like this patch, but how about a slightly different take, make this change instead: -struct tpm_chip *tpm_chip_find_get(int chip_num) +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip); Where chip == NULL means the default TPM. And then at all call sites swap TPM_ANY_NUM to NULL and instead of flowing an 'int chip_num' to tpm_chip_find_get, flow the 'struct tpm_chip *' directly. This gets us much closer to the desired API with about the same amount of churn as this patch has. Jason
Re: [Part2 PATCH v6 15/38] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
On 10/19/2017 09:33 PM, Brijesh Singh wrote:
The SEV_PLATFORM_STATUS command can be used by the platform owner to
get the current status of the platform. The command is defined in
SEV spec section 5.5.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Borislav Petkov
Cc: Herbert Xu
Cc: Gary Hook
Cc: Tom Lendacky
Cc: linux-crypto@vger.kernel.org
Cc: k...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Improvements-by: Borislav Petkov
Signed-off-by: Brijesh Singh
Acked-by: Gary R Hook
---
drivers/crypto/ccp/psp-dev.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 99f3761206da..5c921b36bc23 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -174,6 +174,27 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
return ret;
}
+static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
+{
+ struct sev_user_data_status *data;
+ int ret;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, &argp->error);
+ if (ret)
+ goto e_free;
+
+ if (copy_to_user((void __user *)argp->data, data, sizeof(*data)))
+ ret = -EFAULT;
+
+e_free:
+ kfree(data);
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long
arg)
{
void __user *argp = (void __user *)arg;
@@ -194,6 +215,9 @@ static long sev_ioctl(struct file *file, unsigned int
ioctl, unsigned long arg)
case SEV_FACTORY_RESET:
ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
break;
+ case SEV_PLATFORM_STATUS:
+ ret = sev_ioctl_do_platform_status(&input);
+ break;
default:
ret = -EINVAL;
goto out;
Re: [Part2 PATCH v6.1 16/38] crypto: ccp: Implement SEV_PEK_GEN ioctl command
On 10/23/2017 04:55 PM, Brijesh Singh wrote:
The SEV_PEK_GEN command is used to generate a new Platform Endorsement
Key (PEK). The command is defined in SEV spec section 5.6.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Borislav Petkov
Cc: Herbert Xu
Cc: Gary Hook
Cc: Tom Lendacky
Cc: linux-crypto@vger.kernel.org
Cc: k...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Brijesh Singh
Acked-by: Gary R Hook
---
Changes since v6:
* when sev_do_cmd() and sev_platform_shutdown() fails then propogate
the error status code from sev_do_cmd() because it can give us
much better reason for the failure.
drivers/crypto/ccp/psp-dev.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index dd4bab143de9..18e2d8291997 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -195,6 +195,34 @@ static int sev_ioctl_do_platform_status(struct
sev_issue_cmd *argp)
return ret;
}
+static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp)
+{
+ int ret, err;
+
+ ret = sev_platform_init(NULL, &argp->error);
+ if (ret)
+ return ret;
+
+ ret = sev_do_cmd(cmd, 0, &argp->error);
+
+ if (sev_platform_shutdown(&err)) {
+ /*
+* If both sev_do_cmd() and sev_platform_shutdown() commands
+* failed then propogate the error code from the sev_do_cmd()
+* because it contains a useful status code for the command
+* failure.
+*/
+ if (ret)
+ goto done;
+
+ argp->error = err;
+ ret = -EIO;
+ }
+
+done:
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long
arg)
{
void __user *argp = (void __user *)arg;
@@ -218,6 +246,9 @@ static long sev_ioctl(struct file *file, unsigned int
ioctl, unsigned long arg)
case SEV_PLATFORM_STATUS:
ret = sev_ioctl_do_platform_status(&input);
break;
+ case SEV_PEK_GEN:
+ ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PEK_GEN, &input);
+ break;
default:
ret = -EINVAL;
goto out;
[PATCH] tpm: Move Linux RNG connection to hwrng
The tpm-rng.c approach is completely inconsistent with how the kernel
handles hotplug. Instead manage a hwrng device for each TPM. This will
cause the kernel to read entropy from the TPM when it is plugged in,
and allow access to the TPM rng via /dev/hwrng.
Signed-off-by: PrasannaKumar Muralidharan
Signed-off-by: Jason Gunthorpe
Reviewed-by: Jason Gunthorpe
---
drivers/char/hw_random/Kconfig | 13 ---
drivers/char/hw_random/Makefile | 1 -
drivers/char/hw_random/tpm-rng.c | 50
drivers/char/tpm/Kconfig | 13 +++
drivers/char/tpm/tpm-chip.c | 41
drivers/char/tpm/tpm-interface.c | 37 -
drivers/char/tpm/tpm.h | 5
7 files changed, 76 insertions(+), 84 deletions(-)
delete mode 100644 drivers/char/hw_random/tpm-rng.c
All,
It is such a good idea, I took PrasannaKumar's patch, reviewed and
revised it to the level it could be merged.
This is compile tested only.
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 95a031e9eced07..a20fed182cbcce 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -306,19 +306,6 @@ config HW_RANDOM_POWERNV
If unsure, say Y.
-config HW_RANDOM_TPM
- tristate "TPM HW Random Number Generator support"
- depends on TCG_TPM
- default HW_RANDOM
- ---help---
- This driver provides kernel-side support for the Random Number
- Generator in the Trusted Platform Module
-
- To compile this driver as a module, choose M here: the
- module will be called tpm-rng.
-
- If unsure, say Y.
-
config HW_RANDOM_HISI
tristate "Hisilicon Random Number Generator support"
depends on HW_RANDOM && ARCH_HISI
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 39a67defac67cb..91cb8e8213e7c1 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -26,7 +26,6 @@ obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
obj-$(CONFIG_HW_RANDOM_HISI) += hisi-rng.o
-obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
deleted file mode 100644
index d6d448266f0752..00
--- a/drivers/char/hw_random/tpm-rng.c
+++ /dev/null
@@ -1,50 +0,0 @@
-/*
- * Copyright (C) 2012 Kent Yoder IBM Corporation
- *
- * HWRNG interfaces to pull RNG data from a TPM
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
- */
-
-#include
-#include
-#include
-
-#define MODULE_NAME "tpm-rng"
-
-static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
-{
- return tpm_get_random(TPM_ANY_NUM, data, max);
-}
-
-static struct hwrng tpm_rng = {
- .name = MODULE_NAME,
- .read = tpm_rng_read,
-};
-
-static int __init rng_init(void)
-{
- return hwrng_register(&tpm_rng);
-}
-module_init(rng_init);
-
-static void __exit rng_exit(void)
-{
- hwrng_unregister(&tpm_rng);
-}
-module_exit(rng_exit);
-
-MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR("Kent Yoder ");
-MODULE_DESCRIPTION("RNG driver for TPM devices");
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index a30352202f1fdc..a95725fa77789e 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -26,6 +26,19 @@ menuconfig TCG_TPM
if TCG_TPM
+config HW_RANDOM_TPM
+ tristate "TPM HW Random Number Generator support"
+ depends on TCG_TPM && HW_RANDOM
+ default HW_RANDOM
+ ---help---
+ This driver provides kernel-side support for the Random Number
+ Generator in the Trusted Platform Module
+
+ To compile this driver as a module, choose M here: the
+ module will be called tpm-rng.
+
+ If unsure, say Y.
+
config TCG_TIS_CORE
tristate
---help---
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0eca20c5a80cf2..f3571406fb2995 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -26,6 +26,7 @@
#include
#include
Re: [Part2 PATCH v6.1 19/38] crypto: ccp: Implement SEV_PEK_CERT_IMPORT ioctl command
On 10/23/2017 05:14 PM, Brijesh Singh wrote:
The SEV_PEK_CERT_IMPORT command can be used to import the signed PEK
certificate. The command is defined in SEV spec section 5.8.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Borislav Petkov
Cc: Herbert Xu
Cc: Gary Hook
Cc: Tom Lendacky
Cc: linux-crypto@vger.kernel.org
Cc: k...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Brijesh Singh
Acked-by: Gary R Hook
---
Changes since v6:
* when sev_do_cmd() and sev_platform_shutdown() fails then propogate
the error status code from sev_do_cmd() because it can give us
much better reason for the failure.
drivers/crypto/ccp/psp-dev.c | 92
include/linux/psp-sev.h | 4 ++
2 files changed, 96 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index aaf1c5cf821d..108fc06bcdb3 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -301,6 +301,95 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
return ret;
}
+void *psp_copy_user_blob(u64 __user uaddr, u32 len)
+{
+ void *data;
+
+ if (!uaddr || !len)
+ return ERR_PTR(-EINVAL);
+
+ /* verify that blob length does not exceed our limit */
+ if (len > SEV_FW_BLOB_MAX_SIZE)
+ return ERR_PTR(-EINVAL);
+
+ data = kmalloc(len, GFP_KERNEL);
+ if (!data)
+ return ERR_PTR(-ENOMEM);
+
+ if (copy_from_user(data, (void __user *)(uintptr_t)uaddr, len))
+ goto e_free;
+
+ return data;
+
+e_free:
+ kfree(data);
+ return ERR_PTR(-EFAULT);
+}
+EXPORT_SYMBOL_GPL(psp_copy_user_blob);
+
+static int sev_ioctl_do_pek_cert_import(struct sev_issue_cmd *argp)
+{
+ struct sev_user_data_pek_cert_import input;
+ struct sev_data_pek_cert_import *data;
+ void *pek_blob, *oca_blob;
+ int ret, err;
+
+ if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
+ return -EFAULT;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ /* copy PEK certificate blobs from userspace */
+ pek_blob = psp_copy_user_blob(input.pek_cert_address,
input.pek_cert_len);
+ if (IS_ERR(pek_blob)) {
+ ret = PTR_ERR(pek_blob);
+ goto e_free;
+ }
+
+ data->pek_cert_address = __psp_pa(pek_blob);
+ data->pek_cert_len = input.pek_cert_len;
+
+ /* copy PEK certificate blobs from userspace */
+ oca_blob = psp_copy_user_blob(input.oca_cert_address,
input.oca_cert_len);
+ if (IS_ERR(oca_blob)) {
+ ret = PTR_ERR(oca_blob);
+ goto e_free_pek;
+ }
+
+ data->oca_cert_address = __psp_pa(oca_blob);
+ data->oca_cert_len = input.oca_cert_len;
+
+ ret = sev_platform_init(NULL, &argp->error);
+ if (ret)
+ goto e_free_oca;
+
+ ret = sev_do_cmd(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
+
+ if (sev_platform_shutdown(&err)) {
+ /*
+* If both sev_do_cmd() and sev_platform_shutdown() commands
+* failed then propogate the error code from the sev_do_cmd()
+* because it contains a useful status code for the command
+* failure.
+*/
+ if (ret)
+ goto e_free_oca;
+
+ ret = -EIO;
+ argp->error = err;
+ }
+
+e_free_oca:
+ kfree(oca_blob);
+e_free_pek:
+ kfree(pek_blob);
+e_free:
+ kfree(data);
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long
arg)
{
void __user *argp = (void __user *)arg;
@@ -333,6 +422,9 @@ static long sev_ioctl(struct file *file, unsigned int
ioctl, unsigned long arg)
case SEV_PEK_CSR:
ret = sev_ioctl_do_pek_csr(&input);
break;
+ case SEV_PEK_CERT_IMPORT:
+ ret = sev_ioctl_do_pek_cert_import(&input);
+ break;
default:
ret = -EINVAL;
goto out;
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index eac850a97610..d535153ca82d 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -620,6 +620,8 @@ int sev_guest_df_flush(int *error);
*/
int sev_guest_decommission(struct sev_data_decommission *data, int *error);
+void *psp_copy_user_blob(u64 __user uaddr, u32 len);
+
#else /* !CONFIG_CRYPTO_DEV_SP_PSP */
static inline int
@@ -648,6 +650,8 @@ sev_issue_cmd_external_user(struct file *filep,
return -ENODEV;
}
+static inline void *psp_copy_user_blob(u64 __user uaddr, u32 len) { return ERR_PTR(-EINVAL); }
+
#endif/* CONFIG_CRYPTO_DEV_SP_PSP */
#endif /* __PSP_SEV_H__ */
Re: [Part2 PATCH v6.1 20/38] crypto: ccp: Implement SEV_PDH_CERT_EXPORT ioctl command
On 10/23/2017 05:19 PM, Brijesh Singh wrote:
The SEV_PDH_CERT_EXPORT command can be used to export the PDH and its
certificate chain. The command is defined in SEV spec section 5.10.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Borislav Petkov
Cc: Herbert Xu
Cc: Gary Hook
Cc: Tom Lendacky
Cc: linux-crypto@vger.kernel.org
Cc: k...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Brijesh Singh
Acked-by: Gary R Hook
---
Changes since v6:
* when sev_do_cmd() and sev_platform_shutdown() fails then propogate
the error status code from sev_do_cmd() because it can give us
much better reason for the failure.
drivers/crypto/ccp/psp-dev.c | 110 +++
1 file changed, 110 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 108fc06bcdb3..b9f594cb10c1 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -390,6 +390,113 @@ static int sev_ioctl_do_pek_cert_import(struct
sev_issue_cmd *argp)
return ret;
}
+static int sev_ioctl_do_pdh_cert_export(struct sev_issue_cmd *argp)
+{
+ struct sev_user_data_pdh_cert_export input;
+ void *pdh_blob = NULL, *cert_blob = NULL;
+ struct sev_data_pdh_cert_export *data;
+ int ret, err;
+
+ if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
+ return -EFAULT;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ /* Userspace wants to query the certificate length */
+ if (!input.pdh_cert_address || !input.pdh_cert_len ||
+ !input.cert_chain_address || !input.cert_chain_address)
+ goto cmd;
+
+ /* allocate a physically contiguous buffer to store the PDH blob */
+ if (!access_ok(VERIFY_WRITE, input.pdh_cert_address,
input.pdh_cert_len) ||
+ (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE)) {
+ ret = -EFAULT;
+ goto e_free;
+ }
+
+ pdh_blob = kmalloc(input.pdh_cert_len, GFP_KERNEL);
+ if (!pdh_blob) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+
+ data->pdh_cert_address = __psp_pa(pdh_blob);
+ data->pdh_cert_len = input.pdh_cert_len;
+
+ /* allocate a physically contiguous buffer to store the cert chain blob
*/
+ if (!access_ok(VERIFY_WRITE, input.cert_chain_address,
input.cert_chain_len) ||
+ (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE)) {
+ ret = -EFAULT;
+ goto e_free_pdh;
+ }
+
+ cert_blob = kmalloc(input.cert_chain_len, GFP_KERNEL);
+ if (!cert_blob) {
+ ret = -ENOMEM;
+ goto e_free_pdh;
+ }
+
+ data->cert_chain_address = __psp_pa(cert_blob);
+ data->cert_chain_len = input.cert_chain_len;
+
+cmd:
+ ret = sev_platform_init(NULL, &argp->error);
+ if (ret)
+ goto e_free_cert;
+
+ ret = sev_do_cmd(SEV_CMD_PDH_CERT_EXPORT, data, &argp->error);
+
+ /*
+* If we query the length, FW responded with expected data
+*/
+ input.cert_chain_len = data->cert_chain_len;
+ input.pdh_cert_len = data->pdh_cert_len;
+
+ if (sev_platform_shutdown(&err)) {
+ /*
+* If both sev_do_cmd() and sev_platform_shutdown() commands
+* failed then propogate the error code from the sev_do_cmd()
+* because it contains a useful status code for the command
+* failure.
+*/
+ if (ret)
+ goto e_free_cert;
+
+ ret = -EIO;
+ argp->error = err;
+ goto e_free_cert;
+ }
+
+ if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
+ ret = -EFAULT;
+ goto e_free_cert;
+ }
+
+ if (pdh_blob) {
+ if (copy_to_user((void __user *)input.pdh_cert_address,
+pdh_blob, input.pdh_cert_len)) {
+ ret = -EFAULT;
+ goto e_free_cert;
+ }
+ }
+
+ if (cert_blob) {
+ if (copy_to_user((void __user *)input.cert_chain_address,
+cert_blob, input.cert_chain_len))
+ ret = -EFAULT;
+ }
+
+e_free_cert:
+ kfree(cert_blob);
+e_free_pdh:
+ kfree(pdh_blob);
+e_free:
+ kfree(data);
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long
arg)
{
void __user *argp = (void __user *)arg;
@@ -425,6 +532,9 @@ static long sev_ioctl(struct file *file, unsigned int
ioctl, unsigned long arg)
case SEV_PEK_CERT_IMPORT:
ret = sev_ioctl_do_pek_cert_import(&input);
break;
+ case SEV_PDH_CERT_EXPORT:
+ ret = sev_ioctl_do_pdh_cert_expo
Re: [Part2 PATCH v6.1 18/38] crypto: ccp: Implement SEV_PEK_CSR ioctl command
On 10/23/2017 05:10 PM, Brijesh Singh wrote:
The SEV_PEK_CSR command can be used to generate a PEK certificate
signing request. The command is defined in SEV spec section 5.7.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Borislav Petkov
Cc: Herbert Xu
Cc: Gary Hook
Cc: Tom Lendacky
Cc: linux-crypto@vger.kernel.org
Cc: k...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Brijesh Singh
---
Acked-by: Gary R Hook
Changes since v6:
* when sev_do_cmd() and sev_platform_shutdown() fails then propogate
the error status code from sev_do_cmd() because it can give us
much better reason for the failure.
drivers/crypto/ccp/psp-dev.c | 81
1 file changed, 81 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 3672435150cf..aaf1c5cf821d 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -223,6 +223,84 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct
sev_issue_cmd *argp)
return ret;
}
+static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
+{
+ struct sev_user_data_pek_csr input;
+ struct sev_data_pek_csr *data;
+ void *blob = NULL;
+ int ret, err;
+
+ if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
+ return -EFAULT;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ /* userspace wants to query CSR length */
+ if (!input.address || !input.length)
+ goto cmd;
+
+ /* allocate a physically contiguous buffer to store the CSR blob */
+ if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
+ input.length > SEV_FW_BLOB_MAX_SIZE) {
+ ret = -EFAULT;
+ goto e_free;
+ }
+
+ blob = kmalloc(input.length, GFP_KERNEL);
+ if (!blob) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+
+ data->address = __psp_pa(blob);
+ data->len = input.length;
+
+cmd:
+ ret = sev_platform_init(NULL, &argp->error);
+ if (ret)
+ goto e_free_blob;
+
+ ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
+
+ /*
+* If we query the CSR length, FW responded with expected data
+*/
+ input.length = data->len;
+
+ if (sev_platform_shutdown(&err)) {
+ /*
+* If both sev_do_cmd() and sev_platform_shutdown() commands
+* failed then propogate the error code from the sev_do_cmd()
+* because it contains a useful status code for the command
+* failure.
+*/
+ if (ret)
+ goto e_free_blob;
+
+ ret = -EIO;
+ argp->error = err;
+ goto e_free_blob;
+ }
+
+ if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
+ ret = -EFAULT;
+ goto e_free_blob;
+ }
+
+ if (blob) {
+ if (copy_to_user((void __user *)input.address, blob,
input.length))
+ ret = -EFAULT;
+ }
+
+e_free_blob:
+ kfree(blob);
+e_free:
+ kfree(data);
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long
arg)
{
void __user *argp = (void __user *)arg;
@@ -252,6 +330,9 @@ static long sev_ioctl(struct file *file, unsigned int
ioctl, unsigned long arg)
case SEV_PDH_GEN:
ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PDH_GEN, &input);
break;
+ case SEV_PEK_CSR:
+ ret = sev_ioctl_do_pek_csr(&input);
+ break;
default:
ret = -EINVAL;
goto out;
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
Am 24. Oktober 2017 20:15:12 MESZ schrieb Jarkko Sakkinen : >On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote: >> On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe >> wrote: >> > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar >Muralidharan wrote: >> >> Hi Jason, >> >> >> >> On 24 October 2017 at 21:25, Jason Gunthorpe >> >> wrote: >> >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar >Muralidharan wrote: >> >> > >> >> >> Please check the RFC [1]. It does use chip id. The rfc has >issues and >> >> >> has to be fixed but still there could be users of the API. >> >> >> >> >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html >> >> > >> >> > That patch isn't safe at all. You need to store a kref to th >chip in >> >> > the hwrng, not parse a string. >> >> >> >> The drivers/char/hw_random/tpm-rng.c module does not store the >chip >> >> reference so I guess the usage is safe. >> > >> > It is using the default TPM, it is always safe to use the default >tpm. >> >> tpm-rng is abomination that should be kicked out as soon as possible. >> It wrecks havoc with the power management (TPM chip drivers may go >> into suspend state, but tpm_rng does not do any power management and >> happily forwards requests to suspended hardware) and may be available >> when there is no TPM at all yet (the drivers have not been probed >yet, >> or have gotten a deferral, etc). >> >> TPM core should register HWRNGs when chips are ready. >> >> Thanks. >> >> -- >> Dmitry > >I'm fine to review a two patch set where: > >1. Patch 1 removes the existing TPM rng driver >2. Patch 2 makes the TPM driver as rng producer Yes, but tpm must be kept a hwrng source. This is imho an important use case. > >Unrelate to patch that I'm proposing now but this sounds sensible. > >/Jarkko -- Sent from my mobile
Re: [Part2 PATCH v6 14/38] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command
On 10/19/2017 09:33 PM, Brijesh Singh wrote:
The SEV_FACTORY_RESET command can be used by the platform owner to
reset the non-volatile SEV related data. The command is defined in
SEV spec section 5.4
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Borislav Petkov
Cc: Herbert Xu
Cc: Gary Hook
Cc: Tom Lendacky
Cc: linux-crypto@vger.kernel.org
Cc: k...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Improvements-by: Borislav Petkov
Signed-off-by: Brijesh Singh
Acked-by: Gary R Hook
---
drivers/crypto/ccp/psp-dev.c | 28 +++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e9966d5fc6c4..99f3761206da 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -176,7 +176,33 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
{
- return -ENOTTY;
+ void __user *argp = (void __user *)arg;
+ struct sev_issue_cmd input;
+ int ret = -EFAULT;
+
+ if (ioctl != SEV_ISSUE_CMD)
+ return -EINVAL;
+
+ if (copy_from_user(&input, argp, sizeof(struct sev_issue_cmd)))
+ return -EFAULT;
+
+ if (input.cmd > SEV_MAX)
+ return -EINVAL;
+
+ switch (input.cmd) {
+
+ case SEV_FACTORY_RESET:
+ ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
+ break;
+ default:
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (copy_to_user(argp, &input, sizeof(struct sev_issue_cmd)))
+ ret = -EFAULT;
+out:
+ return ret;
}
static const struct file_operations sev_fops = {
Re: [Part2 PATCH v6 17/38] crypto: ccp: Implement SEV_PDH_GEN ioctl command
On 10/19/2017 09:33 PM, Brijesh Singh wrote: The SEV_PDH_GEN command is used to re-generate the Platform Diffie-Hellman (PDH) key. The command is defined in SEV spec section 5.6. Cc: Paolo Bonzini Cc: "Radim Krčmář" Cc: Borislav Petkov Cc: Herbert Xu Cc: Gary Hook Cc: Tom Lendacky Cc: linux-crypto@vger.kernel.org Cc: k...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Brijesh Singh Acked-by: Gary R Hook --- drivers/crypto/ccp/psp-dev.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c index 1d7212da25a5..d9771d104eea 100644 --- a/drivers/crypto/ccp/psp-dev.c +++ b/drivers/crypto/ccp/psp-dev.c @@ -239,6 +239,9 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) case SEV_PEK_GEN: ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PEK_GEN, &input); break; + case SEV_PDH_GEN: + ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PDH_GEN, &input); + break; default: ret = -EINVAL; goto out;
Re: [Part2 PATCH v6 12/38] crypto: ccp: Add Platform Security Processor (PSP) device support
On 10/19/2017 09:33 PM, Brijesh Singh wrote:
The Platform Security Processor (PSP) is part of the AMD Secure
Processor (AMD-SP) functionality. The PSP is a dedicated processor
that provides support for key management commands in Secure Encrypted
Virtualization (SEV) mode, along with software-based Trusted Execution
Environment (TEE) to enable third-party trusted applications.
Note that the key management functionality provided by the SEV firmware
can be used outside of the kvm-amd driver hence it doesn't need to
depend on CONFIG_KVM_AMD.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Borislav Petkov
Cc: Herbert Xu
Cc: Gary Hook
Cc: Tom Lendacky
Cc: linux-crypto@vger.kernel.org
Cc: k...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Improvements-by: Borislav Petkov
Signed-off-by: Brijesh Singh
Reviewed-by: Borislav Petkov
Acked-by: Gary R Hook
---
drivers/crypto/ccp/Kconfig | 11 +
drivers/crypto/ccp/Makefile | 1 +
drivers/crypto/ccp/psp-dev.c | 105 +++
drivers/crypto/ccp/psp-dev.h | 59
drivers/crypto/ccp/sp-dev.c | 26 +++
drivers/crypto/ccp/sp-dev.h | 24 +-
drivers/crypto/ccp/sp-pci.c | 52 +
7 files changed, 277 insertions(+), 1 deletion(-)
create mode 100644 drivers/crypto/ccp/psp-dev.c
create mode 100644 drivers/crypto/ccp/psp-dev.h
diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
index 9c84f9838931..b9dfae47aefd 100644
--- a/drivers/crypto/ccp/Kconfig
+++ b/drivers/crypto/ccp/Kconfig
@@ -33,3 +33,14 @@ config CRYPTO_DEV_CCP_CRYPTO
Support for using the cryptographic API with the AMD Cryptographic
Coprocessor. This module supports offload of SHA and AES algorithms.
If you choose 'M' here, this module will be called ccp_crypto.
+
+config CRYPTO_DEV_SP_PSP
+ bool "Platform Security Processor (PSP) device"
+ default y
+ depends on CRYPTO_DEV_CCP_DD && X86_64
+ help
+Provide support for the AMD Platform Security Processor (PSP).
+The PSP is a dedicated processor that provides support for key
+management commands in Secure Encrypted Virtualization (SEV) mode,
+along with software-based Trusted Execution Environment (TEE) to
+enable third-party trusted applications.
diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
index 57f8debfcfb3..008bae7e26ec 100644
--- a/drivers/crypto/ccp/Makefile
+++ b/drivers/crypto/ccp/Makefile
@@ -7,6 +7,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_CCP) += ccp-dev.o \
ccp-dmaengine.o \
ccp-debugfs.o
ccp-$(CONFIG_PCI) += sp-pci.o
+ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o
obj-$(CONFIG_CRYPTO_DEV_CCP_CRYPTO) += ccp-crypto.o
ccp-crypto-objs := ccp-crypto-main.o \
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
new file mode 100644
index ..b5789f878560
--- /dev/null
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -0,0 +1,105 @@
+/*
+ * AMD Platform Security Processor (PSP) interface
+ *
+ * Copyright (C) 2016-2017 Advanced Micro Devices, Inc.
+ *
+ * Author: Brijesh Singh
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "sp-dev.h"
+#include "psp-dev.h"
+
+static struct psp_device *psp_alloc_struct(struct sp_device *sp)
+{
+ struct device *dev = sp->dev;
+ struct psp_device *psp;
+
+ psp = devm_kzalloc(dev, sizeof(*psp), GFP_KERNEL);
+ if (!psp)
+ return NULL;
+
+ psp->dev = dev;
+ psp->sp = sp;
+
+ snprintf(psp->name, sizeof(psp->name), "psp-%u", sp->ord);
+
+ return psp;
+}
+
+static irqreturn_t psp_irq_handler(int irq, void *data)
+{
+ return IRQ_HANDLED;
+}
+
+int psp_dev_init(struct sp_device *sp)
+{
+ struct device *dev = sp->dev;
+ struct psp_device *psp;
+ int ret;
+
+ ret = -ENOMEM;
+ psp = psp_alloc_struct(sp);
+ if (!psp)
+ goto e_err;
+
+ sp->psp_data = psp;
+
+ psp->vdata = (struct psp_vdata *)sp->dev_vdata->psp_vdata;
+ if (!psp->vdata) {
+ ret = -ENODEV;
+ dev_err(dev, "missing driver data\n");
+ goto e_err;
+ }
+
+ psp->io_regs = sp->io_map + psp->vdata->offset;
+
+ /* Disable and clear interrupts until ready */
+ iowrite32(0, psp->io_regs + PSP_P2CMSG_INTEN);
+ iowrite32(-1, psp->io_regs + PSP_P2CMSG_INTSTS);
+
+ /* Request an irq */
+ ret = sp_request_psp_irq(psp->sp, psp_irq_handler, psp->name, psp);
+ if (ret) {
+ dev_err(dev, "psp: unable to allocate an IRQ\n");
+ goto e_err;
+ }
+
+
Re: [Part2 PATCH v6 09/38] crypto: ccp: Build the AMD secure processor driver only with AMD CPU support
On 10/19/2017 09:33 PM, Brijesh Singh wrote: From: Borislav Petkov This is AMD-specific hardware so present it in Kconfig only when AMD CPU support is enabled or on ARM64 where it is also used. Signed-off-by: Borislav Petkov Signed-off-by: Brijesh Singh Cc: Brijesh Singh Cc: Tom Lendacky Cc: Gary Hook Cc: Herbert Xu Cc: "David S. Miller" Cc: linux-crypto@vger.kernel.org Reviewed-by: Gary R Hook --- drivers/crypto/ccp/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig index 6d626606b9c5..9c84f9838931 100644 --- a/drivers/crypto/ccp/Kconfig +++ b/drivers/crypto/ccp/Kconfig @@ -1,5 +1,6 @@ config CRYPTO_DEV_CCP_DD tristate "Secure Processor device driver" + depends on CPU_SUP_AMD || ARM64 default m help Provides AMD Secure Processor device driver.
Re: [Part2 PATCH v6 11/38] crypto: ccp: Define SEV key management command id
On 10/19/2017 09:33 PM, Brijesh Singh wrote:
Define Secure Encrypted Virtualization (SEV) key management command id
and structure. The command definition is available in SEV KM [1] spec
0.14.
[1] http://support.amd.com/TechDocs/55766_SEV-KM API_Specification.pdf
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Borislav Petkov
Cc: Herbert Xu
Cc: Gary Hook
Cc: Tom Lendacky
Cc: linux-crypto@vger.kernel.org
Cc: k...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Improvements-by: Borislav Petkov
Signed-off-by: Brijesh Singh
Reviewed-by: Borislav Petkov
Acked-by: Gary R Hook
---
include/linux/psp-sev.h | 494
1 file changed, 494 insertions(+)
create mode 100644 include/linux/psp-sev.h
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
new file mode 100644
index ..15bda519538e
--- /dev/null
+++ b/include/linux/psp-sev.h
@@ -0,0 +1,494 @@
+/*
+ * AMD Secure Encrypted Virtualization (SEV) driver interface
+ *
+ * Copyright (C) 2016-2017 Advanced Micro Devices, Inc.
+ *
+ * Author: Brijesh Singh
+ *
+ * SEV spec 0.14 is available at:
+ * http://support.amd.com/TechDocs/55766_SEV-KM API_Specification.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __PSP_SEV_H__
+#define __PSP_SEV_H__
+
+#include
+
+#ifdef CONFIG_X86
+#include
+
+#define __psp_pa(x)__sme_pa(x)
+#else
+#define __psp_pa(x)__pa(x)
+#endif
+
+#define SEV_FW_BLOB_MAX_SIZE 0x4000 /* 16KB */
+
+/**
+ * SEV platform state
+ */
+enum sev_state {
+ SEV_STATE_UNINIT= 0x0,
+ SEV_STATE_INIT = 0x1,
+ SEV_STATE_WORKING = 0x2,
+
+ SEV_STATE_MAX
+};
+
+/**
+ * SEV platform and guest management commands
+ */
+enum sev_cmd {
+ /* platform commands */
+ SEV_CMD_INIT= 0x001,
+ SEV_CMD_SHUTDOWN= 0x002,
+ SEV_CMD_FACTORY_RESET = 0x003,
+ SEV_CMD_PLATFORM_STATUS = 0x004,
+ SEV_CMD_PEK_GEN = 0x005,
+ SEV_CMD_PEK_CSR = 0x006,
+ SEV_CMD_PEK_CERT_IMPORT = 0x007,
+ SEV_CMD_PDH_CERT_EXPORT = 0x008,
+ SEV_CMD_PDH_GEN = 0x009,
+ SEV_CMD_DF_FLUSH= 0x00A,
+
+ /* Guest commands */
+ SEV_CMD_DECOMMISSION= 0x020,
+ SEV_CMD_ACTIVATE= 0x021,
+ SEV_CMD_DEACTIVATE = 0x022,
+ SEV_CMD_GUEST_STATUS= 0x023,
+
+ /* Guest launch commands */
+ SEV_CMD_LAUNCH_START= 0x030,
+ SEV_CMD_LAUNCH_UPDATE_DATA = 0x031,
+ SEV_CMD_LAUNCH_UPDATE_VMSA = 0x032,
+ SEV_CMD_LAUNCH_MEASURE = 0x033,
+ SEV_CMD_LAUNCH_UPDATE_SECRET= 0x034,
+ SEV_CMD_LAUNCH_FINISH = 0x035,
+
+ /* Guest migration commands (outgoing) */
+ SEV_CMD_SEND_START = 0x040,
+ SEV_CMD_SEND_UPDATE_DATA= 0x041,
+ SEV_CMD_SEND_UPDATE_VMSA= 0x042,
+ SEV_CMD_SEND_FINISH = 0x043,
+
+ /* Guest migration commands (incoming) */
+ SEV_CMD_RECEIVE_START = 0x050,
+ SEV_CMD_RECEIVE_UPDATE_DATA = 0x051,
+ SEV_CMD_RECEIVE_UPDATE_VMSA = 0x052,
+ SEV_CMD_RECEIVE_FINISH = 0x053,
+
+ /* Guest debug commands */
+ SEV_CMD_DBG_DECRYPT = 0x060,
+ SEV_CMD_DBG_ENCRYPT = 0x061,
+
+ SEV_CMD_MAX,
+};
+
+/**
+ * status code returned by the commands
+ */
+enum psp_ret_code {
+ SEV_RET_SUCCESS = 0,
+ SEV_RET_INVALID_PLATFORM_STATE,
+ SEV_RET_INVALID_GUEST_STATE,
+ SEV_RET_INAVLID_CONFIG,
+ SEV_RET_INVALID_len,
+ SEV_RET_ALREADY_OWNED,
+ SEV_RET_INVALID_CERTIFICATE,
+ SEV_RET_POLICY_FAILURE,
+ SEV_RET_INACTIVE,
+ SEV_RET_INVALID_ADDRESS,
+ SEV_RET_BAD_SIGNATURE,
+ SEV_RET_BAD_MEASUREMENT,
+ SEV_RET_ASID_OWNED,
+ SEV_RET_INVALID_ASID,
+ SEV_RET_WBINVD_REQUIRED,
+ SEV_RET_DFFLUSH_REQUIRED,
+ SEV_RET_INVALID_GUEST,
+ SEV_RET_INVALID_COMMAND,
+ SEV_RET_ACTIVE,
+ SEV_RET_HWSEV_RET_PLATFORM,
+ SEV_RET_HWSEV_RET_UNSAFE,
+ SEV_RET_UNSUPPORTED,
+ SEV_RET_MAX,
+};
+
+/**
+ * struct sev_data_init - INIT command parameters
+ *
+ * @flags: processing flags
+ * @tmr_address: system physical address used for SEV-ES
+ * @tmr_len: len of tmr_address
+ */
+struct sev_data_init {
+ u32 flags; /* In */
+ u32 reserved; /* In */
+ u64 tmr_address;/* In */
+ u32 tmr_len;/* In */
+} __packed;
+
+/**
+ * struct sev_data_pek_csr - PEK_CSR command parameters
+ *
+ * @address: PEK certificate chain
+ *
Re: [Part2 PATCH v6 10/38] crypto: ccp: Define SEV userspace ioctl and command id
On 10/19/2017 09:33 PM, Brijesh Singh wrote:
Add a include file which defines the ioctl and command id used for
issuing SEV platform management specific commands.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Borislav Petkov
Cc: Herbert Xu
Cc: Gary Hook
Cc: Tom Lendacky
Cc: linux-crypto@vger.kernel.org
Cc: k...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Improvements-by: Borislav Petkov
Signed-off-by: Brijesh Singh
Reviewed-by: Borislav Petkov
Acked-by: Gary R Hook
---
include/uapi/linux/psp-sev.h | 113 +++
1 file changed, 113 insertions(+)
create mode 100644 include/uapi/linux/psp-sev.h
diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
new file mode 100644
index ..1dd98ba4ff22
--- /dev/null
+++ b/include/uapi/linux/psp-sev.h
@@ -0,0 +1,113 @@
+/*
+ * Userspace interface for AMD Secure Encrypted Virtualization (SEV)
+ * platform management commands.
+ *
+ * Copyright (C) 2016-2017 Advanced Micro Devices, Inc.
+ *
+ * Author: Brijesh Singh
+ *
+ * SEV spec 0.14 is available at:
+ * http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __PSP_SEV_USER_H__
+#define __PSP_SEV_USER_H__
+
+#include
+
+/**
+ * SEV platform commands
+ */
+enum {
+ SEV_FACTORY_RESET = 0,
+ SEV_PLATFORM_STATUS,
+ SEV_PEK_GEN,
+ SEV_PEK_CSR,
+ SEV_PDH_GEN,
+ SEV_PDH_CERT_EXPORT,
+ SEV_PEK_CERT_IMPORT,
+
+ SEV_MAX,
+};
+
+/**
+ * struct sev_user_data_status - PLATFORM_STATUS command parameters
+ *
+ * @major: major API version
+ * @minor: minor API version
+ * @state: platform state
+ * @flags: platform config flags
+ * @build: firmware build id for API version
+ * @guest_count: number of active guests
+ */
+struct sev_user_data_status {
+ __u8 api_major; /* Out */
+ __u8 api_minor; /* Out */
+ __u8 state; /* Out */
+ __u32 flags;/* Out */
+ __u8 build; /* Out */
+ __u32 guest_count; /* Out */
+} __packed;
+
+/**
+ * struct sev_user_data_pek_csr - PEK_CSR command parameters
+ *
+ * @address: PEK certificate chain
+ * @length: length of certificate
+ */
+struct sev_user_data_pek_csr {
+ __u64 address; /* In */
+ __u32 length; /* In/Out */
+} __packed;
+
+/**
+ * struct sev_user_data_cert_import - PEK_CERT_IMPORT command parameters
+ *
+ * @pek_address: PEK certificate chain
+ * @pek_len: length of PEK certificate
+ * @oca_address: OCA certificate chain
+ * @oca_len: length of OCA certificate
+ */
+struct sev_user_data_pek_cert_import {
+ __u64 pek_cert_address; /* In */
+ __u32 pek_cert_len; /* In */
+ __u64 oca_cert_address; /* In */
+ __u32 oca_cert_len; /* In */
+} __packed;
+
+/**
+ * struct sev_user_data_pdh_cert_export - PDH_CERT_EXPORT command parameters
+ *
+ * @pdh_address: PDH certificate address
+ * @pdh_len: length of PDH certificate
+ * @cert_chain_address: PDH certificate chain
+ * @cert_chain_len: length of PDH certificate chain
+ */
+struct sev_user_data_pdh_cert_export {
+ __u64 pdh_cert_address; /* In */
+ __u32 pdh_cert_len; /* In/Out */
+ __u64 cert_chain_address; /* In */
+ __u32 cert_chain_len; /* In/Out */
+} __packed;
+
+/**
+ * struct sev_issue_cmd - SEV ioctl parameters
+ *
+ * @cmd: SEV commands to execute
+ * @opaque: pointer to the command structure
+ * @error: SEV FW return code on failure
+ */
+struct sev_issue_cmd {
+ __u32 cmd; /* In */
+ __u64 data; /* In */
+ __u32 error;/* Out */
+} __packed;
+
+#define SEV_IOC_TYPE 'S'
+#define SEV_ISSUE_CMD _IOWR(SEV_IOC_TYPE, 0x0, struct sev_issue_cmd)
+
+#endif /* __PSP_USER_SEV_H */
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Tue, Oct 24, 2017 at 10:05:20PM +0530, PrasannaKumar Muralidharan wrote: > > 1. Every user in the kernel is using TPM_ANY_NUM, which means there are > >no other users. > > Completely agree that there is no in kernel users yet. And should never be. It's a bogus parameter that makes no sense. > > 2. Moving struct tpm_rng to the TPM client is architecturally > >uacceptable. > > As there was no response to the patch there is no way to know whether > it is acceptable or not. I like the idea of removing the tpm rng driver as discussed in other emails in this thread. > > 3. Using zero deos not give you any better guarantees on anything than > >just using TPM_ANY_NUM. > > Chip id is used, not zero. Sorry I misread the patch first time. Anyway it's not any kind of ID to be trusted. > > Why this patch is not CC'd to linux-integrity? It modifies the TPM > > driver. And in the worst way. > > TPM list is moderated and the moderator has not approved it yet. > get_maintainer script did not say about linux-integrity mailing list. > > It could be doing things in worst way but it is not known until some > one says. If no one tells it is the case I don't think it is possible > to fix. Which is what happened. Understood. We've moved to linux-integr...@vger.kernel.org. MAINTAINERS update is in the queue for the next kernel release. > > Implementing the ideas that Jason explained is the senseful way to > > get stable access. modules.dep makes sure that the modules are loaded > > in the correct order. > > If that is sensible then it is the way to go. > > There must be a reason to believe what is sensible and what is not. > Looks like this RFC has helped in judging that. > > Regards, > PrasannaKumar Would you be interested to work on patch set that would remove the existing tpm rng driver and make the TPM driver the customer? It's not that far away from the work you've been doing already. /Jarkko
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote: > On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe > wrote: > > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote: > >> Hi Jason, > >> > >> On 24 October 2017 at 21:25, Jason Gunthorpe > >> wrote: > >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan > >> > wrote: > >> > > >> >> Please check the RFC [1]. It does use chip id. The rfc has issues and > >> >> has to be fixed but still there could be users of the API. > >> >> > >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html > >> > > >> > That patch isn't safe at all. You need to store a kref to th chip in > >> > the hwrng, not parse a string. > >> > >> The drivers/char/hw_random/tpm-rng.c module does not store the chip > >> reference so I guess the usage is safe. > > > > It is using the default TPM, it is always safe to use the default tpm. > > tpm-rng is abomination that should be kicked out as soon as possible. > It wrecks havoc with the power management (TPM chip drivers may go > into suspend state, but tpm_rng does not do any power management and > happily forwards requests to suspended hardware) and may be available > when there is no TPM at all yet (the drivers have not been probed yet, > or have gotten a deferral, etc). > > TPM core should register HWRNGs when chips are ready. > > Thanks. > > -- > Dmitry I'm fine to review a two patch set where: 1. Patch 1 removes the existing TPM rng driver 2. Patch 2 makes the TPM driver as rng producer Unrelate to patch that I'm proposing now but this sounds sensible. /Jarkko
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Tue, Oct 24, 2017 at 11:37:57AM -0600, Jason Gunthorpe wrote: > On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote: > > tpm-rng is abomination that should be kicked out as soon as possible. > > It wrecks havoc with the power management (TPM chip drivers may go > > into suspend state, but tpm_rng does not do any power management and > > happily forwards requests to suspended hardware) and may be available > > when there is no TPM at all yet (the drivers have not been probed yet, > > or have gotten a deferral, etc). > > Makes sense > > > TPM core should register HWRNGs when chips are ready. > > The main thing I've wanted from the TPM RNG is > 'add_early_randomness'.. I see... However you can't add any randomness if hardware is not quite there yet. > > We can certainly provide a TPM interface to hwrng, it seems > reasonable. > > Excep that we already have a user api in /dev/tpm to access the > tpm RNG, is the duplication a problem? If we already have userspace API we have to maintain this, even if it is duplicate, there is no way around it. I'd expect most of the users will use unified random API, while some TPM-oriented users may still go via /dev/tpm to use the same API as all other their requests. Thanks. -- Dmitry
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
Hi Jason, On 24 October 2017 at 23:16, Jason Gunthorpe wrote: > On Tue, Oct 24, 2017 at 09:44:30PM +0530, PrasannaKumar Muralidharan wrote: > >> I am wondering why it is wrong. Isn't the chip id valid till it is >> unregistered? If so the rfc is correct. Please explain, may be I am >> missing something. > > The lifetime is a bit complicated, but the general rule in the kernel > for things like this it to use pointers, not ids, and certainly not > string ids. > > For that patch it could just use container_of to get the chip.. > > Jason hwrng requires a unique name for every device. In that patch "tpm-rng-" is used. chip_num is nothing but dev->dev_num. This way more than 1 tpm chip can be used as rng provider. tpm_get_random uses chip_num as its parameter. This is why chip->dev_num was used. Is that reasoning correct? Please feel free to correct me if I am wrong. Thanks, PrasannaKumar
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Tue, Oct 24, 2017 at 09:44:30PM +0530, PrasannaKumar Muralidharan wrote: > I am wondering why it is wrong. Isn't the chip id valid till it is > unregistered? If so the rfc is correct. Please explain, may be I am > missing something. The lifetime is a bit complicated, but the general rule in the kernel for things like this it to use pointers, not ids, and certainly not string ids. For that patch it could just use container_of to get the chip.. Jason
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On 24 October 2017 at 23:07, Jason Gunthorpe wrote: > On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote: >> tpm-rng is abomination that should be kicked out as soon as possible. >> It wrecks havoc with the power management (TPM chip drivers may go >> into suspend state, but tpm_rng does not do any power management and >> happily forwards requests to suspended hardware) and may be available >> when there is no TPM at all yet (the drivers have not been probed yet, >> or have gotten a deferral, etc). > > Makes sense > >> TPM core should register HWRNGs when chips are ready. > > The main thing I've wanted from the TPM RNG is > 'add_early_randomness'.. > > We can certainly provide a TPM interface to hwrng, it seems > reasonable. > > Excep that we already have a user api in /dev/tpm to access the > tpm RNG, is the duplication a problem? I tried to do that via the rfc we discussed previously. It may not be the right way but I wanted to start the discussion via the rfc. Thanks, PrasannaKumar
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote: > tpm-rng is abomination that should be kicked out as soon as possible. > It wrecks havoc with the power management (TPM chip drivers may go > into suspend state, but tpm_rng does not do any power management and > happily forwards requests to suspended hardware) and may be available > when there is no TPM at all yet (the drivers have not been probed yet, > or have gotten a deferral, etc). Makes sense > TPM core should register HWRNGs when chips are ready. The main thing I've wanted from the TPM RNG is 'add_early_randomness'.. We can certainly provide a TPM interface to hwrng, it seems reasonable. Excep that we already have a user api in /dev/tpm to access the tpm RNG, is the duplication a problem? Jason
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe wrote: > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote: >> Hi Jason, >> >> On 24 October 2017 at 21:25, Jason Gunthorpe >> wrote: >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote: >> > >> >> Please check the RFC [1]. It does use chip id. The rfc has issues and >> >> has to be fixed but still there could be users of the API. >> >> >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html >> > >> > That patch isn't safe at all. You need to store a kref to th chip in >> > the hwrng, not parse a string. >> >> The drivers/char/hw_random/tpm-rng.c module does not store the chip >> reference so I guess the usage is safe. > > It is using the default TPM, it is always safe to use the default tpm. tpm-rng is abomination that should be kicked out as soon as possible. It wrecks havoc with the power management (TPM chip drivers may go into suspend state, but tpm_rng does not do any power management and happily forwards requests to suspended hardware) and may be available when there is no TPM at all yet (the drivers have not been probed yet, or have gotten a deferral, etc). TPM core should register HWRNGs when chips are ready. Thanks. -- Dmitry
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On 24 October 2017 at 21:53, Jarkko Sakkinen
wrote:
> On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>> On 24 October 2017 at 21:14, Jarkko Sakkinen
>> wrote:
>> > On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
>> >> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
>> >>
>> >> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>> >> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
>> >> > > {
>> >> >
>> >> >
>> >> > I think every kernel internal TPM driver API should be called with the
>> >> > tpm_chip as a parameter. This is in foresight of namespacing of IMA
>> >> > where we
>> >> > want to provide the flexibility of passing a dedicated vTPM to each
>> >> > namespace and IMA would use the chip as a parameter to all of these
>> >> > functions to talk to the right tpm_vtpm_proxy instance. From that
>> >> > perspective this patch goes into the wrong direction.
>> >>
>> >> Yes, we should ultimately try and get to there.. Someday the
>> >> tpm_chip_find_get() will need to become namespace aware.
>> >>
>> >> But this patch is along the right path, eliminating the chip_num is
>> >> the right thing to do..
>> >>
>> >> > >- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
>> >> > >+ tpm2 = tpm_is_tpm2();
>> >> > > if (tpm2 < 0)
>> >> > > return tpm2;
>> >> > >
>> >> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
>> >> > > switch (key_cmd) {
>> >> > > case Opt_load:
>> >> > > if (tpm2)
>> >> > >- ret = tpm_unseal_trusted(TPM_ANY_NUM, payload,
>> >> > >options);
>> >> > >+ ret = tpm_unseal_trusted(payload, options);
>> >>
>> >> Sequences like this are sketchy.
>> >>
>> >> It should be
>> >>
>> >> struct tpm_chip *tpm;
>> >>
>> >> tpm = tpm_chip_find_get()
>> >> tpm2 = tpm_is_tpm2(tpm);
>> >>
>> >> [..]
>> >>
>> >> if (tpm2)
>> >> ret = tpm_unseal_trusted(tpm, payload, options);
>> >>
>> >> [..]
>> >>
>> >> tpm_put_chip(tpm);
>> >>
>> >> As hot plug could alter the 'tpm' between the two tpm calls.
>> >>
>> >> Jason
>> >
>> > This patch just removes bunch of dead code. It does not change existing
>> > semantics. What you are saying should be done after the dead code has
>> > been removed. This commit is first step to that direction.
>> >
>> > /Jarkko
>>
>> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> has to be fixed but still there could be users of the API.
>>
>> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>>
>> Regards,
>> PrasannaKumar
>
> 1. Every user in the kernel is using TPM_ANY_NUM, which means there are
>no other users.
Completely agree that there is no in kernel users yet.
> 2. Moving struct tpm_rng to the TPM client is architecturally
>uacceptable.
As there was no response to the patch there is no way to know whether
it is acceptable or not.
> 3. Using zero deos not give you any better guarantees on anything than
>just using TPM_ANY_NUM.
Chip id is used, not zero.
> Why this patch is not CC'd to linux-integrity? It modifies the TPM
> driver. And in the worst way.
TPM list is moderated and the moderator has not approved it yet.
get_maintainer script did not say about linux-integrity mailing list.
It could be doing things in worst way but it is not known until some
one says. If no one tells it is the case I don't think it is possible
to fix. Which is what happened.
> Implementing the ideas that Jason explained is the senseful way to
> get stable access. modules.dep makes sure that the modules are loaded
> in the correct order.
If that is sensible then it is the way to go.
There must be a reason to believe what is sensible and what is not.
Looks like this RFC has helped in judging that.
Regards,
PrasannaKumar
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
> On 24 October 2017 at 21:14, Jarkko Sakkinen
> wrote:
> > On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
> >> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
> >>
> >> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> >> > > {
> >> >
> >> >
> >> > I think every kernel internal TPM driver API should be called with the
> >> > tpm_chip as a parameter. This is in foresight of namespacing of IMA
> >> > where we
> >> > want to provide the flexibility of passing a dedicated vTPM to each
> >> > namespace and IMA would use the chip as a parameter to all of these
> >> > functions to talk to the right tpm_vtpm_proxy instance. From that
> >> > perspective this patch goes into the wrong direction.
> >>
> >> Yes, we should ultimately try and get to there.. Someday the
> >> tpm_chip_find_get() will need to become namespace aware.
> >>
> >> But this patch is along the right path, eliminating the chip_num is
> >> the right thing to do..
> >>
> >> > >- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> >> > >+ tpm2 = tpm_is_tpm2();
> >> > > if (tpm2 < 0)
> >> > > return tpm2;
> >> > >
> >> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> >> > > switch (key_cmd) {
> >> > > case Opt_load:
> >> > > if (tpm2)
> >> > >- ret = tpm_unseal_trusted(TPM_ANY_NUM, payload,
> >> > >options);
> >> > >+ ret = tpm_unseal_trusted(payload, options);
> >>
> >> Sequences like this are sketchy.
> >>
> >> It should be
> >>
> >> struct tpm_chip *tpm;
> >>
> >> tpm = tpm_chip_find_get()
> >> tpm2 = tpm_is_tpm2(tpm);
> >>
> >> [..]
> >>
> >> if (tpm2)
> >> ret = tpm_unseal_trusted(tpm, payload, options);
> >>
> >> [..]
> >>
> >> tpm_put_chip(tpm);
> >>
> >> As hot plug could alter the 'tpm' between the two tpm calls.
> >>
> >> Jason
> >
> > This patch just removes bunch of dead code. It does not change existing
> > semantics. What you are saying should be done after the dead code has
> > been removed. This commit is first step to that direction.
> >
> > /Jarkko
>
> Please check the RFC [1]. It does use chip id. The rfc has issues and
> has to be fixed but still there could be users of the API.
>
> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>
> Regards,
> PrasannaKumar
1. Every user in the kernel is using TPM_ANY_NUM, which means there are
no other users.
2. Moving struct tpm_rng to the TPM client is architecturally
uacceptable.
3. Using zero deos not give you any better guarantees on anything than
just using TPM_ANY_NUM.
Why this patch is not CC'd to linux-integrity? It modifies the TPM
driver. And in the worst way.
Implementing the ideas that Jason explained is the senseful way to
get stable access. modules.dep makes sure that the modules are loaded
in the correct order.
/Jarkko
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On 24 October 2017 at 21:41, Jason Gunthorpe wrote: > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote: >> Hi Jason, >> >> On 24 October 2017 at 21:25, Jason Gunthorpe >> wrote: >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote: >> > >> >> Please check the RFC [1]. It does use chip id. The rfc has issues and >> >> has to be fixed but still there could be users of the API. >> >> >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html >> > >> > That patch isn't safe at all. You need to store a kref to th chip in >> > the hwrng, not parse a string. >> >> The drivers/char/hw_random/tpm-rng.c module does not store the chip >> reference so I guess the usage is safe. > > It is using the default TPM, it is always safe to use the default tpm. > >> The RFC is just a sample use case of the API. > > Well, a wrong example not to be emulated, and I think, further shows > how Jarkko's direction is the right one. I am wondering why it is wrong. Isn't the chip id valid till it is unregistered? If so the rfc is correct. Please explain, may be I am missing something. Thanks, PrasannaKumar
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote: > Hi Jason, > > On 24 October 2017 at 21:25, Jason Gunthorpe > wrote: > > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote: > > > >> Please check the RFC [1]. It does use chip id. The rfc has issues and > >> has to be fixed but still there could be users of the API. > >> > >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html > > > > That patch isn't safe at all. You need to store a kref to th chip in > > the hwrng, not parse a string. > > The drivers/char/hw_random/tpm-rng.c module does not store the chip > reference so I guess the usage is safe. It is using the default TPM, it is always safe to use the default tpm. > The RFC is just a sample use case of the API. Well, a wrong example not to be emulated, and I think, further shows how Jarkko's direction is the right one. Jason
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
Hi Jason, On 24 October 2017 at 21:25, Jason Gunthorpe wrote: > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote: > >> Please check the RFC [1]. It does use chip id. The rfc has issues and >> has to be fixed but still there could be users of the API. >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html > > That patch isn't safe at all. You need to store a kref to th chip in > the hwrng, not parse a string. The drivers/char/hw_random/tpm-rng.c module does not store the chip reference so I guess the usage is safe. The RFC is just a sample use case of the API. Regards, PrasannaKumar
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote: > Please check the RFC [1]. It does use chip id. The rfc has issues and > has to be fixed but still there could be users of the API. > > 1. https://www.spinics.net/lists/linux-crypto/msg28282.html That patch isn't safe at all. You need to store a kref to th chip in the hwrng, not parse a string. Jason
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On 24 October 2017 at 21:14, Jarkko Sakkinen
wrote:
> On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
>> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
>>
>> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
>> > > {
>> >
>> >
>> > I think every kernel internal TPM driver API should be called with the
>> > tpm_chip as a parameter. This is in foresight of namespacing of IMA where
>> > we
>> > want to provide the flexibility of passing a dedicated vTPM to each
>> > namespace and IMA would use the chip as a parameter to all of these
>> > functions to talk to the right tpm_vtpm_proxy instance. From that
>> > perspective this patch goes into the wrong direction.
>>
>> Yes, we should ultimately try and get to there.. Someday the
>> tpm_chip_find_get() will need to become namespace aware.
>>
>> But this patch is along the right path, eliminating the chip_num is
>> the right thing to do..
>>
>> > >- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
>> > >+ tpm2 = tpm_is_tpm2();
>> > > if (tpm2 < 0)
>> > > return tpm2;
>> > >
>> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
>> > > switch (key_cmd) {
>> > > case Opt_load:
>> > > if (tpm2)
>> > >- ret = tpm_unseal_trusted(TPM_ANY_NUM, payload,
>> > >options);
>> > >+ ret = tpm_unseal_trusted(payload, options);
>>
>> Sequences like this are sketchy.
>>
>> It should be
>>
>> struct tpm_chip *tpm;
>>
>> tpm = tpm_chip_find_get()
>> tpm2 = tpm_is_tpm2(tpm);
>>
>> [..]
>>
>> if (tpm2)
>> ret = tpm_unseal_trusted(tpm, payload, options);
>>
>> [..]
>>
>> tpm_put_chip(tpm);
>>
>> As hot plug could alter the 'tpm' between the two tpm calls.
>>
>> Jason
>
> This patch just removes bunch of dead code. It does not change existing
> semantics. What you are saying should be done after the dead code has
> been removed. This commit is first step to that direction.
>
> /Jarkko
Please check the RFC [1]. It does use chip id. The rfc has issues and
has to be fixed but still there could be users of the API.
1. https://www.spinics.net/lists/linux-crypto/msg28282.html
Regards,
PrasannaKumar
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
>
> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> > > {
> >
> >
> > I think every kernel internal TPM driver API should be called with the
> > tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
> > want to provide the flexibility of passing a dedicated vTPM to each
> > namespace and IMA would use the chip as a parameter to all of these
> > functions to talk to the right tpm_vtpm_proxy instance. From that
> > perspective this patch goes into the wrong direction.
>
> Yes, we should ultimately try and get to there.. Someday the
> tpm_chip_find_get() will need to become namespace aware.
>
> But this patch is along the right path, eliminating the chip_num is
> the right thing to do..
>
> > >- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> > >+ tpm2 = tpm_is_tpm2();
> > > if (tpm2 < 0)
> > > return tpm2;
> > >
> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> > > switch (key_cmd) {
> > > case Opt_load:
> > > if (tpm2)
> > >- ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
> > >+ ret = tpm_unseal_trusted(payload, options);
>
> Sequences like this are sketchy.
>
> It should be
>
> struct tpm_chip *tpm;
>
> tpm = tpm_chip_find_get()
> tpm2 = tpm_is_tpm2(tpm);
>
> [..]
>
> if (tpm2)
> ret = tpm_unseal_trusted(tpm, payload, options);
>
> [..]
>
> tpm_put_chip(tpm);
>
> As hot plug could alter the 'tpm' between the two tpm calls.
>
> Jason
This patch just removes bunch of dead code. It does not change existing
semantics. What you are saying should be done after the dead code has
been removed. This commit is first step to that direction.
/Jarkko
Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator
Hi, Romain,
On 10/18/2017 04:32 PM, Romain Izard wrote:
diff --git a/crypto/ccm.c b/crypto/ccm.c
index 1ce37ae0ce56..e7c2121a3ab2 100644
--- a/crypto/ccm.c
+++ b/crypto/ccm.c
@@ -47,6 +47,7 @@ struct crypto_ccm_req_priv_ctx {
u8 odata[16];
u8 idata[16];
u8 auth_tag[16];
+ u8 iv[16];
u32 flags;
struct scatterlist src[3];
struct scatterlist dst[3];
@@ -248,32 +249,22 @@ static void crypto_ccm_encrypt_done(struct
crypto_async_request *areq, int err)
aead_request_complete(req, err);
}
-static inline int crypto_ccm_check_iv(const u8 *iv)
-{
- /* 2 <= L <= 8, so 1 <= L' <= 7. */
- if (1 > iv[0] || iv[0] > 7)
- return -EINVAL;
-
- return 0;
-}
-
-static int crypto_ccm_init_crypt(struct aead_request *req, u8 *tag)
+static int crypto_ccm_init_crypt(struct aead_request *req, u8 *tag, u8* iv)
{
struct crypto_ccm_req_priv_ctx *pctx = crypto_ccm_reqctx(req);
struct scatterlist *sg;
- u8 *iv = req->iv;
- int err;
+ u8 L = req->iv[0] + 1;
- err = crypto_ccm_check_iv(iv);
- if (err)
- return err;
-
- pctx->flags = aead_request_flags(req);
+ if (2 > L || L > 8)
+ return -EINVAL;
/* Note: rfc 3610 and NIST 800-38C require counter of
* zero to encrypt auth tag.
*/
- memset(iv + 15 - iv[0], 0, iv[0] + 1);
+ memcpy(iv, req->iv, 16 - L);
+ memset(iv + 16 - L, 0, L);
+
+ pctx->flags = aead_request_flags(req);
sg_init_table(pctx->src, 3);
sg_set_buf(pctx->src, tag, 16);
@@ -301,10 +292,10 @@ static int crypto_ccm_encrypt(struct aead_request *req)
struct scatterlist *dst;
unsigned int cryptlen = req->cryptlen;
u8 *odata = pctx->odata;
- u8 *iv = req->iv;
+ u8 *iv = pctx->iv;
int err;
- err = crypto_ccm_init_crypt(req, odata);
+ err = crypto_ccm_init_crypt(req, odata, iv);
if (err)
return err;
@@ -363,12 +354,12 @@ static int crypto_ccm_decrypt(struct aead_request *req)
unsigned int cryptlen = req->cryptlen;
u8 *authtag = pctx->auth_tag;
u8 *odata = pctx->odata;
- u8 *iv = req->iv;
+ u8 *iv = pctx->iv;
int err;
cryptlen -= authsize;
- err = crypto_ccm_init_crypt(req, authtag);
+ err = crypto_ccm_init_crypt(req, authtag, iv);
if (err)
return err;
Looks good. Can you please submit with a commit message?
Thanks,
ta
Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API
On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote: > I think every kernel internal TPM driver API should be called with the > tpm_chip as a parameter. This is in foresight of namespacing of IMA where we > want to provide the flexibility of passing a dedicated vTPM to each > namespace and IMA would use the chip as a parameter to all of these > functions to talk to the right tpm_vtpm_proxy instance. From that > perspective this patch goes into the wrong direction. > >Stefan The goal of this patch is to kernel code that never gets executed. It removes a load of completely dead code. It is the only thing that this commit does. Why do you think this is "going into wrong direction" if it only removes dead code and refines the documentation up to date? After the dead code has been removed it makes sense to propose a better mechanism. Maybe the one that you are speaking about. But you need to remove the cruft first. /Jarkko
Re: [Part2 PATCH v6 00/38] x86: Secure Encrypted Virtualization (AMD)
Hi Herbert and Paolo, On 10/19/17 9:33 PM, Brijesh Singh wrote: > This part of Secure Encryted Virtualization (SEV) patch series focuses on KVM > changes required to create and manage SEV guests. > > SEV is an extension to the AMD-V architecture which supports running encrypted > virtual machine (VMs) under the control of a hypervisor. Encrypted VMs have > their > pages (code and data) secured such that only the guest itself has access to > unencrypted version. Each encrypted VM is associated with a unique encryption > key; > if its data is accessed to a different entity using a different key the > encrypted > guest's data will be incorrectly decrypted, leading to unintelligible data. > This security model ensures that hypervisor will no longer able to inspect or > alter any guest code or data. > > The key management of this feature is handled by a separate processor known as > the AMD Secure Processor (AMD-SP) which is present on AMD SOCs. The SEV Key > Management Specification (see below) provides a set of commands which can be > used by hypervisor to load virtual machine keys through the AMD-SP driver. Since the PSP patches touches both the CCP and KVM driver, hence I was wondering if you guys have any thought on how PSP patches will be merged? I am talking about Patch 9 to 20 from this series. I have ensured that patches apply cleanly on both kvm/master and cryptodev-2.6/master. We can do this in one of two ways: - Paolo can merge the PSP support through the KVM branch or - Herbert can create a topic branch with PSP changes and Paolo can use that topic branch. Any visibility will help my next submission. thank you. -Brijesh
Re: [PATCH v7 2/2] crypto: s5p-sss: Add HASH support for Exynos
Hi Vladimir,
Thank you for review, I will apply almost all of your remarks,
see answers below.
On 22.10.2017 12:18, Vladimir Zapolskiy wrote:
> Hi Kamil,
>
> thank you for updates, I have just a few more comments.
>
> On 10/17/2017 02:28 PM, Kamil Konieczny wrote:
>> [...]
>> - Select sw algorithms MD5, SHA1 and SHA256 in EXYNOS_HASH
>> as they are nedded for fallback.
>
> Typo, s/nedded/needed/
ok, Thank you for spotting this.
> [snip]
>
>>
>> #include
>> #include
>> +#include
>
> I can not find which interfaces from linux/delay.h are needed.
> I suppose it should not be added.
>
> [snip]
Yes, you are right, I will delete this 'include delay.h'
>> -static struct s5p_aes_dev *s5p_dev;
>> +/**
>> + * struct s5p_hash_reqctx - HASH request context
>> + * @dev:Associated device
>
> dev or dd?
dd
>> + * @op_update: Current request operation (OP_UPDATE or OP_FINAL)
>> + * @digcnt: Number of bytes processed by HW (without buffer[] ones)
>> + * @digest: Digest message or IV for partial result
>> + * @nregs: Number of HW registers for digest or IV read/write
>> + * @engine: Bits for selecting type of HASH in SSS block
>> + * @sg: sg for DMA transfer
>> + * @sg_len: Length of sg for DMA transfer
>> + * @sgl[]: sg for joining buffer and req->src scatterlist
>> + * @skip: Skip offset in req->src for current op
>> + * @total: Total number of bytes for current request
>> + * @finup: Keep state for finup or final.
>> + * @error: Keep track of error.
>> + * @bufcnt: Number of bytes holded in buffer[]
>> + * @buffer[]: For byte(s) from end of req->src in UPDATE op
>> + */
>> +struct s5p_hash_reqctx {
>> +struct s5p_aes_dev *dd;
>> +boolop_update;
>> +
>
> [snip]
>
>> +
>> +/**
>> + * s5p_hash_shash_digest() - calculate shash digest
>> + * @tfm:crypto transformation
>> + * @flags: tfm flags
>> + * @data: input data
>> + * @len:length of data
>> + * @out:output buffer
>> + */
>> +static int s5p_hash_shash_digest(struct crypto_shash *tfm, u32 flags,
>> + const u8 *data, unsigned int len, u8 *out)
>> +{
>> +SHASH_DESC_ON_STACK(shash, tfm);
>
> Just for information, this line produces a spatch warning:
>
> drivers/crypto/s5p-sss.c:1534:9: warning: Variable length array is used.
>
> I think it can be ignored.
I also think it can be ignored, it uses 104 bytes on stack in case of SHA256 ,
sizeof struct sha256_state for SW generic implementation, found in
include/crypto/sha.h
>> +
>> +shash->tfm = tfm;
>> +shash->flags = flags & ~CRYPTO_TFM_REQ_MAY_SLEEP;
>> +
>> +return crypto_shash_digest(shash, data, len, out);
>> +}
>> +
>
> [snip]
>
>> +/**
>> + * s5p_hash_final() - close up hash and calculate digest
>> + * @req:AHASH request
>> + *
>> + * Note: in final req->src do not have any data, and req->nbytes can be
>> + * non-zero.
>> + *
>> + * If there were no input data processed yet and the buffered hash data is
>> + * less than BUFLEN (64) then calculate the final hash immediately by using
>> + * SW algorithm fallback.
>> + *
>> + * Otherwise enqueues the current AHASH request with OP_FINAL operation op
>> + * and finalize hash message in HW. Note that if digcnt!=0 then there were
>> + * previous update op, so there are always some buffered bytes in
>> ctx->buffer,
>> + * which means that ctx->bufcnt!=0
>> + *
>> + * Returns:
>> + * 0 if the request has been processed immediately,
>> + * -EINPROGRESS if the operation has been queued for later execution or is
>> set
>> + * to processing by HW,
>> + * -EBUSY if queue is full and request should be resubmitted later, other
>> + * negative values on error.
>
> Do you want to add -EINVAL into the list also?
Here I made bad formatting, it should read:
* -EBUSY if queue is full and request should be resubmitted later,
* other negative values on error.
I do not want to specify explicitly all error negative codes here, as it also
uses
s5p_hash_enqueue and these return codes are referenced from other places. I
intended
to listing success values, 0, -EINPROGRESS, then explaining -EBUSY, then adding
all
other negative as error. The other values can be -EINVAL, -ENOMEM or maybe
other, when
this module gets extended to HMAC implementation.
>> + */
>> +static int s5p_hash_final(struct ahash_request *req)
>> +{
>> +struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
>> +
>> +ctx->finup = true;
>> +if (ctx->error)
>> +return -EINVAL; /* uncompleted hash is not needed */
>> +
>> +if (!ctx->digcnt && ctx->bufcnt < BUFLEN)
>> +return s5p_hash_final_shash(req);
>> +
>> +return s5p_hash_enqueue(req, false); /* HASH_OP_FINAL */
>> +}
>> +
>
> [snip]
>
>> +/**
>> + * s5p_hash_finup() - process last req->src and calculate digest
>> + * @req:AHASH request containing the last update data
>> + *
>> + * Return values: see s5p_hash_final above.
>> + */
>> +static int
Re: [PATCH v7 1/2] crypto: s5p-sss: change spaces into tabs in defines
Hi Vladimir, Thank you for review. On 22.10.2017 12:18, Vladimir Zapolskiy wrote: > Hi Kamil, > > On 10/17/2017 02:28 PM, Kamil Konieczny wrote: >> change spaces into tabs in defines > > Here a grammatically correct sentence in English is welcome. What about: "Change spaces to tabs" ? >> Signed-off-by: Kamil Konieczny >> --- > > Please feel free to add a tag > > Acked-by: Vladimir Zapolskiy -- Best regards, Kamil Konieczny Samsung R&D Institute Poland
Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator
2017-10-24 5:20 GMT+02:00 Herbert Xu : > On Mon, Oct 23, 2017 at 03:38:59PM +0300, Tudor Ambarus wrote: >> >> I will propose a fix, but I'm taking my time to better understand why >> CTR requires to overwrite the iv with the last ciphertext block. > > That's an API requirement. So we should fix ccm. > Where is the documentation for this API requirement? I tried to find it in the kernel, but I only found a few comments in the commit messages or in the implementations, but not an explicit requirement. Moreover, as it seems to be a common mistake in the crypto accelerators, I believe that the algorithms' self-test should also check the IV at the end of a request. In the decryption case, the code should probably be shared for most implementations: we need to save the input data before decryption in case of in-place decoding, and restore it into the IV buffer before returning to the caller. -- Romain Izard
Re: [PATCH] staging: ccree: Fix lines longer than 80 characters
On Mon, Oct 23, 2017 at 6:00 PM, Stephen Brennan wrote: > Hi Gilad, > > Thanks for the quick reply, I really appreciate your taking time to help a > newbie get started. I've made the appropriate changes and re-submitted. It is completely my pleasure. Thanks, > >> TIP: if you run the scripts/get_maintainers.pl script on your patch it >> will tell you exactly which >> list and which people your patch needs to be addressed, so you don't >> have to guess. > > When I ran this tool, it listed out quite a few mailing lists, including > linux-ker...@vger.kernel.org. Is it correct to simply address your patch to > the whole list output by the script? I omitted linux-kernel on my > resubmission, simply to avoid contributing to the heavy volume of that > list, given how trivial this patch is. > Strange as it may sound, it is the protocol. If you think you cringe when sending a trivial patch there wait till you send the 20th revision of a patch that get_maintainer says needs to be cross posted to half a dozen mailing lists... :-) Posting to lkml is more than just informative. There are actually automated tool that will pick your patch and run it through a bunch of static analysers tools and try to compile it and sometime boot on a dozen different archs. Besides, lkml is a fire host as it is... :-) Gilad > Thanks again! > Stephen > -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
Re: [PATCH] staging: ccree: Fix lines longer than 80 characters
Hi Tobin, On Tue, Oct 24, 2017 at 6:02 AM, Tobin C. Harding wrote: > On Mon, Oct 23, 2017 at 07:53:18AM -0700, Stephen Brennan wrote: >> Simply break down some long lines and tab-indent them. > > Hi Stephen, > > Welcome to the Linux kernel. Great that you have put in a patch, you are, > however, unlikely to see > success fixing 'line over 80' warnings. There are a bunch of arguments for > and against the line over > 80 limit, a web search will surely show them to you. The TL;DR is that it > these changes do not > _really_ improve the readability of the code, they are just changes to quiet > the static analysis > tool. I completely agree with you that the end target is more readable code and that lines over 80 char is only a symptom but I do think in this case there is something useful to do. Perhaps, if Stephen is willing, re-write the code to be more readable by, for example, using a temp variable for the register address, and in doing so both making the code more readable as well as treating the symptom? Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
