[Skiboot] [PATCH v2 3/4] secvar: return error if validate_esl has extra data
nnac123 at gmail.com
Thu Jul 1 01:38:05 AEST 2021
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
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.
More information about the Skiboot