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

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: <fab2bb2d-a78e-4130-a5fd-bf07430210c7@gmail.com>
Date: Thu, 10 Apr 2025 14:12:07 +0530
From: Purva Yeshi <purvayeshi550@...il.com>
To: Stefano Garzarella <sgarzare@...hat.com>,
 Jarkko Sakkinen <jarkko@...nel.org>
Cc: peterhuewe@....de, jgg@...pe.ca, linux-integrity@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] char: tpm: tpm-buf: Fix uninitialized return values in
 read helpers

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