[v2,2/3] tpm: Address !chip->auth in tpm_buf_append_name()

Commit Message

Jarkko Sakkinen July 3, 2024, 6:24 p.m. UTC

Unless tpm_chip_bootstrap() was called by the driver, !chip->auth can
cause a null derefence in tpm_buf_append_name().  Thus, address
!chip->auth in tpm_buf_append_name() and remove the fallback
implementation for !TCG_TPM2_HMAC.

Cc: [e-mail] # v6.9+
Reported-by: Stefan Berger <[e-mail]>
Closes: https://lore.kernel.org/linux-integrity/[e-mail]/
Fixes: d0a25bb961e6 ("tpm: Add HMAC session name/handle append")
Signed-off-by: Jarkko Sakkinen <[e-mail]>
---
 drivers/char/tpm/Makefile        |   2 +-
 drivers/char/tpm/tpm2-sessions.c | 205 ++++++++++++++++---------------
 include/linux/tpm.h              |  16 +--
 3 files changed, 113 insertions(+), 110 deletions(-)

Comments

James Bottomley July 3, 2024, 8:11 p.m. UTC | #1

On Wed, 2024-07-03 at 21:24 +0300, Jarkko Sakkinen wrote:
[...]
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 21a67dc9efe8..2844fea4a12a 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -211,8 +211,8 @@ struct tpm_chip {
>         u8 null_key_name[TPM2_NAME_SIZE];
>         u8 null_ec_key_x[EC_PT_SZ];
>         u8 null_ec_key_y[EC_PT_SZ];
> -       struct tpm2_auth *auth;
>  #endif
> +       struct tpm2_auth *auth;
>  };

Since auth should only be present if CONFIG_TCG_TPM2_HMAC this is
clearly an undesirable thing to do.  I think you did it because in a
later patch you want to collapse the hmac sessions to use a single
routine, but you can make that check with the preprocessor __and
function defined in kconfig.h:

if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))

Which will become 0 if the config is not enabled and chip->auth if it
is, thus eliminating the code in the former case while not causing the
compiler to complain about chip->auth not being defined even if it's
under the config parameter.

James

Jarkko Sakkinen July 4, 2024, 6:53 a.m. UTC | #2

On Wed Jul 3, 2024 at 11:11 PM EEST, James Bottomley wrote:
> On Wed, 2024-07-03 at 21:24 +0300, Jarkko Sakkinen wrote:
> [...]
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index 21a67dc9efe8..2844fea4a12a 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -211,8 +211,8 @@ struct tpm_chip {
> >         u8 null_key_name[TPM2_NAME_SIZE];
> >         u8 null_ec_key_x[EC_PT_SZ];
> >         u8 null_ec_key_y[EC_PT_SZ];
> > -       struct tpm2_auth *auth;
> >  #endif
> > +       struct tpm2_auth *auth;
> >  };
>
> Since auth should only be present if CONFIG_TCG_TPM2_HMAC this is
> clearly an undesirable thing to do.  I think you did it because in a
> later patch you want to collapse the hmac sessions to use a single
> routine, but you can make that check with the preprocessor __and
> function defined in kconfig.h:
>
> if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))
>
> Which will become 0 if the config is not enabled and chip->auth if it
> is, thus eliminating the code in the former case while not causing the
> compiler to complain about chip->auth not being defined even if it's
> under the config parameter.

I did not know about '__and()'. Thanks I'll use this!

>
> James

BR, Jarkko

Linus Torvalds July 4, 2024, 5:07 p.m. UTC | #3

On Wed, 3 Jul 2024 at 13:11, James Bottomley
<[e-mail]> wrote:
>
> if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))

Augh. Please don't do this.

That "__and()" thing may work, but it's entirely accidental that it does.

It's designed for config options _only_, and the fact that it then
happens to work for "first argument is config option, second argument
is C conditional".

The comment says that it's implementing "&&" using preprocessor
expansion only, but it's a *really* limited form of it. The arguments
are *not* arbitrary.

So no. Don't do this.

Just create a helper inline like

    static inline struct tpm2_auth *chip_auth(struct tpm_chip *chip)
    {
    #ifdef CONFIG_TCG_TPM2_HMAC
        return chip->auth;
    #else
        return NULL;
    #endif
    }

and if we really want to have some kind of automatic way of doing
this, we will *NOT* be using __and(), we'd do something like

        /* Return zero or 'value' depending on whether OPTION is
enabled or not */
        #define IF_ENABLED(option, value) __and(IS_ENABLED(option), value)

that actually would be documented and meaningful.

Not this internal random __and() implementation that is purely a
kconfig.h helper macro and SHOULD NOT be used anywhere else.

             Linus

James Bottomley July 4, 2024, 5:21 p.m. UTC | #4

On Thu, 2024-07-04 at 10:07 -0700, Linus Torvalds wrote:
> On Wed, 3 Jul 2024 at 13:11, James Bottomley
> <[e-mail]> wrote:
> > 
> > if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))
> 
> Augh. Please don't do this.
> 
> That "__and()" thing may work, but it's entirely accidental that it
> does.
> 
> It's designed for config options _only_, and the fact that it then
> happens to work for "first argument is config option, second argument
> is C conditional".
> 
> The comment says that it's implementing "&&" using preprocessor
> expansion only, but it's a *really* limited form of it. The arguments
> are *not* arbitrary.
> 
> So no. Don't do this.
> 
> Just create a helper inline like
> 
>     static inline struct tpm2_auth *chip_auth(struct tpm_chip *chip)
>     {
>     #ifdef CONFIG_TCG_TPM2_HMAC
>         return chip->auth;
>     #else
>         return NULL;
>     #endif
>     }
> 
> and if we really want to have some kind of automatic way of doing
> this, we will *NOT* be using __and(), we'd do something like
> 
>         /* Return zero or 'value' depending on whether OPTION is
> enabled or not */
>         #define IF_ENABLED(option, value) __and(IS_ENABLED(option),
> value)
> 
> that actually would be documented and meaningful.
> 
> Not this internal random __and() implementation that is purely a
> kconfig.h helper macro and SHOULD NOT be used anywhere else.

I actually like the latter version, but instinct tells me that if this
is the first time the kernel has ever needed something like this then
perhaps we should go with the former because that's how everyone must
have handled it in the past.

James

Jarkko Sakkinen July 4, 2024, 6:05 p.m. UTC | #5

On Thu Jul 4, 2024 at 8:21 PM EEST, James Bottomley wrote:
> On Thu, 2024-07-04 at 10:07 -0700, Linus Torvalds wrote:
> > On Wed, 3 Jul 2024 at 13:11, James Bottomley
> > <[e-mail]> wrote:
> > > 
> > > if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))
> > 
> > Augh. Please don't do this.
> > 
> > That "__and()" thing may work, but it's entirely accidental that it
> > does.
> > 
> > It's designed for config options _only_, and the fact that it then
> > happens to work for "first argument is config option, second argument
> > is C conditional".
> > 
> > The comment says that it's implementing "&&" using preprocessor
> > expansion only, but it's a *really* limited form of it. The arguments
> > are *not* arbitrary.
> > 
> > So no. Don't do this.
> > 
> > Just create a helper inline like
> > 
> >     static inline struct tpm2_auth *chip_auth(struct tpm_chip *chip)
> >     {
> >     #ifdef CONFIG_TCG_TPM2_HMAC
> >         return chip->auth;
> >     #else
> >         return NULL;
> >     #endif
> >     }
> > 
> > and if we really want to have some kind of automatic way of doing
> > this, we will *NOT* be using __and(), we'd do something like
> > 
> >         /* Return zero or 'value' depending on whether OPTION is
> > enabled or not */
> >         #define IF_ENABLED(option, value) __and(IS_ENABLED(option),
> > value)
> > 
> > that actually would be documented and meaningful.
> > 
> > Not this internal random __and() implementation that is purely a
> > kconfig.h helper macro and SHOULD NOT be used anywhere else.
>
> I actually like the latter version, but instinct tells me that if this
> is the first time the kernel has ever needed something like this then
> perhaps we should go with the former because that's how everyone must
> have handled it in the past.

I'll go with the former given it is somewhat idiomatic and familiar
pattern.

> James

BR, Jarkko

Patch

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 4c695b0388f3..9bb142c75243 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -16,8 +16,8 @@  tpm-y += eventlog/common.o
 tpm-y += eventlog/tpm1.o
 tpm-y += eventlog/tpm2.o
 tpm-y += tpm-buf.o
+tpm-y += tpm2-sessions.o
 
-tpm-$(CONFIG_TCG_TPM2_HMAC) += tpm2-sessions.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
 tpm-$(CONFIG_EFI) += eventlog/efi.o
 tpm-$(CONFIG_OF) += eventlog/of.o
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 2f1d96a5a5a7..06d0f10a2301 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -163,6 +163,112 @@  static u8 name_size(const u8 *name)
 	return size_map[alg] + 2;
 }
 
+static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
+{
+	struct tpm_header *head = (struct tpm_header *)buf->data;
+	off_t offset = TPM_HEADER_SIZE;
+	u32 tot_len = be32_to_cpu(head->length);
+	u32 val;
+
+	/* we're starting after the header so adjust the length */
+	tot_len -= TPM_HEADER_SIZE;
+
+	/* skip public */
+	val = tpm_buf_read_u16(buf, &offset);
+	if (val > tot_len)
+		return -EINVAL;
+	offset += val;
+	/* name */
+	val = tpm_buf_read_u16(buf, &offset);
+	if (val != name_size(&buf->data[offset]))
+		return -EINVAL;
+	memcpy(name, &buf->data[offset], val);
+	/* forget the rest */
+	return 0;
+}
+
+static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
+{
+	struct tpm_buf buf;
+	int rc;
+
+	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_READ_PUBLIC);
+	if (rc)
+		return rc;
+
+	tpm_buf_append_u32(&buf, handle);
+	rc = tpm_transmit_cmd(chip, &buf, 0, "read public");
+	if (rc == TPM2_RC_SUCCESS)
+		rc = tpm2_parse_read_public(name, &buf);
+
+	tpm_buf_destroy(&buf);
+
+	return rc;
+}
+
+/**
+ * tpm_buf_append_name() - add a handle area to the buffer
+ * @chip: the TPM chip structure
+ * @buf: The buffer to be appended
+ * @handle: The handle to be appended
+ * @name: The name of the handle (may be NULL)
+ *
+ * In order to compute session HMACs, we need to know the names of the
+ * objects pointed to by the handles.  For most objects, this is simply
+ * the actual 4 byte handle or an empty buf (in these cases @name
+ * should be NULL) but for volatile objects, permanent objects and NV
+ * areas, the name is defined as the hash (according to the name
+ * algorithm which should be set to sha256) of the public area to
+ * which the two byte algorithm id has been appended.  For these
+ * objects, the @name pointer should point to this.  If a name is
+ * required but @name is NULL, then TPM2_ReadPublic() will be called
+ * on the handle to obtain the name.
+ *
+ * As with most tpm_buf operations, success is assumed because failure
+ * will be caused by an incorrect programming model and indicated by a
+ * kernel message.
+ */
+void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
+			 u32 handle, u8 *name)
+{
+	enum tpm2_mso_type mso = tpm2_handle_mso(handle);
+	struct tpm2_auth *auth = chip->auth;
+	int slot;
+
+	if (!chip->auth) {
+		tpm_buf_append_u32(buf, handle);
+		/* count the number of handles in the upper bits of flags */
+		buf->handles++;
+		return;
+	}
+
+	slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE) / 4;
+	if (slot >= AUTH_MAX_NAMES) {
+		dev_err(&chip->dev, "TPM: too many handles\n");
+		return;
+	}
+	WARN(auth->session != tpm_buf_length(buf),
+	     "name added in wrong place\n");
+	tpm_buf_append_u32(buf, handle);
+	auth->session += 4;
+
+	if (mso == TPM2_MSO_PERSISTENT ||
+	    mso == TPM2_MSO_VOLATILE ||
+	    mso == TPM2_MSO_NVRAM) {
+		if (!name)
+			tpm2_read_public(chip, handle, auth->name[slot]);
+	} else {
+		if (name)
+			dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
+	}
+
+	auth->name_h[slot] = handle;
+	if (name)
+		memcpy(auth->name[slot], name, name_size(name));
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_name);
+
+#ifdef CONFIG_TCG_TPM2_HMAC
 /*
  * It turns out the crypto hmac(sha256) is hard for us to consume
  * because it assumes a fixed key and the TPM seems to change the key
@@ -567,104 +673,6 @@  void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
 }
 EXPORT_SYMBOL(tpm_buf_fill_hmac_session);
 
-static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
-{
-	struct tpm_header *head = (struct tpm_header *)buf->data;
-	off_t offset = TPM_HEADER_SIZE;
-	u32 tot_len = be32_to_cpu(head->length);
-	u32 val;
-
-	/* we're starting after the header so adjust the length */
-	tot_len -= TPM_HEADER_SIZE;
-
-	/* skip public */
-	val = tpm_buf_read_u16(buf, &offset);
-	if (val > tot_len)
-		return -EINVAL;
-	offset += val;
-	/* name */
-	val = tpm_buf_read_u16(buf, &offset);
-	if (val != name_size(&buf->data[offset]))
-		return -EINVAL;
-	memcpy(name, &buf->data[offset], val);
-	/* forget the rest */
-	return 0;
-}
-
-static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
-{
-	struct tpm_buf buf;
-	int rc;
-
-	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_READ_PUBLIC);
-	if (rc)
-		return rc;
-
-	tpm_buf_append_u32(&buf, handle);
-	rc = tpm_transmit_cmd(chip, &buf, 0, "read public");
-	if (rc == TPM2_RC_SUCCESS)
-		rc = tpm2_parse_read_public(name, &buf);
-
-	tpm_buf_destroy(&buf);
-
-	return rc;
-}
-
-/**
- * tpm_buf_append_name() - add a handle area to the buffer
- * @chip: the TPM chip structure
- * @buf: The buffer to be appended
- * @handle: The handle to be appended
- * @name: The name of the handle (may be NULL)
- *
- * In order to compute session HMACs, we need to know the names of the
- * objects pointed to by the handles.  For most objects, this is simply
- * the actual 4 byte handle or an empty buf (in these cases @name
- * should be NULL) but for volatile objects, permanent objects and NV
- * areas, the name is defined as the hash (according to the name
- * algorithm which should be set to sha256) of the public area to
- * which the two byte algorithm id has been appended.  For these
- * objects, the @name pointer should point to this.  If a name is
- * required but @name is NULL, then TPM2_ReadPublic() will be called
- * on the handle to obtain the name.
- *
- * As with most tpm_buf operations, success is assumed because failure
- * will be caused by an incorrect programming model and indicated by a
- * kernel message.
- */
-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
-			 u32 handle, u8 *name)
-{
-	enum tpm2_mso_type mso = tpm2_handle_mso(handle);
-	struct tpm2_auth *auth = chip->auth;
-	int slot;
-
-	slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE)/4;
-	if (slot >= AUTH_MAX_NAMES) {
-		dev_err(&chip->dev, "TPM: too many handles\n");
-		return;
-	}
-	WARN(auth->session != tpm_buf_length(buf),
-	     "name added in wrong place\n");
-	tpm_buf_append_u32(buf, handle);
-	auth->session += 4;
-
-	if (mso == TPM2_MSO_PERSISTENT ||
-	    mso == TPM2_MSO_VOLATILE ||
-	    mso == TPM2_MSO_NVRAM) {
-		if (!name)
-			tpm2_read_public(chip, handle, auth->name[slot]);
-	} else {
-		if (name)
-			dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
-	}
-
-	auth->name_h[slot] = handle;
-	if (name)
-		memcpy(auth->name[slot], name, name_size(name));
-}
-EXPORT_SYMBOL(tpm_buf_append_name);
-
 /**
  * tpm_buf_check_hmac_response() - check the TPM return HMAC for correctness
  * @chip: the TPM chip structure
@@ -1311,3 +1319,4 @@  int tpm2_sessions_init(struct tpm_chip *chip)
 
 	return rc;
 }
+#endif /* CONFIG_TCG_TPM2_HMAC */
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 21a67dc9efe8..2844fea4a12a 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -211,8 +211,8 @@  struct tpm_chip {
 	u8 null_key_name[TPM2_NAME_SIZE];
 	u8 null_ec_key_x[EC_PT_SZ];
 	u8 null_ec_key_y[EC_PT_SZ];
-	struct tpm2_auth *auth;
 #endif
+	struct tpm2_auth *auth;
 };
 
 #define TPM_HEADER_SIZE		10
@@ -490,11 +490,13 @@  static inline void tpm_buf_append_empty_auth(struct tpm_buf *buf, u32 handle)
 {
 }
 #endif
-#ifdef CONFIG_TCG_TPM2_HMAC
 
-int tpm2_start_auth_session(struct tpm_chip *chip);
 void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
 			 u32 handle, u8 *name);
+
+#ifdef CONFIG_TCG_TPM2_HMAC
+
+int tpm2_start_auth_session(struct tpm_chip *chip);
 void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
 				 u8 attributes, u8 *passphrase,
 				 int passphraselen);
@@ -521,14 +523,6 @@  static inline int tpm2_start_auth_session(struct tpm_chip *chip)
 static inline void tpm2_end_auth_session(struct tpm_chip *chip)
 {
 }
-static inline void tpm_buf_append_name(struct tpm_chip *chip,
-				       struct tpm_buf *buf,
-				       u32 handle, u8 *name)
-{
-	tpm_buf_append_u32(buf, handle);
-	/* count the number of handles in the upper bits of flags */
-	buf->handles++;
-}
 static inline void tpm_buf_append_hmac_session(struct tpm_chip *chip,
 					       struct tpm_buf *buf,
 					       u8 attributes, u8 *passphrase,