[Skiboot] [PATCH v2 3/4] secvar: return error if validate_esl has extra data

[Nick Child]

Daniel Axtens <dja at axtens.net> writes:

> I can't figure out whether this patch is too complex or you've just
> explained some simple changes in a roundabout way.

Good point. This commit does add in a bunch of semi related code
changes. I was having trouble deciding whether to add everything in
this commit or add another patch.

> > ---
> >  libstb/secvar/backend/edk2-compat-process.c  | 82 +++++++++-----------
> >  libstb/secvar/test/secvar-test-edk2-compat.c | 15 ++++
> >  2 files changed, 52 insertions(+), 45 deletions(-)
> >
> > diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> > index 7d83c912..ff445d45 100644
> > --- a/libstb/secvar/backend/edk2-compat-process.c
> > +++ b/libstb/secvar/backend/edk2-compat-process.c
>>
>
> So if we're in the process of redesigning APIs, and apologies that it's
> taking me a while to come around to what I think is the best design:
>
> Would it make more sense to:
>
>  - have a function that verifies the list - that is, checks
>    SignatureListSize, and SignatureSize, and SignatureHeaderSize ---
>    because they're all from the data we use all of them later on, and
>
>  - have that function take a buffer and buffer len and return an error
>    code, and
>
>  - after that function runs, you just cast the buffer to an
>    EFI_SIGNATURE_LIST directly, which is what the function does anyway.
>
> Saves us passing pointer-to-pointer around and allows for cleaner
> names...

My only concern with that idea is sometimes the calling function does not
want anything to do with the ESL and only wants the size, see
`verify_signature`. I guess this could be resolved by adding back
`get_esl_singature_list_size`. Also, having a function to just determine
if a data buffer is a valid ESL would require the data buffer to be typecast
to an EFI_SIGNATURE_LIST twice (once in the function and again
on success in the calling function). This isn't a bad/costly operation
but I am just bringing awareness that the somewhat scary cast from
data buffer to ESL struct would have to appear in several more places.

Let me know your thoughts and I can go ahead and implement your
recommendations. Additionally, let me know if I should put these changes
in a separate patch and leave this patch as it was in V1.

Thanks,
Nick