| Message ID | 20230403214003.32093-7-James.Bottomley@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add integrity and security to TPM2 transactions | expand |
On 4/3/23 17:39, James Bottomley wrote: > Introducing encryption sessions changes where the return parameters > are located in the buffer because if a return session is present > they're 4 bytes beyond the header with those 4 bytes showing the > parameter length. If there is no return session, then they're in the > usual place immediately after the header. The tpm_buf_parameters() > encapsulates this calculation and should be used everywhere > &buf.data[TPM_HEADER_SIZE] is used now. > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > --- > v4: add kdoc > --- > drivers/char/tpm/tpm-buf.c | 27 +++++++++++++++++++++++++++ > include/linux/tpm.h | 2 ++ > 2 files changed, 29 insertions(+) > > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c > index da0f6e725c3f..d107321bcdff 100644 > --- a/drivers/char/tpm/tpm-buf.c > +++ b/drivers/char/tpm/tpm-buf.c > @@ -277,3 +277,30 @@ u32 tpm_get_inc_u32(const u8 **ptr) > return val; > } > EXPORT_SYMBOL_GPL(tpm_get_inc_u32); > + > +static u16 tpm_buf_tag(struct tpm_buf *buf) > +{ > + struct tpm_header *head = (struct tpm_header *)buf->data; > + > + return be16_to_cpu(head->tag); > +} > + > +/** > + * tpm_buf_parameters - return the parameters area of the tpm_buf in a TPM response > + * @buf: tpm_buf to use > + * > + * Where the parameters are located depends on the tag of a TPM > + * command. Evaluate this and return a pointer to the first byte of > + * the parameters area. I would also describe here what the non-obvious 4 bytes are that you are skipping over in case of TPM2_ST_SESSIONS. With this: Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > + * > + * @return: pointer to parameters area > + */ > +u8 *tpm_buf_parameters(struct tpm_buf *buf) > +{ > + int offset = TPM_HEADER_SIZE; > + > + if (tpm_buf_tag(buf) == TPM2_ST_SESSIONS) > + offset += 4; > + > + return &buf->data[offset]; > +} > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index 845eadfed715..d2fea2afd37c 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -340,6 +340,8 @@ u8 tpm_get_inc_u8(const u8 **ptr); > u16 tpm_get_inc_u16(const u8 **ptr); > u32 tpm_get_inc_u32(const u8 **ptr); > > +u8 *tpm_buf_parameters(struct tpm_buf *buf); > + > /* > * Check if TPM device is in the firmware upgrade mode. > */
On Tue May 2, 2023 at 5:09 PM EEST, Stefan Berger wrote: > > > On 4/3/23 17:39, James Bottomley wrote: > > Introducing encryption sessions changes where the return parameters s/Introducing/Introduce/ Commit messages should always be in the imperative form. > > are located in the buffer because if a return session is present > > they're 4 bytes beyond the header with those 4 bytes showing the > > parameter length. If there is no return session, then they're in the > > usual place immediately after the header. The tpm_buf_parameters() > > encapsulates this calculation and should be used everywhere > > &buf.data[TPM_HEADER_SIZE] is used now. > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > --- > > v4: add kdoc > > --- > > drivers/char/tpm/tpm-buf.c | 27 +++++++++++++++++++++++++++ > > include/linux/tpm.h | 2 ++ > > 2 files changed, 29 insertions(+) > > > > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c > > index da0f6e725c3f..d107321bcdff 100644 > > --- a/drivers/char/tpm/tpm-buf.c > > +++ b/drivers/char/tpm/tpm-buf.c > > @@ -277,3 +277,30 @@ u32 tpm_get_inc_u32(const u8 **ptr) > > return val; > > } > > EXPORT_SYMBOL_GPL(tpm_get_inc_u32); > > + > > +static u16 tpm_buf_tag(struct tpm_buf *buf) > > +{ > > + struct tpm_header *head = (struct tpm_header *)buf->data; > > + > > + return be16_to_cpu(head->tag); > > +} > > + > > +/** > > + * tpm_buf_parameters - return the parameters area of the tpm_buf > > in a TPM response > > > > + * @buf: tpm_buf to use > > + * > > + * Where the parameters are located depends on the tag of a TPM > > + * command. Evaluate this and return a pointer to the first byte of > > + * the parameters area. > > I would also describe here what the non-obvious 4 bytes are that you are skipping over in case of TPM2_ST_SESSIONS. > > With this: > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > > > + * > > + * @return: pointer to parameters area > > + */ > > +u8 *tpm_buf_parameters(struct tpm_buf *buf) > > +{ > > + int offset = TPM_HEADER_SIZE; > > + > > + if (tpm_buf_tag(buf) == TPM2_ST_SESSIONS) > > + offset += 4; > > + > > + return &buf->data[offset]; > > +} > > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > > index 845eadfed715..d2fea2afd37c 100644 > > --- a/include/linux/tpm.h > > +++ b/include/linux/tpm.h > > @@ -340,6 +340,8 @@ u8 tpm_get_inc_u8(const u8 **ptr); > > u16 tpm_get_inc_u16(const u8 **ptr); > > u32 tpm_get_inc_u32(const u8 **ptr); > > > > +u8 *tpm_buf_parameters(struct tpm_buf *buf); > > + > > /* > > * Check if TPM device is in the firmware upgrade mode. > > */ BR, Jarkko
On Wed, 2023-05-03 at 14:31 +0300, Jarkko Sakkinen wrote: > On Tue May 2, 2023 at 5:09 PM EEST, Stefan Berger wrote: > > > > > > On 4/3/23 17:39, James Bottomley wrote: > > > Introducing encryption sessions changes where the return > > > parameters > > s/Introducing/Introduce/ > > Commit messages should always be in the imperative form. "Introducing" in this sentence is a gerund (verb used as a noun); it can't be changed to an imperative because it's not being used as a direct verb in the sentence (that honour goes to "changes", which also can't be made imperative because the gerund is the subject). I can reword it like this if you want the sentence to begin with an imperative (and get rid of the gerund before Linus bites my head off again for using obscure grammatical constructions): "Replace all instances of &buf.data[TPM_HEADER_SIZE] with a new function tpm_buf_parameters() because encryption sessions change where the return parameters are located in the buffer since if a return session is present they're 4 bytes beyond the header with those 4 bytes giving the parameter length. If there is no return session, then they're in the usual place immediately after the header." James
On Tue Jun 6, 2023 at 5:09 AM EEST, James Bottomley wrote: > On Wed, 2023-05-03 at 14:31 +0300, Jarkko Sakkinen wrote: > > On Tue May 2, 2023 at 5:09 PM EEST, Stefan Berger wrote: > > > > > > > > > On 4/3/23 17:39, James Bottomley wrote: > > > > Introducing encryption sessions changes where the return > > > > parameters > > > > s/Introducing/Introduce/ > > > > Commit messages should always be in the imperative form. > > "Introducing" in this sentence is a gerund (verb used as a noun); it > can't be changed to an imperative because it's not being used as a > direct verb in the sentence (that honour goes to "changes", which also > can't be made imperative because the gerund is the subject). I can > reword it like this if you want the sentence to begin with an > imperative (and get rid of the gerund before Linus bites my head off > again for using obscure grammatical constructions): > > "Replace all instances of &buf.data[TPM_HEADER_SIZE] with a new > function tpm_buf_parameters() because encryption sessions change > where the return parameters are located in the buffer since if a > return session is present they're 4 bytes beyond the header with those > 4 bytes giving the parameter length. If there is no return session, > then they're in the usual place immediately after the header." I'm planning to write a small (RFC) patch set just for the tpm_buf portion because it is the part that does not work for me. What builds on top of that looks decent, or will converge to decent. I have some ideas that have building up in my head so I'll just dump that as source code and see if that works for you (or not). BR, Jarkko
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c index da0f6e725c3f..d107321bcdff 100644 --- a/drivers/char/tpm/tpm-buf.c +++ b/drivers/char/tpm/tpm-buf.c @@ -277,3 +277,30 @@ u32 tpm_get_inc_u32(const u8 **ptr) return val; } EXPORT_SYMBOL_GPL(tpm_get_inc_u32); + +static u16 tpm_buf_tag(struct tpm_buf *buf) +{ + struct tpm_header *head = (struct tpm_header *)buf->data; + + return be16_to_cpu(head->tag); +} + +/** + * tpm_buf_parameters - return the parameters area of the tpm_buf + * @buf: tpm_buf to use + * + * Where the parameters are located depends on the tag of a TPM + * command. Evaluate this and return a pointer to the first byte of + * the parameters area. + * + * @return: pointer to parameters area + */ +u8 *tpm_buf_parameters(struct tpm_buf *buf) +{ + int offset = TPM_HEADER_SIZE; + + if (tpm_buf_tag(buf) == TPM2_ST_SESSIONS) + offset += 4; + + return &buf->data[offset]; +} diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 845eadfed715..d2fea2afd37c 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -340,6 +340,8 @@ u8 tpm_get_inc_u8(const u8 **ptr); u16 tpm_get_inc_u16(const u8 **ptr); u32 tpm_get_inc_u32(const u8 **ptr); +u8 *tpm_buf_parameters(struct tpm_buf *buf); + /* * Check if TPM device is in the firmware upgrade mode. */
Introducing encryption sessions changes where the return parameters are located in the buffer because if a return session is present they're 4 bytes beyond the header with those 4 bytes showing the parameter length. If there is no return session, then they're in the usual place immediately after the header. The tpm_buf_parameters() encapsulates this calculation and should be used everywhere &buf.data[TPM_HEADER_SIZE] is used now. Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- v4: add kdoc --- drivers/char/tpm/tpm-buf.c | 27 +++++++++++++++++++++++++++ include/linux/tpm.h | 2 ++ 2 files changed, 29 insertions(+)