| Message ID | 20240716185225.873090-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] tpm: Relocate buf->handles to appropriate place | expand |
On Tue Jul 16, 2024 at 9:52 PM EEST, Jarkko Sakkinen wrote: > tpm_buf_append_name() has the following snippet in the beginning: > > if (!tpm2_chip_auth(chip)) { > tpm_buf_append_u32(buf, handle); > /* count the number of handles in the upper bits of flags */ > buf->handles++; > return; > } > > The claim in the comment is wrong, and the comment is in the wrong place > as alignment in this case should not anyway be a concern of the call > site. In essence the comment is lying about the code, and thus needs to > be adressed. > > Further, 'handles' was incorrectly place to struct tpm_buf, as tpm-buf.c > does manage its state. It is easy to grep that only piece of code that > actually uses the field is tpm2-sessions.c. > > Address the issues by moving the variable to struct tpm_chip. > > Cc: stable@vger.kernel.org # v6.10+ > Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions") > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > Dashes missing but I can fix that when I apply this. Just like to keep change log in git and I add the dashes before sending... > v3: > * Reset chip->handles in the beginning of tpm2_start_auth_session() > so that it shows correct value, when TCG_TPM2_HMAC is enabled but > tpm2_sessions_init() has never been called. > v2: > * Was a bit more broken than I first thought, as 'handles' is only > useful for tpm2-sessions.c and has zero relation to tpm-buf.c. > --- BR, Jarkko
On Tue, 2024-07-16 at 21:52 +0300, Jarkko Sakkinen wrote: [...] > Further, 'handles' was incorrectly place to struct tpm_buf, as tpm- > buf.c does manage its state. It is easy to grep that only piece of > code that actually uses the field is tpm2-sessions.c. > > Address the issues by moving the variable to struct tpm_chip. That's really not a good idea, you should keep counts local to the structures they're counting, not elsewhere. tpm_buf->handles counts the number of handles present in the command encoded in a particular tpm_buf. Right at the moment we only ever construct one tpm_buf per tpm (i.e. per tpm_chip) at any one time, so you can get away with moving handles into tpm_chip. If we ever constructed more than one tpm_buf per chip, the handles count would become corrupted. James
On Tue, 2024-07-16 at 15:32 -0400, James Bottomley wrote: > On Tue, 2024-07-16 at 21:52 +0300, Jarkko Sakkinen wrote: > [...] > > Further, 'handles' was incorrectly place to struct tpm_buf, as tpm- > > buf.c does manage its state. It is easy to grep that only piece of > > code that actually uses the field is tpm2-sessions.c. > > > > Address the issues by moving the variable to struct tpm_chip. > > That's really not a good idea, you should keep counts local to the > structures they're counting, not elsewhere. > > tpm_buf->handles counts the number of handles present in the command > encoded in a particular tpm_buf. Right at the moment we only ever > construct one tpm_buf per tpm (i.e. per tpm_chip) at any one time, so > you can get away with moving handles into tpm_chip. If we ever > constructed more than one tpm_buf per chip, the handles count would > become corrupted. It is not an idea. That count is in the wrong place. Buffer code has no use for it. BR, Jarkko
On Wed, 2024-07-17 at 12:27 +0300, Jarkko Sakkinen wrote: > On Tue, 2024-07-16 at 15:32 -0400, James Bottomley wrote: > > On Tue, 2024-07-16 at 21:52 +0300, Jarkko Sakkinen wrote: > > [...] > > > Further, 'handles' was incorrectly place to struct tpm_buf, as tpm- > > > buf.c does manage its state. It is easy to grep that only piece of > > > code that actually uses the field is tpm2-sessions.c. > > > > > > Address the issues by moving the variable to struct tpm_chip. > > > > That's really not a good idea, you should keep counts local to the > > structures they're counting, not elsewhere. > > > > tpm_buf->handles counts the number of handles present in the command > > encoded in a particular tpm_buf. Right at the moment we only ever > > construct one tpm_buf per tpm (i.e. per tpm_chip) at any one time, so > > you can get away with moving handles into tpm_chip. If we ever > > constructed more than one tpm_buf per chip, the handles count would > > become corrupted. > > It is not an idea. That count is in the wrong place. Buffer code > has no use for it. Also you are misleading here again. Depending on context tpm_buf stores different data, including handles. BR, Jarkko
On Wed, 2024-07-17 at 12:31 +0300, Jarkko Sakkinen wrote: > On Wed, 2024-07-17 at 12:27 +0300, Jarkko Sakkinen wrote: > > On Tue, 2024-07-16 at 15:32 -0400, James Bottomley wrote: > > > On Tue, 2024-07-16 at 21:52 +0300, Jarkko Sakkinen wrote: > > > [...] > > > > Further, 'handles' was incorrectly place to struct tpm_buf, as tpm- > > > > buf.c does manage its state. It is easy to grep that only piece of > > > > code that actually uses the field is tpm2-sessions.c. > > > > > > > > Address the issues by moving the variable to struct tpm_chip. > > > > > > That's really not a good idea, you should keep counts local to the > > > structures they're counting, not elsewhere. > > > > > > tpm_buf->handles counts the number of handles present in the command > > > encoded in a particular tpm_buf. Right at the moment we only ever > > > construct one tpm_buf per tpm (i.e. per tpm_chip) at any one time, so > > > you can get away with moving handles into tpm_chip. If we ever > > > constructed more than one tpm_buf per chip, the handles count would > > > become corrupted. > > > > It is not an idea. That count is in the wrong place. Buffer code > > has no use for it. > > Also you are misleading here again. Depending on context tpm_buf > stores different data, including handles. These false claims can be also proved wrong by trivial git grep, which clearly shows its scope. BR, Jarkko
On Tue, Jul 16, 2024 at 09:52:24PM +0300, Jarkko Sakkinen wrote: > tpm_buf_append_name() has the following snippet in the beginning: > > if (!tpm2_chip_auth(chip)) { > tpm_buf_append_u32(buf, handle); > /* count the number of handles in the upper bits of flags */ > buf->handles++; > return; > } > > The claim in the comment is wrong, and the comment is in the wrong place > as alignment in this case should not anyway be a concern of the call > site. In essence the comment is lying about the code, and thus needs to > be adressed. > > Further, 'handles' was incorrectly place to struct tpm_buf, as tpm-buf.c > does manage its state. It is easy to grep that only piece of code that > actually uses the field is tpm2-sessions.c. > > Address the issues by moving the variable to struct tpm_chip. > > Cc: stable@vger.kernel.org # v6.10+ > Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions") > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > v3: > * Reset chip->handles in the beginning of tpm2_start_auth_session() > so that it shows correct value, when TCG_TPM2_HMAC is enabled but > tpm2_sessions_init() has never been called. > v2: > * Was a bit more broken than I first thought, as 'handles' is only > useful for tpm2-sessions.c and has zero relation to tpm-buf.c. > --- > drivers/char/tpm/tpm-buf.c | 1 - > drivers/char/tpm/tpm2-cmd.c | 2 +- > drivers/char/tpm/tpm2-sessions.c | 7 ++++--- > include/linux/tpm.h | 8 ++++---- > 4 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c > index cad0048bcc3c..d06e8e063151 100644 > --- a/drivers/char/tpm/tpm-buf.c > +++ b/drivers/char/tpm/tpm-buf.c > @@ -44,7 +44,6 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) > head->tag = cpu_to_be16(tag); > head->length = cpu_to_be32(sizeof(*head)); > head->ordinal = cpu_to_be32(ordinal); > - buf->handles = 0; > } > EXPORT_SYMBOL_GPL(tpm_buf_reset); > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index 1e856259219e..b781e4406fc2 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -776,7 +776,7 @@ int tpm2_auto_startup(struct tpm_chip *chip) > if (rc) > goto out; > > - rc = tpm2_sessions_init(chip); > + /* rc = tpm2_sessions_init(chip); */ Left over from testing? Or should be removed entirely? > out: > /* > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c > index d3521aadd43e..5e7c12d64ba8 100644 > --- a/drivers/char/tpm/tpm2-sessions.c > +++ b/drivers/char/tpm/tpm2-sessions.c > @@ -238,8 +238,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf, > > if (!tpm2_chip_auth(chip)) { > tpm_buf_append_u32(buf, handle); > - /* count the number of handles in the upper bits of flags */ > - buf->handles++; > + chip->handles++; > return; > } > > @@ -310,7 +309,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf, > > if (!tpm2_chip_auth(chip)) { > /* offset tells us where the sessions area begins */ > - int offset = buf->handles * 4 + TPM_HEADER_SIZE; > + int offset = chip->handles * 4 + TPM_HEADER_SIZE; > u32 len = 9 + passphrase_len; > > if (tpm_buf_length(buf) != offset) { > @@ -963,6 +962,8 @@ int tpm2_start_auth_session(struct tpm_chip *chip) > int rc; > u32 null_key; > > + chip->handles = 0; > + > if (!auth) { > dev_warn_once(&chip->dev, "auth session is not active\n"); > return 0; > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index e93ee8d936a9..b664f7556494 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -202,9 +202,9 @@ struct tpm_chip { > /* active locality */ > int locality; > > + /* handle count for session: */ > + u8 handles; > #ifdef CONFIG_TCG_TPM2_HMAC > - /* details for communication security via sessions */ > - > /* saved context for NULL seed */ > u8 null_key_context[TPM2_MAX_CONTEXT_SIZE]; > /* name of NULL seed */ > @@ -377,7 +377,6 @@ struct tpm_buf { > u32 flags; > u32 length; > u8 *data; > - u8 handles; > }; > > enum tpm2_object_attributes { > @@ -517,7 +516,7 @@ static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip, > if (tpm2_chip_auth(chip)) { > tpm_buf_append_hmac_session(chip, buf, attributes, passphrase, passphraselen); > } else { > - offset = buf->handles * 4 + TPM_HEADER_SIZE; > + offset = chip->handles * 4 + TPM_HEADER_SIZE; > head = (struct tpm_header *)buf->data; > > /* > @@ -541,6 +540,7 @@ void tpm2_end_auth_session(struct tpm_chip *chip); > > static inline int tpm2_start_auth_session(struct tpm_chip *chip) > { > + chip->handles = 0; > return 0; > } > static inline void tpm2_end_auth_session(struct tpm_chip *chip) > -- > 2.45.2 J.
On Wed Jul 17, 2024 at 12:55 PM EEST, Jonathan McDowell wrote:
> Left over from testing? Or should be removed entirely?
Yes. You're right. I noticed it too yesterday, but I'll probably
postpone this patch after holidays and since it is only cosmetic
fix, it can be included to any feature patch set for hmac.
Thanks for the remark however! I'll revisit these when the patch
is needed.
BR, Jarkko
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c index cad0048bcc3c..d06e8e063151 100644 --- a/drivers/char/tpm/tpm-buf.c +++ b/drivers/char/tpm/tpm-buf.c @@ -44,7 +44,6 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) head->tag = cpu_to_be16(tag); head->length = cpu_to_be32(sizeof(*head)); head->ordinal = cpu_to_be32(ordinal); - buf->handles = 0; } EXPORT_SYMBOL_GPL(tpm_buf_reset); diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index 1e856259219e..b781e4406fc2 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -776,7 +776,7 @@ int tpm2_auto_startup(struct tpm_chip *chip) if (rc) goto out; - rc = tpm2_sessions_init(chip); + /* rc = tpm2_sessions_init(chip); */ out: /* diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c index d3521aadd43e..5e7c12d64ba8 100644 --- a/drivers/char/tpm/tpm2-sessions.c +++ b/drivers/char/tpm/tpm2-sessions.c @@ -238,8 +238,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf, if (!tpm2_chip_auth(chip)) { tpm_buf_append_u32(buf, handle); - /* count the number of handles in the upper bits of flags */ - buf->handles++; + chip->handles++; return; } @@ -310,7 +309,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf, if (!tpm2_chip_auth(chip)) { /* offset tells us where the sessions area begins */ - int offset = buf->handles * 4 + TPM_HEADER_SIZE; + int offset = chip->handles * 4 + TPM_HEADER_SIZE; u32 len = 9 + passphrase_len; if (tpm_buf_length(buf) != offset) { @@ -963,6 +962,8 @@ int tpm2_start_auth_session(struct tpm_chip *chip) int rc; u32 null_key; + chip->handles = 0; + if (!auth) { dev_warn_once(&chip->dev, "auth session is not active\n"); return 0; diff --git a/include/linux/tpm.h b/include/linux/tpm.h index e93ee8d936a9..b664f7556494 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -202,9 +202,9 @@ struct tpm_chip { /* active locality */ int locality; + /* handle count for session: */ + u8 handles; #ifdef CONFIG_TCG_TPM2_HMAC - /* details for communication security via sessions */ - /* saved context for NULL seed */ u8 null_key_context[TPM2_MAX_CONTEXT_SIZE]; /* name of NULL seed */ @@ -377,7 +377,6 @@ struct tpm_buf { u32 flags; u32 length; u8 *data; - u8 handles; }; enum tpm2_object_attributes { @@ -517,7 +516,7 @@ static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip, if (tpm2_chip_auth(chip)) { tpm_buf_append_hmac_session(chip, buf, attributes, passphrase, passphraselen); } else { - offset = buf->handles * 4 + TPM_HEADER_SIZE; + offset = chip->handles * 4 + TPM_HEADER_SIZE; head = (struct tpm_header *)buf->data; /* @@ -541,6 +540,7 @@ void tpm2_end_auth_session(struct tpm_chip *chip); static inline int tpm2_start_auth_session(struct tpm_chip *chip) { + chip->handles = 0; return 0; } static inline void tpm2_end_auth_session(struct tpm_chip *chip)
tpm_buf_append_name() has the following snippet in the beginning: if (!tpm2_chip_auth(chip)) { tpm_buf_append_u32(buf, handle); /* count the number of handles in the upper bits of flags */ buf->handles++; return; } The claim in the comment is wrong, and the comment is in the wrong place as alignment in this case should not anyway be a concern of the call site. In essence the comment is lying about the code, and thus needs to be adressed. Further, 'handles' was incorrectly place to struct tpm_buf, as tpm-buf.c does manage its state. It is easy to grep that only piece of code that actually uses the field is tpm2-sessions.c. Address the issues by moving the variable to struct tpm_chip. Cc: stable@vger.kernel.org # v6.10+ Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions") Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> v3: * Reset chip->handles in the beginning of tpm2_start_auth_session() so that it shows correct value, when TCG_TPM2_HMAC is enabled but tpm2_sessions_init() has never been called. v2: * Was a bit more broken than I first thought, as 'handles' is only useful for tpm2-sessions.c and has zero relation to tpm-buf.c. --- drivers/char/tpm/tpm-buf.c | 1 - drivers/char/tpm/tpm2-cmd.c | 2 +- drivers/char/tpm/tpm2-sessions.c | 7 ++++--- include/linux/tpm.h | 8 ++++---- 4 files changed, 9 insertions(+), 9 deletions(-)