| Message ID | 20221209160611.30207-2-James.Bottomley@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add integrity and security to TPM2 transactions | expand |
On Fri, Dec 09, 2022 at 11:06:01AM -0500, James Bottomley wrote: > This separates out the old tpm_buf_... handling functions from static > inlines in tpm.h and makes them their own tpm-buf.c file. This is a > precursor so we can add new functions for other TPM type handling > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> I don't comprehend that explanation at all. Please, add a bit more detail why this precursory change is required. > --- > drivers/char/tpm/Makefile | 1 + > drivers/char/tpm/tpm-buf.c | 96 ++++++++++++++++++++++++++++++++++++++ > include/linux/tpm.h | 86 ++++------------------------------ > 3 files changed, 106 insertions(+), 77 deletions(-) > create mode 100644 drivers/char/tpm/tpm-buf.c > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile > index 0222b1ddb310..ad3594e383e1 100644 > --- a/drivers/char/tpm/Makefile > +++ b/drivers/char/tpm/Makefile > @@ -15,6 +15,7 @@ tpm-y += tpm-sysfs.o > tpm-y += eventlog/common.o > tpm-y += eventlog/tpm1.o > tpm-y += eventlog/tpm2.o > +tpm-y += tpm-buf.o > > tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o > tpm-$(CONFIG_EFI) += eventlog/efi.o > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c > new file mode 100644 > index 000000000000..4e6e8b281f6a > --- /dev/null > +++ b/drivers/char/tpm/tpm-buf.c > @@ -0,0 +1,96 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* > + * Handing for tpm_buf structures to facilitate the building of commands > + */ > + > +#include <linux/module.h> > +#include <linux/tpm.h> > + > +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) > +{ > + buf->data = (u8 *)__get_free_page(GFP_KERNEL); > + if (!buf->data) > + return -ENOMEM; > + > + buf->flags = 0; > + tpm_buf_reset(buf, tag, ordinal); > + return 0; > +} > +EXPORT_SYMBOL_GPL(tpm_buf_init); > + > +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) > +{ > + struct tpm_header *head = (struct tpm_header *) buf->data; > + > + head->tag = cpu_to_be16(tag); > + head->length = cpu_to_be32(sizeof(*head)); > + head->ordinal = cpu_to_be32(ordinal); > +} > +EXPORT_SYMBOL_GPL(tpm_buf_reset); > + > +void tpm_buf_destroy(struct tpm_buf *buf) > +{ > + free_page((unsigned long)buf->data); > +} > +EXPORT_SYMBOL_GPL(tpm_buf_destroy); > + > +u32 tpm_buf_length(struct tpm_buf *buf) > +{ > + struct tpm_header *head = (struct tpm_header *)buf->data; > + > + return be32_to_cpu(head->length); > +} > +EXPORT_SYMBOL_GPL(tpm_buf_length); > + > +u16 tpm_buf_tag(struct tpm_buf *buf) > +{ > + struct tpm_header *head = (struct tpm_header *)buf->data; > + > + return be16_to_cpu(head->tag); > +} > +EXPORT_SYMBOL_GPL(tpm_buf_tag); > + > +void tpm_buf_append(struct tpm_buf *buf, > + const unsigned char *new_data, > + unsigned int new_len) > +{ > + struct tpm_header *head = (struct tpm_header *) buf->data; > + u32 len = tpm_buf_length(buf); > + > + /* Return silently if overflow has already happened. */ > + if (buf->flags & TPM_BUF_OVERFLOW) > + return; > + > + if ((len + new_len) > PAGE_SIZE) { > + WARN(1, "tpm_buf: overflow\n"); > + buf->flags |= TPM_BUF_OVERFLOW; > + return; > + } > + > + memcpy(&buf->data[len], new_data, new_len); > + head->length = cpu_to_be32(len + new_len); > +} > +EXPORT_SYMBOL_GPL(tpm_buf_append); > + > +void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value) > +{ > + tpm_buf_append(buf, &value, 1); > +} > +EXPORT_SYMBOL_GPL(tpm_buf_append_u8); > + > +void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value) > +{ > + __be16 value2 = cpu_to_be16(value); > + > + tpm_buf_append(buf, (u8 *) &value2, 2); > +} > +EXPORT_SYMBOL_GPL(tpm_buf_append_u16); > + > +void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value) > +{ > + __be32 value2 = cpu_to_be32(value); > + > + tpm_buf_append(buf, (u8 *) &value2, 4); > +} > +EXPORT_SYMBOL_GPL(tpm_buf_append_u32); > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index dfeb25a0362d..150b39b6190e 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -322,84 +322,16 @@ struct tpm2_hash { > unsigned int tpm_id; > }; > > -static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) > -{ > - struct tpm_header *head = (struct tpm_header *)buf->data; > - > - head->tag = cpu_to_be16(tag); > - head->length = cpu_to_be32(sizeof(*head)); > - head->ordinal = cpu_to_be32(ordinal); > -} > - > -static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) > -{ > - buf->data = (u8 *)__get_free_page(GFP_KERNEL); > - if (!buf->data) > - return -ENOMEM; > - > - buf->flags = 0; > - tpm_buf_reset(buf, tag, ordinal); > - return 0; > -} > - > -static inline void tpm_buf_destroy(struct tpm_buf *buf) > -{ > - free_page((unsigned long)buf->data); > -} > - > -static inline u32 tpm_buf_length(struct tpm_buf *buf) > -{ > - struct tpm_header *head = (struct tpm_header *)buf->data; > - > - return be32_to_cpu(head->length); > -} > - > -static inline u16 tpm_buf_tag(struct tpm_buf *buf) > -{ > - struct tpm_header *head = (struct tpm_header *)buf->data; > - > - return be16_to_cpu(head->tag); > -} > - > -static inline void tpm_buf_append(struct tpm_buf *buf, > - const unsigned char *new_data, > - unsigned int new_len) > -{ > - struct tpm_header *head = (struct tpm_header *)buf->data; > - u32 len = tpm_buf_length(buf); > - > - /* Return silently if overflow has already happened. */ > - if (buf->flags & TPM_BUF_OVERFLOW) > - return; > - > - if ((len + new_len) > PAGE_SIZE) { > - WARN(1, "tpm_buf: overflow\n"); > - buf->flags |= TPM_BUF_OVERFLOW; > - return; > - } > > - memcpy(&buf->data[len], new_data, new_len); > - head->length = cpu_to_be32(len + new_len); > -} > - > -static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value) > -{ > - tpm_buf_append(buf, &value, 1); > -} > - > -static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value) > -{ > - __be16 value2 = cpu_to_be16(value); > - > - tpm_buf_append(buf, (u8 *) &value2, 2); > -} > - > -static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value) > -{ > - __be32 value2 = cpu_to_be32(value); > - > - tpm_buf_append(buf, (u8 *) &value2, 4); > -} > +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal); > +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal); > +void tpm_buf_destroy(struct tpm_buf *buf); > +u32 tpm_buf_length(struct tpm_buf *buf); > +void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data, > + unsigned int new_len); > +void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value); > +void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value); > +void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value); > > /* > * Check if TPM device is in the firmware upgrade mode. > -- > 2.35.3 > BR, Jarkko
On Sat, 2022-12-10 at 02:09 +0000, Jarkko Sakkinen wrote: > On Fri, Dec 09, 2022 at 11:06:01AM -0500, James Bottomley wrote: > > This separates out the old tpm_buf_... handling functions from > > static > > inlines in tpm.h and makes them their own tpm-buf.c file. This is > > a > > precursor so we can add new functions for other TPM type handling > > > > Signed-off-by: James Bottomley > > <James.Bottomley@HansenPartnership.com> > > I don't comprehend that explanation at all. > > Please, add a bit more detail why this precursory change is required. It's the usual submitting-patches requirement of moving code first before modifying it. Since it's the recommended way of doing things in our process docs, I'm not sure how much more explanation can be given. James
On Sun, Dec 11, 2022 at 03:01:57PM -0500, James Bottomley wrote: > On Sat, 2022-12-10 at 02:09 +0000, Jarkko Sakkinen wrote: > > On Fri, Dec 09, 2022 at 11:06:01AM -0500, James Bottomley wrote: > > > This separates out the old tpm_buf_... handling functions from > > > static > > > inlines in tpm.h and makes them their own tpm-buf.c file. This is > > > a > > > precursor so we can add new functions for other TPM type handling > > > > > > Signed-off-by: James Bottomley > > > <James.Bottomley@HansenPartnership.com> > > > > I don't comprehend that explanation at all. > > > > Please, add a bit more detail why this precursory change is required. > > It's the usual submitting-patches requirement of moving code first > before modifying it. Since it's the recommended way of doing things in > our process docs, I'm not sure how much more explanation can be given. It doesn not contain any reasonable argument for not continue using inline functions. BR, Jarkko
On Mon, 2022-12-12 at 00:52 +0000, Jarkko Sakkinen wrote: > On Sun, Dec 11, 2022 at 03:01:57PM -0500, James Bottomley wrote: > > On Sat, 2022-12-10 at 02:09 +0000, Jarkko Sakkinen wrote: > > > On Fri, Dec 09, 2022 at 11:06:01AM -0500, James Bottomley wrote: > > > > This separates out the old tpm_buf_... handling functions from > > > > static > > > > inlines in tpm.h and makes them their own tpm-buf.c file. This > > > > is > > > > a > > > > precursor so we can add new functions for other TPM type > > > > handling > > > > > > > > Signed-off-by: James Bottomley > > > > <James.Bottomley@HansenPartnership.com> > > > > > > I don't comprehend that explanation at all. > > > > > > Please, add a bit more detail why this precursory change is > > > required. > > > > It's the usual submitting-patches requirement of moving code first > > before modifying it. Since it's the recommended way of doing > > things in our process docs, I'm not sure how much more explanation > > can be given. > > It doesn not contain any reasonable argument for not continue > using inline functions. In principle nothing prevents them being inlines in tpm.h. There's quite a lot of them, so it's growing unweildy and __tpm_buf_init can't be hidden in that scenario but it could, in theory, be done. James
On Sun, Dec 11, 2022 at 10:16:42PM -0500, James Bottomley wrote: > On Mon, 2022-12-12 at 00:52 +0000, Jarkko Sakkinen wrote: > > On Sun, Dec 11, 2022 at 03:01:57PM -0500, James Bottomley wrote: > > > On Sat, 2022-12-10 at 02:09 +0000, Jarkko Sakkinen wrote: > > > > On Fri, Dec 09, 2022 at 11:06:01AM -0500, James Bottomley wrote: > > > > > This separates out the old tpm_buf_... handling functions from > > > > > static > > > > > inlines in tpm.h and makes them their own tpm-buf.c file. This > > > > > is > > > > > a > > > > > precursor so we can add new functions for other TPM type > > > > > handling > > > > > > > > > > Signed-off-by: James Bottomley > > > > > <James.Bottomley@HansenPartnership.com> > > > > > > > > I don't comprehend that explanation at all. > > > > > > > > Please, add a bit more detail why this precursory change is > > > > required. > > > > > > It's the usual submitting-patches requirement of moving code first > > > before modifying it. Since it's the recommended way of doing > > > things in our process docs, I'm not sure how much more explanation > > > can be given. > > > > It doesn not contain any reasonable argument for not continue > > using inline functions. > > In principle nothing prevents them being inlines in tpm.h. There's > quite a lot of them, so it's growing unweildy and __tpm_buf_init can't > be hidden in that scenario but it could, in theory, be done. So: all I'm asking write this down. I'm cool with it, i.e. since complexity will grow heavily because of TPM2B structure handling etc. it is better to encapsulate he implementation, correct? BR, Jarkko
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index 0222b1ddb310..ad3594e383e1 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -15,6 +15,7 @@ tpm-y += tpm-sysfs.o tpm-y += eventlog/common.o tpm-y += eventlog/tpm1.o tpm-y += eventlog/tpm2.o +tpm-y += tpm-buf.o tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o tpm-$(CONFIG_EFI) += eventlog/efi.o diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c new file mode 100644 index 000000000000..4e6e8b281f6a --- /dev/null +++ b/drivers/char/tpm/tpm-buf.c @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Handing for tpm_buf structures to facilitate the building of commands + */ + +#include <linux/module.h> +#include <linux/tpm.h> + +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) +{ + buf->data = (u8 *)__get_free_page(GFP_KERNEL); + if (!buf->data) + return -ENOMEM; + + buf->flags = 0; + tpm_buf_reset(buf, tag, ordinal); + return 0; +} +EXPORT_SYMBOL_GPL(tpm_buf_init); + +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) +{ + struct tpm_header *head = (struct tpm_header *) buf->data; + + head->tag = cpu_to_be16(tag); + head->length = cpu_to_be32(sizeof(*head)); + head->ordinal = cpu_to_be32(ordinal); +} +EXPORT_SYMBOL_GPL(tpm_buf_reset); + +void tpm_buf_destroy(struct tpm_buf *buf) +{ + free_page((unsigned long)buf->data); +} +EXPORT_SYMBOL_GPL(tpm_buf_destroy); + +u32 tpm_buf_length(struct tpm_buf *buf) +{ + struct tpm_header *head = (struct tpm_header *)buf->data; + + return be32_to_cpu(head->length); +} +EXPORT_SYMBOL_GPL(tpm_buf_length); + +u16 tpm_buf_tag(struct tpm_buf *buf) +{ + struct tpm_header *head = (struct tpm_header *)buf->data; + + return be16_to_cpu(head->tag); +} +EXPORT_SYMBOL_GPL(tpm_buf_tag); + +void tpm_buf_append(struct tpm_buf *buf, + const unsigned char *new_data, + unsigned int new_len) +{ + struct tpm_header *head = (struct tpm_header *) buf->data; + u32 len = tpm_buf_length(buf); + + /* Return silently if overflow has already happened. */ + if (buf->flags & TPM_BUF_OVERFLOW) + return; + + if ((len + new_len) > PAGE_SIZE) { + WARN(1, "tpm_buf: overflow\n"); + buf->flags |= TPM_BUF_OVERFLOW; + return; + } + + memcpy(&buf->data[len], new_data, new_len); + head->length = cpu_to_be32(len + new_len); +} +EXPORT_SYMBOL_GPL(tpm_buf_append); + +void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value) +{ + tpm_buf_append(buf, &value, 1); +} +EXPORT_SYMBOL_GPL(tpm_buf_append_u8); + +void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value) +{ + __be16 value2 = cpu_to_be16(value); + + tpm_buf_append(buf, (u8 *) &value2, 2); +} +EXPORT_SYMBOL_GPL(tpm_buf_append_u16); + +void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value) +{ + __be32 value2 = cpu_to_be32(value); + + tpm_buf_append(buf, (u8 *) &value2, 4); +} +EXPORT_SYMBOL_GPL(tpm_buf_append_u32); diff --git a/include/linux/tpm.h b/include/linux/tpm.h index dfeb25a0362d..150b39b6190e 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -322,84 +322,16 @@ struct tpm2_hash { unsigned int tpm_id; }; -static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) -{ - struct tpm_header *head = (struct tpm_header *)buf->data; - - head->tag = cpu_to_be16(tag); - head->length = cpu_to_be32(sizeof(*head)); - head->ordinal = cpu_to_be32(ordinal); -} - -static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) -{ - buf->data = (u8 *)__get_free_page(GFP_KERNEL); - if (!buf->data) - return -ENOMEM; - - buf->flags = 0; - tpm_buf_reset(buf, tag, ordinal); - return 0; -} - -static inline void tpm_buf_destroy(struct tpm_buf *buf) -{ - free_page((unsigned long)buf->data); -} - -static inline u32 tpm_buf_length(struct tpm_buf *buf) -{ - struct tpm_header *head = (struct tpm_header *)buf->data; - - return be32_to_cpu(head->length); -} - -static inline u16 tpm_buf_tag(struct tpm_buf *buf) -{ - struct tpm_header *head = (struct tpm_header *)buf->data; - - return be16_to_cpu(head->tag); -} - -static inline void tpm_buf_append(struct tpm_buf *buf, - const unsigned char *new_data, - unsigned int new_len) -{ - struct tpm_header *head = (struct tpm_header *)buf->data; - u32 len = tpm_buf_length(buf); - - /* Return silently if overflow has already happened. */ - if (buf->flags & TPM_BUF_OVERFLOW) - return; - - if ((len + new_len) > PAGE_SIZE) { - WARN(1, "tpm_buf: overflow\n"); - buf->flags |= TPM_BUF_OVERFLOW; - return; - } - memcpy(&buf->data[len], new_data, new_len); - head->length = cpu_to_be32(len + new_len); -} - -static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value) -{ - tpm_buf_append(buf, &value, 1); -} - -static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value) -{ - __be16 value2 = cpu_to_be16(value); - - tpm_buf_append(buf, (u8 *) &value2, 2); -} - -static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value) -{ - __be32 value2 = cpu_to_be32(value); - - tpm_buf_append(buf, (u8 *) &value2, 4); -} +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal); +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal); +void tpm_buf_destroy(struct tpm_buf *buf); +u32 tpm_buf_length(struct tpm_buf *buf); +void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data, + unsigned int new_len); +void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value); +void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value); +void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value); /* * Check if TPM device is in the firmware upgrade mode.
This separates out the old tpm_buf_... handling functions from static inlines in tpm.h and makes them their own tpm-buf.c file. This is a precursor so we can add new functions for other TPM type handling Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- drivers/char/tpm/Makefile | 1 + drivers/char/tpm/tpm-buf.c | 96 ++++++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 86 ++++------------------------------ 3 files changed, 106 insertions(+), 77 deletions(-) create mode 100644 drivers/char/tpm/tpm-buf.c