* Re: [PATCH] tpm_tis: Fix undefined behavior in tpm_tis_spi_transfer_half()
2025-09-17 15:29 [PATCH] tpm_tis: Fix undefined behavior in tpm_tis_spi_transfer_half() Gunnar Kudrjavets
@ 2025-09-17 22:13 ` Paul Menzel
2025-09-18 8:55 ` Stefano Garzarella
2025-09-18 15:54 ` Jarkko Sakkinen
2 siblings, 0 replies; 4+ messages in thread
From: Paul Menzel @ 2025-09-17 22:13 UTC (permalink / raw)
To: Gunnar Kudrjavets
Cc: peterhuewe, jarkko, jgg, kyarlagadda, linux-integrity,
linux-kernel, Justinien Bouron
Dear Gunnar,
Thank you very much for your patch.
Am 17.09.25 um 17:29 schrieb Gunnar Kudrjavets:
> When len is 0, the while loop in tpm_tis_spi_transfer_half() never
> executes, leaving ret uninitialized. This will lead to undefined
> behavior when the function returns.
>
> The issue was introduced when tpm_tis_spi_transfer() was refactored
> to call tpm_tis_spi_transfer_half() or tpm_tis_spi_transfer_full().
> While ret is properly initialized in tpm_tis_spi_transfer_full(), it
> was missed in tpm_tis_spi_transfer_half().
>
> Initialize ret to 0 at the beginning of the function to ensure
> defined behavior in all code paths.
>
> Found by GCC 14.2.0 static analyzer with -fanalyzer.
>
> Fixes: a86a42ac2bd6 ("tpm_tis_spi: Add hardware wait polling")
> Signed-off-by: Gunnar Kudrjavets <gunnarku@amazon.com>
> Reviewed-by: Justinien Bouron <jbouron@amazon.com>
> ---
> drivers/char/tpm/tpm_tis_spi_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
> index 61b42c83ced8..1b6d79662ca1 100644
> --- a/drivers/char/tpm/tpm_tis_spi_main.c
> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> @@ -85,7 +85,7 @@ static int tpm_tis_spi_transfer_half(struct tpm_tis_data *data, u32 addr,
> struct spi_transfer spi_xfer[3];
> struct spi_message m;
> u8 transfer_len;
> - int ret;
> + int ret = 0;
>
> while (len) {
> transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Kind regards,
Paul
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] tpm_tis: Fix undefined behavior in tpm_tis_spi_transfer_half()
2025-09-17 15:29 [PATCH] tpm_tis: Fix undefined behavior in tpm_tis_spi_transfer_half() Gunnar Kudrjavets
2025-09-17 22:13 ` Paul Menzel
@ 2025-09-18 8:55 ` Stefano Garzarella
2025-09-18 15:54 ` Jarkko Sakkinen
2 siblings, 0 replies; 4+ messages in thread
From: Stefano Garzarella @ 2025-09-18 8:55 UTC (permalink / raw)
To: Gunnar Kudrjavets
Cc: peterhuewe, jarkko, jgg, kyarlagadda, linux-integrity,
linux-kernel, Justinien Bouron
On Wed, Sep 17, 2025 at 03:29:56PM +0000, Gunnar Kudrjavets wrote:
>When len is 0, the while loop in tpm_tis_spi_transfer_half() never
>executes, leaving ret uninitialized. This will lead to undefined
>behavior when the function returns.
>
>The issue was introduced when tpm_tis_spi_transfer() was refactored
>to call tpm_tis_spi_transfer_half() or tpm_tis_spi_transfer_full().
>While ret is properly initialized in tpm_tis_spi_transfer_full(), it
>was missed in tpm_tis_spi_transfer_half().
>
>Initialize ret to 0 at the beginning of the function to ensure
>defined behavior in all code paths.
>
>Found by GCC 14.2.0 static analyzer with -fanalyzer.
>
>Fixes: a86a42ac2bd6 ("tpm_tis_spi: Add hardware wait polling")
>Signed-off-by: Gunnar Kudrjavets <gunnarku@amazon.com>
>Reviewed-by: Justinien Bouron <jbouron@amazon.com>
>---
> drivers/char/tpm/tpm_tis_spi_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
>index 61b42c83ced8..1b6d79662ca1 100644
>--- a/drivers/char/tpm/tpm_tis_spi_main.c
>+++ b/drivers/char/tpm/tpm_tis_spi_main.c
>@@ -85,7 +85,7 @@ static int tpm_tis_spi_transfer_half(struct tpm_tis_data *data, u32 addr,
> struct spi_transfer spi_xfer[3];
> struct spi_message m;
> u8 transfer_len;
>- int ret;
>+ int ret = 0;
>
> while (len) {
> transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
>
As an alternative, we can move the declaration in the while block where
`ret` is always initialized, and we can return 0 at the end of the
function since all the errors have an early return, I mean something
like this:
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
index 61b42c83ced8..c78c0a2d0541 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -85,9 +85,10 @@ static int tpm_tis_spi_transfer_half(struct tpm_tis_data *data, u32 addr,
struct spi_transfer spi_xfer[3];
struct spi_message m;
u8 transfer_len;
- int ret;
while (len) {
+ int ret;
+
transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
spi_message_init(&m);
@@ -135,7 +136,7 @@ static int tpm_tis_spi_transfer_half(struct tpm_tis_data *data, u32 addr,
len -= transfer_len;
}
- return ret;
+ return 0;
}
static int tpm_tis_spi_transfer_full(struct tpm_tis_data *data, u32 addr,
BTW, I don't have a strong opinion on that, your patch also LGTM:
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Thanks,
Stefano
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] tpm_tis: Fix undefined behavior in tpm_tis_spi_transfer_half()
2025-09-17 15:29 [PATCH] tpm_tis: Fix undefined behavior in tpm_tis_spi_transfer_half() Gunnar Kudrjavets
2025-09-17 22:13 ` Paul Menzel
2025-09-18 8:55 ` Stefano Garzarella
@ 2025-09-18 15:54 ` Jarkko Sakkinen
2 siblings, 0 replies; 4+ messages in thread
From: Jarkko Sakkinen @ 2025-09-18 15:54 UTC (permalink / raw)
To: Gunnar Kudrjavets
Cc: peterhuewe, jgg, kyarlagadda, linux-integrity, linux-kernel,
Justinien Bouron
On Wed, Sep 17, 2025 at 03:29:56PM +0000, Gunnar Kudrjavets wrote:
> When len is 0, the while loop in tpm_tis_spi_transfer_half() never
> executes, leaving ret uninitialized. This will lead to undefined
> behavior when the function returns.
>
> The issue was introduced when tpm_tis_spi_transfer() was refactored
> to call tpm_tis_spi_transfer_half() or tpm_tis_spi_transfer_full().
> While ret is properly initialized in tpm_tis_spi_transfer_full(), it
> was missed in tpm_tis_spi_transfer_half().
>
> Initialize ret to 0 at the beginning of the function to ensure
> defined behavior in all code paths.
>
> Found by GCC 14.2.0 static analyzer with -fanalyzer.
>
> Fixes: a86a42ac2bd6 ("tpm_tis_spi: Add hardware wait polling")
> Signed-off-by: Gunnar Kudrjavets <gunnarku@amazon.com>
> Reviewed-by: Justinien Bouron <jbouron@amazon.com>
> ---
> drivers/char/tpm/tpm_tis_spi_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
> index 61b42c83ced8..1b6d79662ca1 100644
> --- a/drivers/char/tpm/tpm_tis_spi_main.c
> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> @@ -85,7 +85,7 @@ static int tpm_tis_spi_transfer_half(struct tpm_tis_data *data, u32 addr,
> struct spi_transfer spi_xfer[3];
> struct spi_message m;
> u8 transfer_len;
> - int ret;
> + int ret = 0;
>
> while (len) {
> transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
>
> base-commit: 5aca7966d2a7255ba92fd5e63268dd767b223aa5
> --
> 2.47.3
>
Thank you.
I just applied the earlier fix, and I'll apply this too (before next
PR).
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply [flat|nested] 4+ messages in thread