| Message ID | 20250409205536.210202-1-purvayeshi550@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | char: tpm: tpm-buf: Fix uninitialized return values in read helpers | expand |
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 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 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 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 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
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 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
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);
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(-)