Re: [PATCH 3/8] tpm: Correct the permissions command in TPMv1

[Simon Glass <sjg@chromium.org>]

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