Re: [PATCH] char: tpm: tpm-buf: Fix uninitialized return values in read helpers

[Purva Yeshi <purvayeshi550@gmail.com>]

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