| Message ID | 20220301001125.1554442-4-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilias Apalodimas |
| Headers | show |
| Series | tpm: Various minor fixes and enhancements | expand |
Hi Simon, On Mon, Feb 28, 2022 at 05:11:20PM -0700, Simon Glass wrote: > The offset here is incorrect. Fix it. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > lib/tpm-v1.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c > index 22a769c587..d0e3ab1b21 100644 > --- a/lib/tpm-v1.c > +++ b/lib/tpm-v1.c > @@ -456,12 +456,13 @@ u32 tpm1_get_permissions(struct udevice *dev, u32 index, u32 *perm) > 0x0, 0x0, 0x0, 0x4, > }; > const size_t index_offset = 18; > - const size_t perm_offset = 60; > + const size_t perm_offset = 74; This is fine, but I hate all these magic values sprinkled around the TPM APIs. Starting with this patch can we do the right thing and at lease have those in a #define with a names that makes some sense? > u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; > size_t response_length = sizeof(response); > u32 err; > > - if (pack_byte_string(buf, sizeof(buf), "d", 0, command, sizeof(command), > + if (pack_byte_string(buf, sizeof(buf), "sd", > + 0, command, sizeof(command), > index_offset, index)) > return TPM_LIB_ERROR; > err = tpm_sendrecv_command(dev, buf, response, &response_length); > -- > 2.35.1.574.g5d30c73bfb-goog > Thanks /Ilias
Hi Ilias, On Tue, 7 Jun 2022 at 02:44, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Mon, Feb 28, 2022 at 05:11:20PM -0700, Simon Glass wrote: > > The offset here is incorrect. Fix it. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > lib/tpm-v1.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c > > index 22a769c587..d0e3ab1b21 100644 > > --- a/lib/tpm-v1.c > > +++ b/lib/tpm-v1.c > > @@ -456,12 +456,13 @@ u32 tpm1_get_permissions(struct udevice *dev, u32 index, u32 *perm) > > 0x0, 0x0, 0x0, 0x4, > > }; > > const size_t index_offset = 18; > > - const size_t perm_offset = 60; > > + const size_t perm_offset = 74; > > This is fine, but I hate all these magic values sprinkled around the TPM > APIs. Starting with this patch can we do the right thing and at lease have > those in a #define with a names that makes some sense? Er, isn't this what I am doing? See the const values which have sensible names. It really doesn't make sense to put them in a #define in a distant header file since we don't use these in most commands. There are just local to the function and determined by the format used with pack_byte_string(), etc. (see below). > > > u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; > > size_t response_length = sizeof(response); > > u32 err; > > > > - if (pack_byte_string(buf, sizeof(buf), "d", 0, command, sizeof(command), > > + if (pack_byte_string(buf, sizeof(buf), "sd", > > + 0, command, sizeof(command), > > index_offset, index)) > > return TPM_LIB_ERROR; > > err = tpm_sendrecv_command(dev, buf, response, &response_length); > > -- > > 2.35.1.574.g5d30c73bfb-goog > > > > Thanks > /Ilias Regards, SImon
diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c index 22a769c587..d0e3ab1b21 100644 --- a/lib/tpm-v1.c +++ b/lib/tpm-v1.c @@ -456,12 +456,13 @@ u32 tpm1_get_permissions(struct udevice *dev, u32 index, u32 *perm) 0x0, 0x0, 0x0, 0x4, }; const size_t index_offset = 18; - const size_t perm_offset = 60; + const size_t perm_offset = 74; u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; size_t response_length = sizeof(response); u32 err; - if (pack_byte_string(buf, sizeof(buf), "d", 0, command, sizeof(command), + if (pack_byte_string(buf, sizeof(buf), "sd", + 0, command, sizeof(command), index_offset, index)) return TPM_LIB_ERROR; err = tpm_sendrecv_command(dev, buf, response, &response_length);
The offset here is incorrect. Fix it. Signed-off-by: Simon Glass <sjg@chromium.org> --- lib/tpm-v1.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)