[Skiboot] [PATCH v4 1/2] secvar: return error if validate_esl has extra data

[Daniel Axtens]

Nick Child <nnac123 at gmail.com> writes:

> Hi Daniel,
>
> Forgot to mention, these patches are still dependent on:
> https://lists.ozlabs.org/pipermail/skiboot/2021-July/017641.html
>
> They were once in the same patchset but I split them since these
> patches will likely need more discussions before merging.
>
> Sorry, I am still new to sending to mailing lists. Let me know if I should
> take any actions.

Ah cool, it all works now.

You could mark your superseded series as superseded on Patchwork if you
wanted to be extra helpful: https://patchwork.ozlabs.org/project/skiboot/list/

Kind regards,
Daniel

> Thanks,
> Nick
>
> On Wed, Jul 14, 2021 at 11:07 PM Daniel Axtens <dja at axtens.net> wrote:
>>
>> Hi Nick,
>>
>> I can't get either of these patches to apply to the upstream master
>> branch - are there some dependencies I'm missing? From patch 2 it looks
>> like the patch adding a trimmed KEK test is expected?
>>
>> Kind regards,
>> Daniel
>>
>> > Currently, in `validate_esl_list`, the return code is initialized to zero
>> > (our success value). While looping though the ESL's in the submitted ESL
>> > chain, the loop will break if there is not enough data to meet minimum ESL
>> > requirements. This condition was not setting a return code, meaning that the
>> > successful return code can pass to the end of the function if there is extra
>> > data at the end of the ESL. As a consequence, any properly signed update can
>> > successfully commit any data (as long as it is less than the min size of an
>> > ESL) to the secvars.
>> >
>> > This commit will return an error if the described condition is met. This
>> > means all data in the appended ESL of an auth file must be accounted for. No
>> > extra bytes can be added to the end since, on success, this data will become
>> > the updated secvar.
>> >
>> > Additionally, a test case has been added to ensure that this commit
>> > addresses the issue correctly.
>> >
>> > Signed-off-by: Nick Child <nick.child at ibm.com>
>> > Reviewed-by: Nayna Jain <nayna at linux.ibm.com>
>> > Tested-by: Nayna Jain <nayna at linux.ibm.com>
>> > ---
>> >
>> > Previous version at https://lists.ozlabs.org/pipermail/skiboot/2021-July/017643.html
>> >
>> > ---
>> >
>> >  libstb/secvar/backend/edk2-compat-process.c  |  5 ++++-
>> >  libstb/secvar/test/secvar-test-edk2-compat.c | 15 +++++++++++++++
>> >  2 files changed, 19 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
>> > index 5204fdd5..983a3313 100644
>> > --- a/libstb/secvar/backend/edk2-compat-process.c
>> > +++ b/libstb/secvar/backend/edk2-compat-process.c
>> > @@ -267,8 +267,11 @@ int validate_esl_list(const char *key, const char *esl, const size_t size)
>> >
>> >       while (eslvarsize > 0) {
>> >               prlog(PR_DEBUG, "esl var size is %d offset is %lu\n", eslvarsize, size - eslvarsize);
>> > -             if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
>> > +             if (eslvarsize < sizeof(EFI_SIGNATURE_LIST)) {
>> > +                     prlog(PR_ERR, "ESL with size %d is too small\n", eslvarsize);
>> > +                     rc = OPAL_PARAMETER;
>> >                       break;
>> > +             }
>> >
>> >               /* Check Supported ESL Type */
>> >               list = get_esl_signature_list(esl, eslvarsize);
>> > diff --git a/libstb/secvar/test/secvar-test-edk2-compat.c b/libstb/secvar/test/secvar-test-edk2-compat.c
>> > index 3cdc6ef2..1edce112 100644
>> > --- a/libstb/secvar/test/secvar-test-edk2-compat.c
>> > +++ b/libstb/secvar/test/secvar-test-edk2-compat.c
>> > @@ -165,6 +165,21 @@ int run_test()
>> >       ASSERT(5 == list_length(&variable_bank));
>> >       ASSERT(setup_mode);
>> >
>> > +     /* Add PK with bad ESL. should fail since data is not big enough to be ESL*/
>> > +     printf("Add PK with invalid appended ESL");
>> > +     /* 1014 is length of appended ESL Header and its data */
>> > +     tmp = new_secvar("PK", 3, PK_auth, PK_auth_len - 1014 + sizeof(EFI_SIGNATURE_LIST) - 1, 0);
>> > +     ASSERT(0 == edk2_compat_validate(tmp));
>> > +     list_add_tail(&update_bank, &tmp->link);
>> > +     ASSERT(1 == list_length(&update_bank));
>> > +     rc = edk2_compat_process(&variable_bank, &update_bank);
>> > +     ASSERT(5 == list_length(&variable_bank));
>> > +     ASSERT(0 == list_length(&update_bank));
>> > +     rc = edk2_compat_post_process(&variable_bank, &update_bank);
>> > +     ASSERT(5 == list_length(&variable_bank));
>> > +     ASSERT(setup_mode);
>> > +
>> > +
>> >       /* Add PK to update and .process(). */
>> >       printf("Add PK");
>> >       tmp = new_secvar("PK", 3, PK_auth, PK_auth_len, 0);
>> > --
>> > 2.25.1