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

[Daniel Axtens]

Nick Child <nnac123 at gmail.com> writes:

> 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.

Hmm OK. Something I will ponder after I return from leave.

> 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.

Yes, please do a minimal fix and then split the rework out. Whether or
not the fix ends up being critical (it requires a signed update or
physical presence IIUC?), it's good to at least structure the patches so
that there's the option of pulling it in separate from a bigger
restructure.

Kind regards,
Daniel

>
> Thanks,
> Nick