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
