| Message ID | 20220228120638.678137-8-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: rng: Move TPM RNG functionality to driver model | expand |
Hi Sughosh, On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > Currently, the TPM random number generator(RNG) functions are defined > as part of the library functions under the corresponding tpm files for > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers > complying with the driver model. > > Also make changes to the tpm_get_random function to have it call the > TPM RNG driver functions instead of the library functions. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > > Changes since V1: > * Added existing copyrights for the rng functions taken from the tpm > library routines > * Return -EIO for TPM command returning an error > * Simplify the logic in tpm_get_random based on the review comments > from Ilias > > drivers/rng/Makefile | 1 + > drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++ > drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++ > lib/tpm-v1.c | 44 --------------------- > lib/tpm-v2.c | 44 --------------------- > lib/tpm_api.c | 23 +++++++++-- > 6 files changed, 193 insertions(+), 92 deletions(-) > create mode 100644 drivers/rng/tpm1_rng.c > create mode 100644 drivers/rng/tpm2_rng.c > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile > index 39f7ee3f03..129cfbd006 100644 > --- a/drivers/rng/Makefile > +++ b/drivers/rng/Makefile > @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o > obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o > obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o > obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o > +obj-$(CONFIG_TPM) += tpm1_rng.o tpm2_rng.o > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c > new file mode 100644 > index 0000000000..7e629756b3 > --- /dev/null > +++ b/drivers/rng/tpm1_rng.c > @@ -0,0 +1,87 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2013 The Chromium OS Authors. > + * Coypright (c) 2013 Guntermann & Drunck GmbH > + * Copyright (c) 2022, Linaro Limited > + */ > + > +#include <common.h> > +#include <dm.h> > +#include <rng.h> > +#include <tpm-utils.h> > +#include <tpm-v1.h> > + > +#include <linux/errno.h> > + > +#define TPM_HEADER_SIZE 10 > + > +#define TPMV1_DATA_OFFSET 14 > + > +/** > + * tpm1_rng_read() - Read the random bytes from TPMv1 device > + * @param dev TPMv1 RNG device > + * @param data data buffer to write random bytes > + * @param count number of random bytes to read from > + * the device > + * > + * Function to read the random bytes from the RNG pseudo device > + * built into the TPMv1 device. Reads 'count' number of bytes > + * from the random number generator and copies them into the > + * 'data' buffer. > + * > + * Return: 0 if OK, -ve on error. > + * > + */ > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count) > +{ > + const u8 command[14] = { > + 0x0, 0xc1, /* TPM_TAG */ > + 0x0, 0x0, 0x0, 0xe, /* parameter size */ > + 0x0, 0x0, 0x0, 0x46, /* TPM_COMMAND_CODE */ > + }; > + const size_t length_offset = TPM_HEADER_SIZE; > + const size_t data_size_offset = TPM_HEADER_SIZE; > + const size_t data_offset = TPMV1_DATA_OFFSET; > + u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; > + size_t response_length = sizeof(response); > + u32 data_size; > + u8 *out = data; > + The current model is that all TPM calls are set up in lib/tpm and I don't think we should change it. You should be able to move these functions into lib/tpm and add your random_read function to tpm_api.h Regards, Simon
hi Simon, On Tue, 1 Mar 2022 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > Currently, the TPM random number generator(RNG) functions are defined > > as part of the library functions under the corresponding tpm files for > > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers > > complying with the driver model. > > > > Also make changes to the tpm_get_random function to have it call the > > TPM RNG driver functions instead of the library functions. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > > > Changes since V1: > > * Added existing copyrights for the rng functions taken from the tpm > > library routines > > * Return -EIO for TPM command returning an error > > * Simplify the logic in tpm_get_random based on the review comments > > from Ilias > > > > drivers/rng/Makefile | 1 + > > drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++ > > drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++ > > lib/tpm-v1.c | 44 --------------------- > > lib/tpm-v2.c | 44 --------------------- > > lib/tpm_api.c | 23 +++++++++-- > > 6 files changed, 193 insertions(+), 92 deletions(-) > > create mode 100644 drivers/rng/tpm1_rng.c > > create mode 100644 drivers/rng/tpm2_rng.c > > > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile > > index 39f7ee3f03..129cfbd006 100644 > > --- a/drivers/rng/Makefile > > +++ b/drivers/rng/Makefile > > @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o > > obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o > > obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o > > obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o > > +obj-$(CONFIG_TPM) += tpm1_rng.o tpm2_rng.o > > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c > > new file mode 100644 > > index 0000000000..7e629756b3 > > --- /dev/null > > +++ b/drivers/rng/tpm1_rng.c > > @@ -0,0 +1,87 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (c) 2013 The Chromium OS Authors. > > + * Coypright (c) 2013 Guntermann & Drunck GmbH > > + * Copyright (c) 2022, Linaro Limited > > + */ > > + > > +#include <common.h> > > +#include <dm.h> > > +#include <rng.h> > > +#include <tpm-utils.h> > > +#include <tpm-v1.h> > > + > > +#include <linux/errno.h> > > + > > +#define TPM_HEADER_SIZE 10 > > + > > +#define TPMV1_DATA_OFFSET 14 > > + > > +/** > > + * tpm1_rng_read() - Read the random bytes from TPMv1 device > > + * @param dev TPMv1 RNG device > > + * @param data data buffer to write random bytes > > + * @param count number of random bytes to read from > > + * the device > > + * > > + * Function to read the random bytes from the RNG pseudo device > > + * built into the TPMv1 device. Reads 'count' number of bytes > > + * from the random number generator and copies them into the > > + * 'data' buffer. > > + * > > + * Return: 0 if OK, -ve on error. > > + * > > + */ > > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count) > > +{ > > + const u8 command[14] = { > > + 0x0, 0xc1, /* TPM_TAG */ > > + 0x0, 0x0, 0x0, 0xe, /* parameter size */ > > + 0x0, 0x0, 0x0, 0x46, /* TPM_COMMAND_CODE */ > > + }; > > + const size_t length_offset = TPM_HEADER_SIZE; > > + const size_t data_size_offset = TPM_HEADER_SIZE; > > + const size_t data_offset = TPMV1_DATA_OFFSET; > > + u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; > > + size_t response_length = sizeof(response); > > + u32 data_size; > > + u8 *out = data; > > + > > The current model is that all TPM calls are set up in lib/tpm and I > don't think we should change it. You should be able to move these > functions into lib/tpm and add your random_read function to tpm_api.h I moved these functions under separate drivers as I thought that looked cleaner as against exporting the driver interface in tpm-v{1,2}.c. If you strongly feel that these should remain under lib/tpm, I will make the change. But I believe you do not have a problem with exporting these rng functions as part of the driver model. We do need that so that the EFI_RNG_PROTOCOL can use these for getting the random bytes. -sughosh > > Regards, > Simon
Hi Sughosh, On Tue, 1 Mar 2022 at 21:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Tue, 1 Mar 2022 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > Currently, the TPM random number generator(RNG) functions are defined > > > as part of the library functions under the corresponding tpm files for > > > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers > > > complying with the driver model. > > > > > > Also make changes to the tpm_get_random function to have it call the > > > TPM RNG driver functions instead of the library functions. > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > > > > Changes since V1: > > > * Added existing copyrights for the rng functions taken from the tpm > > > library routines > > > * Return -EIO for TPM command returning an error > > > * Simplify the logic in tpm_get_random based on the review comments > > > from Ilias > > > > > > drivers/rng/Makefile | 1 + > > > drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++ > > > drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++ > > > lib/tpm-v1.c | 44 --------------------- > > > lib/tpm-v2.c | 44 --------------------- > > > lib/tpm_api.c | 23 +++++++++-- > > > 6 files changed, 193 insertions(+), 92 deletions(-) > > > create mode 100644 drivers/rng/tpm1_rng.c > > > create mode 100644 drivers/rng/tpm2_rng.c > > > > > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile > > > index 39f7ee3f03..129cfbd006 100644 > > > --- a/drivers/rng/Makefile > > > +++ b/drivers/rng/Makefile > > > @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o > > > obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o > > > obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o > > > obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o > > > +obj-$(CONFIG_TPM) += tpm1_rng.o tpm2_rng.o > > > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c > > > new file mode 100644 > > > index 0000000000..7e629756b3 > > > --- /dev/null > > > +++ b/drivers/rng/tpm1_rng.c > > > @@ -0,0 +1,87 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * Copyright (c) 2013 The Chromium OS Authors. > > > + * Coypright (c) 2013 Guntermann & Drunck GmbH > > > + * Copyright (c) 2022, Linaro Limited > > > + */ > > > + > > > +#include <common.h> > > > +#include <dm.h> > > > +#include <rng.h> > > > +#include <tpm-utils.h> > > > +#include <tpm-v1.h> > > > + > > > +#include <linux/errno.h> > > > + > > > +#define TPM_HEADER_SIZE 10 > > > + > > > +#define TPMV1_DATA_OFFSET 14 > > > + > > > +/** > > > + * tpm1_rng_read() - Read the random bytes from TPMv1 device > > > + * @param dev TPMv1 RNG device > > > + * @param data data buffer to write random bytes > > > + * @param count number of random bytes to read from > > > + * the device > > > + * > > > + * Function to read the random bytes from the RNG pseudo device > > > + * built into the TPMv1 device. Reads 'count' number of bytes > > > + * from the random number generator and copies them into the > > > + * 'data' buffer. > > > + * > > > + * Return: 0 if OK, -ve on error. > > > + * > > > + */ > > > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count) > > > +{ > > > + const u8 command[14] = { > > > + 0x0, 0xc1, /* TPM_TAG */ > > > + 0x0, 0x0, 0x0, 0xe, /* parameter size */ > > > + 0x0, 0x0, 0x0, 0x46, /* TPM_COMMAND_CODE */ > > > + }; > > > + const size_t length_offset = TPM_HEADER_SIZE; > > > + const size_t data_size_offset = TPM_HEADER_SIZE; > > > + const size_t data_offset = TPMV1_DATA_OFFSET; > > > + u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; > > > + size_t response_length = sizeof(response); > > > + u32 data_size; > > > + u8 *out = data; > > > + > > > > The current model is that all TPM calls are set up in lib/tpm and I > > don't think we should change it. You should be able to move these > > functions into lib/tpm and add your random_read function to tpm_api.h > > I moved these functions under separate drivers as I thought that > looked cleaner as against exporting the driver interface in But you are now creating TPM messages in a different file so I don't think it is cleaner. The message pack/unpack should happen in the tpm_... functions. > tpm-v{1,2}.c. If you strongly feel that these should remain under > lib/tpm, I will make the change. But I believe you do not have a > problem with exporting these rng functions as part of the driver > model. We do need that so that the EFI_RNG_PROTOCOL can use these for > getting the random bytes. I think you might misunderstand me. I mean that the code that calls pack_byte_string() should be in lib/ like the other code. I agree that the driver should be were you have put it, I just don't like having the tpm message creation in that driver. It should call a tpm_... function to create and send the message, like elsewhere. Regards, Simon
hi Simon, On Thu, 3 Mar 2022 at 09:17, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Tue, 1 Mar 2022 at 21:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Tue, 1 Mar 2022 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > Currently, the TPM random number generator(RNG) functions are defined > > > > as part of the library functions under the corresponding tpm files for > > > > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers > > > > complying with the driver model. > > > > > > > > Also make changes to the tpm_get_random function to have it call the > > > > TPM RNG driver functions instead of the library functions. > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > > > > > Changes since V1: > > > > * Added existing copyrights for the rng functions taken from the tpm > > > > library routines > > > > * Return -EIO for TPM command returning an error > > > > * Simplify the logic in tpm_get_random based on the review comments > > > > from Ilias > > > > > > > > drivers/rng/Makefile | 1 + > > > > drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++ > > > > drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++ > > > > lib/tpm-v1.c | 44 --------------------- > > > > lib/tpm-v2.c | 44 --------------------- > > > > lib/tpm_api.c | 23 +++++++++-- > > > > 6 files changed, 193 insertions(+), 92 deletions(-) > > > > create mode 100644 drivers/rng/tpm1_rng.c > > > > create mode 100644 drivers/rng/tpm2_rng.c > > > > > > > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile > > > > index 39f7ee3f03..129cfbd006 100644 > > > > --- a/drivers/rng/Makefile > > > > +++ b/drivers/rng/Makefile > > > > @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o > > > > obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o > > > > obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o > > > > obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o > > > > +obj-$(CONFIG_TPM) += tpm1_rng.o tpm2_rng.o > > > > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c > > > > new file mode 100644 > > > > index 0000000000..7e629756b3 > > > > --- /dev/null > > > > +++ b/drivers/rng/tpm1_rng.c > > > > @@ -0,0 +1,87 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > +/* > > > > + * Copyright (c) 2013 The Chromium OS Authors. > > > > + * Coypright (c) 2013 Guntermann & Drunck GmbH > > > > + * Copyright (c) 2022, Linaro Limited > > > > + */ > > > > + > > > > +#include <common.h> > > > > +#include <dm.h> > > > > +#include <rng.h> > > > > +#include <tpm-utils.h> > > > > +#include <tpm-v1.h> > > > > + > > > > +#include <linux/errno.h> > > > > + > > > > +#define TPM_HEADER_SIZE 10 > > > > + > > > > +#define TPMV1_DATA_OFFSET 14 > > > > + > > > > +/** > > > > + * tpm1_rng_read() - Read the random bytes from TPMv1 device > > > > + * @param dev TPMv1 RNG device > > > > + * @param data data buffer to write random bytes > > > > + * @param count number of random bytes to read from > > > > + * the device > > > > + * > > > > + * Function to read the random bytes from the RNG pseudo device > > > > + * built into the TPMv1 device. Reads 'count' number of bytes > > > > + * from the random number generator and copies them into the > > > > + * 'data' buffer. > > > > + * > > > > + * Return: 0 if OK, -ve on error. > > > > + * > > > > + */ > > > > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count) > > > > +{ > > > > + const u8 command[14] = { > > > > + 0x0, 0xc1, /* TPM_TAG */ > > > > + 0x0, 0x0, 0x0, 0xe, /* parameter size */ > > > > + 0x0, 0x0, 0x0, 0x46, /* TPM_COMMAND_CODE */ > > > > + }; > > > > + const size_t length_offset = TPM_HEADER_SIZE; > > > > + const size_t data_size_offset = TPM_HEADER_SIZE; > > > > + const size_t data_offset = TPMV1_DATA_OFFSET; > > > > + u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; > > > > + size_t response_length = sizeof(response); > > > > + u32 data_size; > > > > + u8 *out = data; > > > > + > > > > > > The current model is that all TPM calls are set up in lib/tpm and I > > > don't think we should change it. You should be able to move these > > > functions into lib/tpm and add your random_read function to tpm_api.h > > > > I moved these functions under separate drivers as I thought that > > looked cleaner as against exporting the driver interface in > > But you are now creating TPM messages in a different file so I don't > think it is cleaner. The message pack/unpack should happen in the > tpm_... functions. > > > tpm-v{1,2}.c. If you strongly feel that these should remain under > > lib/tpm, I will make the change. But I believe you do not have a > > problem with exporting these rng functions as part of the driver > > model. We do need that so that the EFI_RNG_PROTOCOL can use these for > > getting the random bytes. > > I think you might misunderstand me. I mean that the code that calls > pack_byte_string() should be in lib/ like the other code. I agree that > the driver should be were you have put it, I just don't like having > the tpm message creation in that driver. It should call a tpm_... > function to create and send the message, like elsewhere. I tried putting a wrapper around the pack/unpack functions like you suggest, but it is getting more complicated than required. Even after putting the wrapper, there still is an issue of tpm_sendrecv being declared in tpm-utils.h, along with some other necessary macros like COMMAND_BUFFER_SIZE. You have mentioned in another review that you don't want tpm-utils.h to be moved to the include/ directory. If we are not to move tpm-utils.h and also not call functions like pack_byte_string directly from the driver, I feel it will be easier to simply export the rng functions to the driver model from within lib/tpm/ instead of moving them under drivers/rng/. Will you be okay with this. -sughosh > > Regards, > Simon
Hi Sughosh, On Thu, 3 Mar 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Thu, 3 Mar 2022 at 09:17, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Tue, 1 Mar 2022 at 21:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Tue, 1 Mar 2022 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > Currently, the TPM random number generator(RNG) functions are defined > > > > > as part of the library functions under the corresponding tpm files for > > > > > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers > > > > > complying with the driver model. > > > > > > > > > > Also make changes to the tpm_get_random function to have it call the > > > > > TPM RNG driver functions instead of the library functions. > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > --- > > > > > > > > > > Changes since V1: > > > > > * Added existing copyrights for the rng functions taken from the tpm > > > > > library routines > > > > > * Return -EIO for TPM command returning an error > > > > > * Simplify the logic in tpm_get_random based on the review comments > > > > > from Ilias > > > > > > > > > > drivers/rng/Makefile | 1 + > > > > > drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++ > > > > > drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++ > > > > > lib/tpm-v1.c | 44 --------------------- > > > > > lib/tpm-v2.c | 44 --------------------- > > > > > lib/tpm_api.c | 23 +++++++++-- > > > > > 6 files changed, 193 insertions(+), 92 deletions(-) > > > > > create mode 100644 drivers/rng/tpm1_rng.c > > > > > create mode 100644 drivers/rng/tpm2_rng.c > > > > > > > > > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile > > > > > index 39f7ee3f03..129cfbd006 100644 > > > > > --- a/drivers/rng/Makefile > > > > > +++ b/drivers/rng/Makefile > > > > > @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o > > > > > obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o > > > > > obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o > > > > > obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o > > > > > +obj-$(CONFIG_TPM) += tpm1_rng.o tpm2_rng.o > > > > > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c > > > > > new file mode 100644 > > > > > index 0000000000..7e629756b3 > > > > > --- /dev/null > > > > > +++ b/drivers/rng/tpm1_rng.c > > > > > @@ -0,0 +1,87 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > +/* > > > > > + * Copyright (c) 2013 The Chromium OS Authors. > > > > > + * Coypright (c) 2013 Guntermann & Drunck GmbH > > > > > + * Copyright (c) 2022, Linaro Limited > > > > > + */ > > > > > + > > > > > +#include <common.h> > > > > > +#include <dm.h> > > > > > +#include <rng.h> > > > > > +#include <tpm-utils.h> > > > > > +#include <tpm-v1.h> > > > > > + > > > > > +#include <linux/errno.h> > > > > > + > > > > > +#define TPM_HEADER_SIZE 10 > > > > > + > > > > > +#define TPMV1_DATA_OFFSET 14 > > > > > + > > > > > +/** > > > > > + * tpm1_rng_read() - Read the random bytes from TPMv1 device > > > > > + * @param dev TPMv1 RNG device > > > > > + * @param data data buffer to write random bytes > > > > > + * @param count number of random bytes to read from > > > > > + * the device > > > > > + * > > > > > + * Function to read the random bytes from the RNG pseudo device > > > > > + * built into the TPMv1 device. Reads 'count' number of bytes > > > > > + * from the random number generator and copies them into the > > > > > + * 'data' buffer. > > > > > + * > > > > > + * Return: 0 if OK, -ve on error. > > > > > + * > > > > > + */ > > > > > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count) > > > > > +{ > > > > > + const u8 command[14] = { > > > > > + 0x0, 0xc1, /* TPM_TAG */ > > > > > + 0x0, 0x0, 0x0, 0xe, /* parameter size */ > > > > > + 0x0, 0x0, 0x0, 0x46, /* TPM_COMMAND_CODE */ > > > > > + }; > > > > > + const size_t length_offset = TPM_HEADER_SIZE; > > > > > + const size_t data_size_offset = TPM_HEADER_SIZE; > > > > > + const size_t data_offset = TPMV1_DATA_OFFSET; > > > > > + u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; > > > > > + size_t response_length = sizeof(response); > > > > > + u32 data_size; > > > > > + u8 *out = data; > > > > > + > > > > > > > > The current model is that all TPM calls are set up in lib/tpm and I > > > > don't think we should change it. You should be able to move these > > > > functions into lib/tpm and add your random_read function to tpm_api.h > > > > > > I moved these functions under separate drivers as I thought that > > > looked cleaner as against exporting the driver interface in > > > > But you are now creating TPM messages in a different file so I don't > > think it is cleaner. The message pack/unpack should happen in the > > tpm_... functions. > > > > > tpm-v{1,2}.c. If you strongly feel that these should remain under > > > lib/tpm, I will make the change. But I believe you do not have a > > > problem with exporting these rng functions as part of the driver > > > model. We do need that so that the EFI_RNG_PROTOCOL can use these for > > > getting the random bytes. > > > > I think you might misunderstand me. I mean that the code that calls > > pack_byte_string() should be in lib/ like the other code. I agree that > > the driver should be were you have put it, I just don't like having > > the tpm message creation in that driver. It should call a tpm_... > > function to create and send the message, like elsewhere. > > I tried putting a wrapper around the pack/unpack functions like you > suggest, but it is getting more complicated than required. Even after See tpm1_get_random() ... can you write a function that works like that, i.e. implements the tpm API? You are bypassing the API, which is what I don't like. > putting the wrapper, there still is an issue of tpm_sendrecv being > declared in tpm-utils.h, along with some other necessary macros like > COMMAND_BUFFER_SIZE. You have mentioned in another review that you > don't want tpm-utils.h to be moved to the include/ directory. If we > are not to move tpm-utils.h and also not call functions like > pack_byte_string directly from the driver, I feel it will be easier to > simply export the rng functions to the driver model from within > lib/tpm/ instead of moving them under drivers/rng/. Will you be okay > with this. I'm just a bit lost as to what the problem is. See above. Perhaps you can send a patch or add a bit more detail so I can understand it. Regards, SImon
hi Simon, On Fri, 4 Mar 2022 at 08:08, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Thu, 3 Mar 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Thu, 3 Mar 2022 at 09:17, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Tue, 1 Mar 2022 at 21:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > hi Simon, > > > > > > > > On Tue, 1 Mar 2022 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > Currently, the TPM random number generator(RNG) functions are defined > > > > > > as part of the library functions under the corresponding tpm files for > > > > > > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers > > > > > > complying with the driver model. > > > > > > > > > > > > Also make changes to the tpm_get_random function to have it call the > > > > > > TPM RNG driver functions instead of the library functions. > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > --- > > > > > > > > > > > > Changes since V1: > > > > > > * Added existing copyrights for the rng functions taken from the tpm > > > > > > library routines > > > > > > * Return -EIO for TPM command returning an error > > > > > > * Simplify the logic in tpm_get_random based on the review comments > > > > > > from Ilias > > > > > > > > > > > > drivers/rng/Makefile | 1 + > > > > > > drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++ > > > > > > drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++ > > > > > > lib/tpm-v1.c | 44 --------------------- > > > > > > lib/tpm-v2.c | 44 --------------------- > > > > > > lib/tpm_api.c | 23 +++++++++-- > > > > > > 6 files changed, 193 insertions(+), 92 deletions(-) > > > > > > create mode 100644 drivers/rng/tpm1_rng.c > > > > > > create mode 100644 drivers/rng/tpm2_rng.c > > > > > > > > > > > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile > > > > > > index 39f7ee3f03..129cfbd006 100644 > > > > > > --- a/drivers/rng/Makefile > > > > > > +++ b/drivers/rng/Makefile > > > > > > @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o > > > > > > obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o > > > > > > obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o > > > > > > obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o > > > > > > +obj-$(CONFIG_TPM) += tpm1_rng.o tpm2_rng.o > > > > > > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c > > > > > > new file mode 100644 > > > > > > index 0000000000..7e629756b3 > > > > > > --- /dev/null > > > > > > +++ b/drivers/rng/tpm1_rng.c > > > > > > @@ -0,0 +1,87 @@ > > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > > +/* > > > > > > + * Copyright (c) 2013 The Chromium OS Authors. > > > > > > + * Coypright (c) 2013 Guntermann & Drunck GmbH > > > > > > + * Copyright (c) 2022, Linaro Limited > > > > > > + */ > > > > > > + > > > > > > +#include <common.h> > > > > > > +#include <dm.h> > > > > > > +#include <rng.h> > > > > > > +#include <tpm-utils.h> > > > > > > +#include <tpm-v1.h> > > > > > > + > > > > > > +#include <linux/errno.h> > > > > > > + > > > > > > +#define TPM_HEADER_SIZE 10 > > > > > > + > > > > > > +#define TPMV1_DATA_OFFSET 14 > > > > > > + > > > > > > +/** > > > > > > + * tpm1_rng_read() - Read the random bytes from TPMv1 device > > > > > > + * @param dev TPMv1 RNG device > > > > > > + * @param data data buffer to write random bytes > > > > > > + * @param count number of random bytes to read from > > > > > > + * the device > > > > > > + * > > > > > > + * Function to read the random bytes from the RNG pseudo device > > > > > > + * built into the TPMv1 device. Reads 'count' number of bytes > > > > > > + * from the random number generator and copies them into the > > > > > > + * 'data' buffer. > > > > > > + * > > > > > > + * Return: 0 if OK, -ve on error. > > > > > > + * > > > > > > + */ > > > > > > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count) > > > > > > +{ > > > > > > + const u8 command[14] = { > > > > > > + 0x0, 0xc1, /* TPM_TAG */ > > > > > > + 0x0, 0x0, 0x0, 0xe, /* parameter size */ > > > > > > + 0x0, 0x0, 0x0, 0x46, /* TPM_COMMAND_CODE */ > > > > > > + }; > > > > > > + const size_t length_offset = TPM_HEADER_SIZE; > > > > > > + const size_t data_size_offset = TPM_HEADER_SIZE; > > > > > > + const size_t data_offset = TPMV1_DATA_OFFSET; > > > > > > + u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; > > > > > > + size_t response_length = sizeof(response); > > > > > > + u32 data_size; > > > > > > + u8 *out = data; > > > > > > + > > > > > > > > > > The current model is that all TPM calls are set up in lib/tpm and I > > > > > don't think we should change it. You should be able to move these > > > > > functions into lib/tpm and add your random_read function to tpm_api.h > > > > > > > > I moved these functions under separate drivers as I thought that > > > > looked cleaner as against exporting the driver interface in > > > > > > But you are now creating TPM messages in a different file so I don't > > > think it is cleaner. The message pack/unpack should happen in the > > > tpm_... functions. > > > > > > > tpm-v{1,2}.c. If you strongly feel that these should remain under > > > > lib/tpm, I will make the change. But I believe you do not have a > > > > problem with exporting these rng functions as part of the driver > > > > model. We do need that so that the EFI_RNG_PROTOCOL can use these for > > > > getting the random bytes. > > > > > > I think you might misunderstand me. I mean that the code that calls > > > pack_byte_string() should be in lib/ like the other code. I agree that > > > the driver should be were you have put it, I just don't like having > > > the tpm message creation in that driver. It should call a tpm_... > > > function to create and send the message, like elsewhere. > > > > I tried putting a wrapper around the pack/unpack functions like you > > suggest, but it is getting more complicated than required. Even after > > See tpm1_get_random() ... can you write a function that works like > that, i.e. implements the tpm API? You are bypassing the API, which is > what I don't like. > > > putting the wrapper, there still is an issue of tpm_sendrecv being > > declared in tpm-utils.h, along with some other necessary macros like > > COMMAND_BUFFER_SIZE. You have mentioned in another review that you > > don't want tpm-utils.h to be moved to the include/ directory. If we > > are not to move tpm-utils.h and also not call functions like > > pack_byte_string directly from the driver, I feel it will be easier to > > simply export the rng functions to the driver model from within > > lib/tpm/ instead of moving them under drivers/rng/. Will you be okay > > with this. > > I'm just a bit lost as to what the problem is. See above. Perhaps you > can send a patch or add a bit more detail so I can understand it. I have sent a v3 patch series which takes the approach I was trying to explain above. Please check. Thanks. -sughosh > > Regards, > SImon
diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile index 39f7ee3f03..129cfbd006 100644 --- a/drivers/rng/Makefile +++ b/drivers/rng/Makefile @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o +obj-$(CONFIG_TPM) += tpm1_rng.o tpm2_rng.o diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c new file mode 100644 index 0000000000..7e629756b3 --- /dev/null +++ b/drivers/rng/tpm1_rng.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2013 The Chromium OS Authors. + * Coypright (c) 2013 Guntermann & Drunck GmbH + * Copyright (c) 2022, Linaro Limited + */ + +#include <common.h> +#include <dm.h> +#include <rng.h> +#include <tpm-utils.h> +#include <tpm-v1.h> + +#include <linux/errno.h> + +#define TPM_HEADER_SIZE 10 + +#define TPMV1_DATA_OFFSET 14 + +/** + * tpm1_rng_read() - Read the random bytes from TPMv1 device + * @param dev TPMv1 RNG device + * @param data data buffer to write random bytes + * @param count number of random bytes to read from + * the device + * + * Function to read the random bytes from the RNG pseudo device + * built into the TPMv1 device. Reads 'count' number of bytes + * from the random number generator and copies them into the + * 'data' buffer. + * + * Return: 0 if OK, -ve on error. + * + */ +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count) +{ + const u8 command[14] = { + 0x0, 0xc1, /* TPM_TAG */ + 0x0, 0x0, 0x0, 0xe, /* parameter size */ + 0x0, 0x0, 0x0, 0x46, /* TPM_COMMAND_CODE */ + }; + const size_t length_offset = TPM_HEADER_SIZE; + const size_t data_size_offset = TPM_HEADER_SIZE; + const size_t data_offset = TPMV1_DATA_OFFSET; + u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; + size_t response_length = sizeof(response); + u32 data_size; + u8 *out = data; + + while (count > 0) { + u32 this_bytes = min(count, + sizeof(response) - data_offset); + u32 err; + + if (pack_byte_string(buf, sizeof(buf), "sd", + 0, command, sizeof(command), + length_offset, this_bytes)) + return -EIO; + err = tpm_sendrecv_command(dev->parent, buf, response, + &response_length); + if (err) + return err; + if (unpack_byte_string(response, response_length, "d", + data_size_offset, &data_size)) + return -EIO; + if (data_size > count) + return TPM_LIB_ERROR; + if (unpack_byte_string(response, response_length, "s", + data_offset, out, data_size)) + return -EIO; + + count -= data_size; + out += data_size; + } + + return 0; +} + +static const struct dm_rng_ops tpm1_rng_ops = { + .read = tpm1_rng_read, +}; + +U_BOOT_DRIVER(tpm1_rng) = { + .name = "tpm1-rng", + .id = UCLASS_RNG, + .ops = &tpm1_rng_ops, +}; diff --git a/drivers/rng/tpm2_rng.c b/drivers/rng/tpm2_rng.c new file mode 100644 index 0000000000..e7f6be46e4 --- /dev/null +++ b/drivers/rng/tpm2_rng.c @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2018 Bootlin + * Copyright (c) 2022, Linaro Limited + */ + +#include <common.h> +#include <dm.h> +#include <rng.h> +#include <tpm-utils.h> +#include <tpm-v2.h> + +#include <linux/errno.h> + +#define TPM_HEADER_SIZE 10 + +#define TPMV2_DATA_OFFSET 12 + +/** + * tpm2_rng_read() - Read the random bytes from TPMv2 device + * @param dev TPMv2 RNG device + * @param data data buffer to write random bytes + * @param count number of random bytes to read from + * the device + * + * Function to read the random bytes from the RNG pseudo device + * built into the TPMv2 device. Reads 'count' number of bytes + * from the random number generator and copies them into the + * 'data' buffer. + * + * Return: 0 if OK, -ve on error. + * + */ +static int tpm2_rng_read(struct udevice *dev, void *data, size_t count) +{ + const u8 command_v2[10] = { + tpm_u16(TPM2_ST_NO_SESSIONS), + tpm_u32(12), + tpm_u32(TPM2_CC_GET_RANDOM), + }; + u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; + + const size_t data_size_offset = TPM_HEADER_SIZE; + const size_t data_offset = TPMV2_DATA_OFFSET; + size_t response_length = sizeof(response); + u32 data_size; + u8 *out = data; + + while (count > 0) { + u32 this_bytes = min((size_t)count, + sizeof(response) - data_offset); + u32 err; + + if (pack_byte_string(buf, sizeof(buf), "sw", + 0, command_v2, sizeof(command_v2), + sizeof(command_v2), this_bytes)) + return -EIO; + err = tpm_sendrecv_command(dev->parent, buf, response, + &response_length); + if (err) + return err; + if (unpack_byte_string(response, response_length, "w", + data_size_offset, &data_size)) + return -EIO; + if (data_size > this_bytes) + return TPM_LIB_ERROR; + if (unpack_byte_string(response, response_length, "s", + data_offset, out, data_size)) + return -EIO; + + count -= data_size; + out += data_size; + } + + return 0; +} + +static const struct dm_rng_ops tpm2_rng_ops = { + .read = tpm2_rng_read, +}; + +U_BOOT_DRIVER(tpm2_rng) = { + .name = "tpm2-rng", + .id = UCLASS_RNG, + .ops = &tpm2_rng_ops, +}; diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c index 467992e04e..71cc90a2ab 100644 --- a/lib/tpm-v1.c +++ b/lib/tpm-v1.c @@ -868,47 +868,3 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20], #endif /* CONFIG_TPM_LOAD_KEY_BY_SHA1 */ #endif /* CONFIG_TPM_AUTH_SESSIONS */ - -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count) -{ - const u8 command[14] = { - 0x0, 0xc1, /* TPM_TAG */ - 0x0, 0x0, 0x0, 0xe, /* parameter size */ - 0x0, 0x0, 0x0, 0x46, /* TPM_COMMAND_CODE */ - }; - const size_t length_offset = 10; - const size_t data_size_offset = 10; - const size_t data_offset = 14; - u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; - size_t response_length = sizeof(response); - u32 data_size; - u8 *out = data; - - while (count > 0) { - u32 this_bytes = min((size_t)count, - sizeof(response) - data_offset); - u32 err; - - if (pack_byte_string(buf, sizeof(buf), "sd", - 0, command, sizeof(command), - length_offset, this_bytes)) - return TPM_LIB_ERROR; - err = tpm_sendrecv_command(dev, buf, response, - &response_length); - if (err) - return err; - if (unpack_byte_string(response, response_length, "d", - data_size_offset, &data_size)) - return TPM_LIB_ERROR; - if (data_size > count) - return TPM_LIB_ERROR; - if (unpack_byte_string(response, response_length, "s", - data_offset, out, data_size)) - return TPM_LIB_ERROR; - - count -= data_size; - out += data_size; - } - - return 0; -} diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c index 2f16b0007b..c1456c1974 100644 --- a/lib/tpm-v2.c +++ b/lib/tpm-v2.c @@ -562,50 +562,6 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw, return tpm_sendrecv_command(dev, command_v2, NULL, NULL); } -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count) -{ - const u8 command_v2[10] = { - tpm_u16(TPM2_ST_NO_SESSIONS), - tpm_u32(12), - tpm_u32(TPM2_CC_GET_RANDOM), - }; - u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; - - const size_t data_size_offset = 10; - const size_t data_offset = 12; - size_t response_length = sizeof(response); - u32 data_size; - u8 *out = data; - - while (count > 0) { - u32 this_bytes = min((size_t)count, - sizeof(response) - data_offset); - u32 err; - - if (pack_byte_string(buf, sizeof(buf), "sw", - 0, command_v2, sizeof(command_v2), - sizeof(command_v2), this_bytes)) - return TPM_LIB_ERROR; - err = tpm_sendrecv_command(dev, buf, response, - &response_length); - if (err) - return err; - if (unpack_byte_string(response, response_length, "w", - data_size_offset, &data_size)) - return TPM_LIB_ERROR; - if (data_size > this_bytes) - return TPM_LIB_ERROR; - if (unpack_byte_string(response, response_length, "s", - data_offset, out, data_size)) - return TPM_LIB_ERROR; - - count -= data_size; - out += data_size; - } - - return 0; -} - u32 tpm2_write_lock(struct udevice *dev, u32 index) { u8 command_v2[COMMAND_BUFFER_SIZE] = { diff --git a/lib/tpm_api.c b/lib/tpm_api.c index 9dd9606fa8..5492f89959 100644 --- a/lib/tpm_api.c +++ b/lib/tpm_api.c @@ -6,6 +6,7 @@ #include <common.h> #include <dm.h> #include <log.h> +#include <rng.h> #include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> @@ -264,12 +265,26 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm) return -ENOSYS; } +#if CONFIG_IS_ENABLED(DM_RNG) int tpm_get_random(struct udevice *dev, void *data, u32 count) { + int ret = -ENOSYS; + struct udevice *rng_dev; + if (is_tpm1(dev)) - return tpm1_get_random(dev, data, count); + ret = uclass_get_device_by_driver(UCLASS_RNG, + DM_DRIVER_GET(tpm1_rng), + &rng_dev); else if (is_tpm2(dev)) - return -ENOSYS; /* not implemented yet */ - else - return -ENOSYS; + ret = uclass_get_device_by_driver(UCLASS_RNG, + DM_DRIVER_GET(tpm2_rng), + &rng_dev); + + if (ret) { + log_err("Getting tpm rng device failed\n"); + return ret; + } + + return dm_rng_read(rng_dev, data, (size_t)count); } +#endif /* DM_RNG */
Currently, the TPM random number generator(RNG) functions are defined as part of the library functions under the corresponding tpm files for tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers complying with the driver model. Also make changes to the tpm_get_random function to have it call the TPM RNG driver functions instead of the library functions. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V1: * Added existing copyrights for the rng functions taken from the tpm library routines * Return -EIO for TPM command returning an error * Simplify the logic in tpm_get_random based on the review comments from Ilias drivers/rng/Makefile | 1 + drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++ drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++ lib/tpm-v1.c | 44 --------------------- lib/tpm-v2.c | 44 --------------------- lib/tpm_api.c | 23 +++++++++-- 6 files changed, 193 insertions(+), 92 deletions(-) create mode 100644 drivers/rng/tpm1_rng.c create mode 100644 drivers/rng/tpm2_rng.c