Fix Smatch-detected error:
drivers/char/tpm/tpm-buf.c:208 tpm_buf_read_u8() error:
uninitialized symbol 'value'.
drivers/char/tpm/tpm-buf.c:225 tpm_buf_read_u16() error:
uninitialized symbol 'value'.
drivers/char/tpm/tpm-buf.c:242 tpm_buf_read_u32() error:
uninitialized symbol 'value'.

Call tpm_buf_read() to populate value but do not check its return
status. If the read fails, value remains uninitialized, causing
undefined behavior when returned or processed.

Initialize value to zero to ensure a defined return even if
tpm_buf_read() fails, avoiding undefined behavior from using
an uninitialized variable.

Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
---
 drivers/char/tpm/tpm-buf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index e49a19fea3bd..dc882fc9fa9e 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -201,7 +201,7 @@ static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count, void
  */
 u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset)
 {
-	u8 value;
+	u8 value = 0;
 
 	tpm_buf_read(buf, offset, sizeof(value), &value);
 
@@ -218,7 +218,7 @@ EXPORT_SYMBOL_GPL(tpm_buf_read_u8);
  */
 u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset)
 {
-	u16 value;
+	u16 value = 0;
 
 	tpm_buf_read(buf, offset, sizeof(value), &value);
 
@@ -235,7 +235,7 @@ EXPORT_SYMBOL_GPL(tpm_buf_read_u16);
  */
 u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset)
 {
-	u32 value;
+	u32 value = 0;
 
 	tpm_buf_read(buf, offset, sizeof(value), &value);
 
-- 
2.34.1

On Thu, Apr 10, 2025 at 02:25:36AM +0530, Purva Yeshi wrote:
> Fix Smatch-detected error:
> drivers/char/tpm/tpm-buf.c:208 tpm_buf_read_u8() error:
> uninitialized symbol 'value'.
> drivers/char/tpm/tpm-buf.c:225 tpm_buf_read_u16() error:
> uninitialized symbol 'value'.
> drivers/char/tpm/tpm-buf.c:242 tpm_buf_read_u32() error:
> uninitialized symbol 'value'.
> 
> Call tpm_buf_read() to populate value but do not check its return
> status. If the read fails, value remains uninitialized, causing
> undefined behavior when returned or processed.
> 
> Initialize value to zero to ensure a defined return even if
> tpm_buf_read() fails, avoiding undefined behavior from using
> an uninitialized variable.

How does tpm_buf_read() fail?

> 
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> ---
>  drivers/char/tpm/tpm-buf.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index e49a19fea3bd..dc882fc9fa9e 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -201,7 +201,7 @@ static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count, void
>   */
>  u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset)
>  {
> -	u8 value;
> +	u8 value = 0;
>  
>  	tpm_buf_read(buf, offset, sizeof(value), &value);
>  
> @@ -218,7 +218,7 @@ EXPORT_SYMBOL_GPL(tpm_buf_read_u8);
>   */
>  u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset)
>  {
> -	u16 value;
> +	u16 value = 0;
>  
>  	tpm_buf_read(buf, offset, sizeof(value), &value);
>  
> @@ -235,7 +235,7 @@ EXPORT_SYMBOL_GPL(tpm_buf_read_u16);
>   */
>  u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset)
>  {
> -	u32 value;
> +	u32 value = 0;
>  
>  	tpm_buf_read(buf, offset, sizeof(value), &value);
>  
> -- 
> 2.34.1
> 

BR, Jarkko

On Thu, Apr 10, 2025 at 09:14:58AM +0300, Jarkko Sakkinen wrote:
>On Thu, Apr 10, 2025 at 02:25:36AM +0530, Purva Yeshi wrote:
>> Fix Smatch-detected error:
>> drivers/char/tpm/tpm-buf.c:208 tpm_buf_read_u8() error:
>> uninitialized symbol 'value'.
>> drivers/char/tpm/tpm-buf.c:225 tpm_buf_read_u16() error:
>> uninitialized symbol 'value'.
>> drivers/char/tpm/tpm-buf.c:242 tpm_buf_read_u32() error:
>> uninitialized symbol 'value'.
>>
>> Call tpm_buf_read() to populate value but do not check its return
>> status. If the read fails, value remains uninitialized, causing
>> undefined behavior when returned or processed.
>>
>> Initialize value to zero to ensure a defined return even if
>> tpm_buf_read() fails, avoiding undefined behavior from using
>> an uninitialized variable.
>
>How does tpm_buf_read() fail?

If TPM_BUF_BOUNDARY_ERROR is set (or we are setting it), we are 
effectively returning random stack bytes to the caller.
Could this be a problem?

If it is, maybe instead of this patch, we could set `*output` to zero in 
the error path of tpm_buf_read(). Or return an error from tpm_buf_read() 
so callers can return 0 or whatever they want.

Thanks,
Stefano

On Thu, Apr 10, 2025 at 09:51:09AM +0200, Stefano Garzarella wrote:
> On Thu, Apr 10, 2025 at 09:14:58AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Apr 10, 2025 at 02:25:36AM +0530, Purva Yeshi wrote:
> > > Fix Smatch-detected error:

Empty line and s/error/issue/.

> > > drivers/char/tpm/tpm-buf.c:208 tpm_buf_read_u8() error:
> > > uninitialized symbol 'value'.
> > > drivers/char/tpm/tpm-buf.c:225 tpm_buf_read_u16() error:
> > > uninitialized symbol 'value'.
> > > drivers/char/tpm/tpm-buf.c:242 tpm_buf_read_u32() error:
> > > uninitialized symbol 'value'.
> > > 
> > > Call tpm_buf_read() to populate value but do not check its return
> > > status. If the read fails, value remains uninitialized, causing
> > > undefined behavior when returned or processed.
> > > 
> > > Initialize value to zero to ensure a defined return even if
> > > tpm_buf_read() fails, avoiding undefined behavior from using
> > > an uninitialized variable.
> > 
> > How does tpm_buf_read() fail?
> 
> If TPM_BUF_BOUNDARY_ERROR is set (or we are setting it), we are effectively
> returning random stack bytes to the caller.
> Could this be a problem?

It should never happen, if the kernel is working correctly. The commit
message implies a legit failure scenario, which would imply that the
patch is a bug fix, which it actually is not.

"Add a sanity check for boundary overflow, and zero out the value,
if the unexpected happens" is what this patch actually does. I.e.,
code change acceptable, commit message is all wrong.

> 
> If it is, maybe instead of this patch, we could set `*output` to zero in the
> error path of tpm_buf_read(). Or return an error from tpm_buf_read() so
> callers can return 0 or whatever they want.
> 
> Thanks,
> Stefano
> 
> 

BR, Jarkko

On 10/04/25 14:24, Jarkko Sakkinen wrote:
> On Thu, Apr 10, 2025 at 09:51:09AM +0200, Stefano Garzarella wrote:
>> On Thu, Apr 10, 2025 at 09:14:58AM +0300, Jarkko Sakkinen wrote:
>>> On Thu, Apr 10, 2025 at 02:25:36AM +0530, Purva Yeshi wrote:
>>>> Fix Smatch-detected error:
> 
> Empty line and s/error/issue/.
> 
>>>> drivers/char/tpm/tpm-buf.c:208 tpm_buf_read_u8() error:
>>>> uninitialized symbol 'value'.
>>>> drivers/char/tpm/tpm-buf.c:225 tpm_buf_read_u16() error:
>>>> uninitialized symbol 'value'.
>>>> drivers/char/tpm/tpm-buf.c:242 tpm_buf_read_u32() error:
>>>> uninitialized symbol 'value'.
>>>>
>>>> Call tpm_buf_read() to populate value but do not check its return
>>>> status. If the read fails, value remains uninitialized, causing
>>>> undefined behavior when returned or processed.
>>>>
>>>> Initialize value to zero to ensure a defined return even if
>>>> tpm_buf_read() fails, avoiding undefined behavior from using
>>>> an uninitialized variable.
>>>
>>> How does tpm_buf_read() fail?
>>
>> If TPM_BUF_BOUNDARY_ERROR is set (or we are setting it), we are effectively
>> returning random stack bytes to the caller.
>> Could this be a problem?
> 
> It should never happen, if the kernel is working correctly. The commit
> message implies a legit failure scenario, which would imply that the
> patch is a bug fix, which it actually is not.
> 
> "Add a sanity check for boundary overflow, and zero out the value,
> if the unexpected happens" is what this patch actually does. I.e.,
> code change acceptable, commit message is all wrong.

Understood now. I’ll update the commit message to clearly state this is 
a sanity check for unexpected boundary overflows. Thanks for the 
clarification!

> 
>>
>> If it is, maybe instead of this patch, we could set `*output` to zero in the
>> error path of tpm_buf_read(). Or return an error from tpm_buf_read() so
>> callers can return 0 or whatever they want.
>>
>> Thanks,
>> Stefano
>>
>>
> 
> BR, Jarkko

On 10/04/25 13:21, Stefano Garzarella wrote:
> On Thu, Apr 10, 2025 at 09:14:58AM +0300, Jarkko Sakkinen wrote:
>> On Thu, Apr 10, 2025 at 02:25:36AM +0530, Purva Yeshi wrote:
>>> Fix Smatch-detected error:
>>> drivers/char/tpm/tpm-buf.c:208 tpm_buf_read_u8() error:
>>> uninitialized symbol 'value'.
>>> drivers/char/tpm/tpm-buf.c:225 tpm_buf_read_u16() error:
>>> uninitialized symbol 'value'.
>>> drivers/char/tpm/tpm-buf.c:242 tpm_buf_read_u32() error:
>>> uninitialized symbol 'value'.
>>>
>>> Call tpm_buf_read() to populate value but do not check its return
>>> status. If the read fails, value remains uninitialized, causing
>>> undefined behavior when returned or processed.
>>>
>>> Initialize value to zero to ensure a defined return even if
>>> tpm_buf_read() fails, avoiding undefined behavior from using
>>> an uninitialized variable.
>>
>> How does tpm_buf_read() fail?
> 
> If TPM_BUF_BOUNDARY_ERROR is set (or we are setting it), we are 
> effectively returning random stack bytes to the caller.
> Could this be a problem?
> 
> If it is, maybe instead of this patch, we could set `*output` to zero in 
> the error path of tpm_buf_read(). Or return an error from tpm_buf_read() 
> so callers can return 0 or whatever they want.
> 
> Thanks,
> Stefano
> 

Hi Jarkko, Stefano,
Thank you for the review.

I've revisited the issue and updated the implementation of 
tpm_buf_read() to zero out the *output buffer in the error paths, 
instead of initializing the return value in each caller.

static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t 
count, void *output)
{
	off_t next_offset;

	/* Return silently if overflow has already happened. */
	if (buf->flags & TPM_BUF_BOUNDARY_ERROR) {
		memset(output, 0, count);
		return;
	}

	next_offset = *offset + count;
	if (next_offset > buf->length) {
		WARN(1, "tpm_buf: read out of boundary\n");
		buf->flags |= TPM_BUF_BOUNDARY_ERROR;
		memset(output, 0, count);
		return;
	}

	memcpy(output, &buf->data[*offset], count);
	*offset = next_offset;
}

This approach ensures that output is always zeroed when the read fails, 
which avoids returning uninitialized stack values from the helper 
functions like tpm_buf_read_u8(), tpm_buf_read_u16(), and 
tpm_buf_read_u32().

Does this solution look acceptable for the next version of the patch?

Best regards,
Purva Yeshi

On Thu, Apr 10, 2025 at 02:12:07PM +0530, Purva Yeshi wrote:
> On 10/04/25 13:21, Stefano Garzarella wrote:
> > On Thu, Apr 10, 2025 at 09:14:58AM +0300, Jarkko Sakkinen wrote:
> > > On Thu, Apr 10, 2025 at 02:25:36AM +0530, Purva Yeshi wrote:
> > > > Fix Smatch-detected error:
> > > > drivers/char/tpm/tpm-buf.c:208 tpm_buf_read_u8() error:
> > > > uninitialized symbol 'value'.
> > > > drivers/char/tpm/tpm-buf.c:225 tpm_buf_read_u16() error:
> > > > uninitialized symbol 'value'.
> > > > drivers/char/tpm/tpm-buf.c:242 tpm_buf_read_u32() error:
> > > > uninitialized symbol 'value'.
> > > > 
> > > > Call tpm_buf_read() to populate value but do not check its return
> > > > status. If the read fails, value remains uninitialized, causing
> > > > undefined behavior when returned or processed.
> > > > 
> > > > Initialize value to zero to ensure a defined return even if
> > > > tpm_buf_read() fails, avoiding undefined behavior from using
> > > > an uninitialized variable.
> > > 
> > > How does tpm_buf_read() fail?
> > 
> > If TPM_BUF_BOUNDARY_ERROR is set (or we are setting it), we are
> > effectively returning random stack bytes to the caller.
> > Could this be a problem?
> > 
> > If it is, maybe instead of this patch, we could set `*output` to zero in
> > the error path of tpm_buf_read(). Or return an error from tpm_buf_read()
> > so callers can return 0 or whatever they want.
> > 
> > Thanks,
> > Stefano
> > 
> 
> Hi Jarkko, Stefano,
> Thank you for the review.
> 
> I've revisited the issue and updated the implementation of tpm_buf_read() to
> zero out the *output buffer in the error paths, instead of initializing the
> return value in each caller.
> 
> static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count,
> void *output)
> {
> 	off_t next_offset;
> 
> 	/* Return silently if overflow has already happened. */
> 	if (buf->flags & TPM_BUF_BOUNDARY_ERROR) {
> 		memset(output, 0, count);
> 		return;
> 	}
> 
> 	next_offset = *offset + count;
> 	if (next_offset > buf->length) {
> 		WARN(1, "tpm_buf: read out of boundary\n");
> 		buf->flags |= TPM_BUF_BOUNDARY_ERROR;
> 		memset(output, 0, count);
> 		return;
> 	}
> 
> 	memcpy(output, &buf->data[*offset], count);
> 	*offset = next_offset;
> }

Please don't touch this.

> 
> This approach ensures that output is always zeroed when the read fails,
> which avoids returning uninitialized stack values from the helper functions
> like tpm_buf_read_u8(), tpm_buf_read_u16(), and tpm_buf_read_u32().
> 
> Does this solution look acceptable for the next version of the patch?
> 
> Best regards,
> Purva Yeshi

BR, Jarkko

On 10/04/25 14:25, Jarkko Sakkinen wrote:
> On Thu, Apr 10, 2025 at 02:12:07PM +0530, Purva Yeshi wrote:
>> On 10/04/25 13:21, Stefano Garzarella wrote:
>>> On Thu, Apr 10, 2025 at 09:14:58AM +0300, Jarkko Sakkinen wrote:
>>>> On Thu, Apr 10, 2025 at 02:25:36AM +0530, Purva Yeshi wrote:
>>>>> Fix Smatch-detected error:
>>>>> drivers/char/tpm/tpm-buf.c:208 tpm_buf_read_u8() error:
>>>>> uninitialized symbol 'value'.
>>>>> drivers/char/tpm/tpm-buf.c:225 tpm_buf_read_u16() error:
>>>>> uninitialized symbol 'value'.
>>>>> drivers/char/tpm/tpm-buf.c:242 tpm_buf_read_u32() error:
>>>>> uninitialized symbol 'value'.
>>>>>
>>>>> Call tpm_buf_read() to populate value but do not check its return
>>>>> status. If the read fails, value remains uninitialized, causing
>>>>> undefined behavior when returned or processed.
>>>>>
>>>>> Initialize value to zero to ensure a defined return even if
>>>>> tpm_buf_read() fails, avoiding undefined behavior from using
>>>>> an uninitialized variable.
>>>>
>>>> How does tpm_buf_read() fail?
>>>
>>> If TPM_BUF_BOUNDARY_ERROR is set (or we are setting it), we are
>>> effectively returning random stack bytes to the caller.
>>> Could this be a problem?
>>>
>>> If it is, maybe instead of this patch, we could set `*output` to zero in
>>> the error path of tpm_buf_read(). Or return an error from tpm_buf_read()
>>> so callers can return 0 or whatever they want.
>>>
>>> Thanks,
>>> Stefano
>>>
>>
>> Hi Jarkko, Stefano,
>> Thank you for the review.
>>
>> I've revisited the issue and updated the implementation of tpm_buf_read() to
>> zero out the *output buffer in the error paths, instead of initializing the
>> return value in each caller.
>>
>> static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count,
>> void *output)
>> {
>> 	off_t next_offset;
>>
>> 	/* Return silently if overflow has already happened. */
>> 	if (buf->flags & TPM_BUF_BOUNDARY_ERROR) {
>> 		memset(output, 0, count);
>> 		return;
>> 	}
>>
>> 	next_offset = *offset + count;
>> 	if (next_offset > buf->length) {
>> 		WARN(1, "tpm_buf: read out of boundary\n");
>> 		buf->flags |= TPM_BUF_BOUNDARY_ERROR;
>> 		memset(output, 0, count);
>> 		return;
>> 	}
>>
>> 	memcpy(output, &buf->data[*offset], count);
>> 	*offset = next_offset;
>> }
> 
> Please don't touch this.

Got it, thanks!

> 
>>
>> This approach ensures that output is always zeroed when the read fails,
>> which avoids returning uninitialized stack values from the helper functions
>> like tpm_buf_read_u8(), tpm_buf_read_u16(), and tpm_buf_read_u32().
>>
>> Does this solution look acceptable for the next version of the patch?
>>
>> Best regards,
>> Purva Yeshi
> 
> BR, Jarkko

On Thu, Apr 10, 2025 at 11:55:22AM +0300, Jarkko Sakkinen wrote:
> On Thu, Apr 10, 2025 at 02:12:07PM +0530, Purva Yeshi wrote:
> > On 10/04/25 13:21, Stefano Garzarella wrote:
> > > On Thu, Apr 10, 2025 at 09:14:58AM +0300, Jarkko Sakkinen wrote:
> > > > On Thu, Apr 10, 2025 at 02:25:36AM +0530, Purva Yeshi wrote:
> > > > > Fix Smatch-detected error:
> > > > > drivers/char/tpm/tpm-buf.c:208 tpm_buf_read_u8() error:
> > > > > uninitialized symbol 'value'.
> > > > > drivers/char/tpm/tpm-buf.c:225 tpm_buf_read_u16() error:
> > > > > uninitialized symbol 'value'.
> > > > > drivers/char/tpm/tpm-buf.c:242 tpm_buf_read_u32() error:
> > > > > uninitialized symbol 'value'.
> > > > > 
> > > > > Call tpm_buf_read() to populate value but do not check its return
> > > > > status. If the read fails, value remains uninitialized, causing
> > > > > undefined behavior when returned or processed.
> > > > > 
> > > > > Initialize value to zero to ensure a defined return even if
> > > > > tpm_buf_read() fails, avoiding undefined behavior from using
> > > > > an uninitialized variable.
> > > > 
> > > > How does tpm_buf_read() fail?
> > > 
> > > If TPM_BUF_BOUNDARY_ERROR is set (or we are setting it), we are
> > > effectively returning random stack bytes to the caller.
> > > Could this be a problem?
> > > 
> > > If it is, maybe instead of this patch, we could set `*output` to zero in
> > > the error path of tpm_buf_read(). Or return an error from tpm_buf_read()
> > > so callers can return 0 or whatever they want.
> > > 
> > > Thanks,
> > > Stefano
> > > 
> > 
> > Hi Jarkko, Stefano,
> > Thank you for the review.
> > 
> > I've revisited the issue and updated the implementation of tpm_buf_read() to
> > zero out the *output buffer in the error paths, instead of initializing the
> > return value in each caller.
> > 
> > static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count,
> > void *output)
> > {
> > 	off_t next_offset;
> > 
> > 	/* Return silently if overflow has already happened. */
> > 	if (buf->flags & TPM_BUF_BOUNDARY_ERROR) {
> > 		memset(output, 0, count);
> > 		return;
> > 	}
> > 
> > 	next_offset = *offset + count;
> > 	if (next_offset > buf->length) {
> > 		WARN(1, "tpm_buf: read out of boundary\n");
> > 		buf->flags |= TPM_BUF_BOUNDARY_ERROR;
> > 		memset(output, 0, count);
> > 		return;
> > 	}
> > 
> > 	memcpy(output, &buf->data[*offset], count);
> > 	*offset = next_offset;
> > }
> 
> Please don't touch this.

If you want to do anything, check the call sites for raw tpm_buf_read()
instead, which is not very common.

BR, Jarkko