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

[Jarkko Sakkinen <jarkko@kernel.org>]

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