linux-kernel - Re: [PATCH] tpm: Make chip->{status,cancel,req

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <exzxzomw7wcobjuoje37x6i2ta54xzx5ho74t3atd7g74xltlb@ymw2pn3yo27b>
Date: Thu, 27 Mar 2025 10:58:00 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Jarkko Sakkinen <jarkko@...nel.org>
Cc: linux-integrity@...r.kernel.org, Sumit Garg <sumit.garg@...nel.org>, 
	Jens Wiklander <jens.wiklander@...aro.org>, James Bottomley <James.Bottomley@...senpartnership.com>, 
	Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>, Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

On Wed, Mar 26, 2025 at 06:18:38PM +0200, Jarkko Sakkinen wrote:
>From: Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>
>
>tpm_ftpm_tee does not require chip->status, chip->cancel and
>chip->req_canceled. Make them optional.
>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>
>---
> drivers/char/tpm/tpm-interface.c | 31 ++++++++++++++++++++++++++++---
> drivers/char/tpm/tpm_ftpm_tee.c  | 20 --------------------
> 2 files changed, 28 insertions(+), 23 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>index f62f7871edbd..10ba47a882d8 100644
>--- a/drivers/char/tpm/tpm-interface.c
>+++ b/drivers/char/tpm/tpm-interface.c
>@@ -58,6 +58,30 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
> }
> EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
>
>+static void tpm_chip_cancel(struct tpm_chip *chip)
>+{
>+	if (!chip->ops->cancel)
>+		return;
>+
>+	chip->ops->cancel(chip);
>+}
>+
>+static u8 tpm_chip_status(struct tpm_chip *chip)
>+{
>+	if (!chip->ops->status)
>+		return 0;
>+
>+	return chip->ops->status(chip);
>+}
>+
>+static bool tpm_chip_req_canceled(struct tpm_chip *chip, u8 status)
>+{
>+	if (!chip->ops->req_canceled)
>+		return false;
>+
>+	return chip->ops->req_canceled(chip, status);
>+}
>+
> static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> {
> 	struct tpm_header *header = buf;
>@@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> 	ssize_t len = 0;
> 	u32 count, ordinal;
> 	unsigned long stop;
>+	u8 status;

Why move `status` out of the do/while block?

>
> 	if (bufsiz < TPM_HEADER_SIZE)
> 		return -EINVAL;
>@@ -104,12 +129,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>

What about doing an early return avoiding also calling
tpm_calc_ordinal_duration()?

I mean something like this:

                 rc = 0;
         }

-       if (chip->flags & TPM_CHIP_FLAG_IRQ)
+       if (!chip->ops->status || chip->flags & TPM_CHIP_FLAG_IRQ)
                 goto out_recv;


Anyway, those are small things, the patch LGTM and it's a great cleanup
for ftpm and the svsm driver I'm developing!


Reviewed-by: Stefano Garzarella <sgarzare@...hat.com>


> 	stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
> 	do {
>-		u8 status = chip->ops->status(chip);
>+		status = tpm_chip_status(chip);
> 		if ((status & chip->ops->req_complete_mask) ==
> 		    chip->ops->req_complete_val)
> 			goto out_recv;
>
>-		if (chip->ops->req_canceled(chip, status)) {
>+		if (tpm_chip_req_canceled(chip, status)) {
> 			dev_err(&chip->dev, "Operation Canceled\n");
> 			return -ECANCELED;
> 		}
>@@ -118,7 +143,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> 		rmb();
> 	} while (time_before(jiffies, stop));
>
>-	chip->ops->cancel(chip);
>+	tpm_chip_cancel(chip);
> 	dev_err(&chip->dev, "Operation Timed out\n");
> 	return -ETIME;
>
>diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
>index 8d9209dfc384..53ba28ccd5d3 100644
>--- a/drivers/char/tpm/tpm_ftpm_tee.c
>+++ b/drivers/char/tpm/tpm_ftpm_tee.c
>@@ -164,30 +164,10 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
> 	return 0;
> }
>
>-static void ftpm_tee_tpm_op_cancel(struct tpm_chip *chip)
>-{
>-	/* not supported */
>-}
>-
>-static u8 ftpm_tee_tpm_op_status(struct tpm_chip *chip)
>-{
>-	return 0;
>-}
>-
>-static bool ftpm_tee_tpm_req_canceled(struct tpm_chip *chip, u8 status)
>-{
>-	return false;
>-}
>-
> static const struct tpm_class_ops ftpm_tee_tpm_ops = {
> 	.flags = TPM_OPS_AUTO_STARTUP,
> 	.recv = ftpm_tee_tpm_op_recv,
> 	.send = ftpm_tee_tpm_op_send,
>-	.cancel = ftpm_tee_tpm_op_cancel,
>-	.status = ftpm_tee_tpm_op_status,
>-	.req_complete_mask = 0,
>-	.req_complete_val = 0,
>-	.req_canceled = ftpm_tee_tpm_req_canceled,
> };
>
> /*
>-- 
>2.39.5
>