lkml.org 
[lkml]   [2020]   [Sep]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/3] integrity: Move import of MokListRT certs to a separate routine
From
Date
On 9/2/20 3:55 AM, Andy Shevchenko wrote:
> On Wed, Aug 26, 2020 at 6:45 AM Lenny Szubowicz <lszubowi@redhat.com> wrote:
>>
>> Move the loading of certs from the UEFI MokListRT into a separate
>> routine to facilitate additional MokList functionality.
>>
>> There is no visible functional change as a result of this patch.
>> Although the UEFI dbx certs are now loaded before the MokList certs,
>> they are loaded onto different key rings. So the order of the keys
>> on their respective key rings is the same.
>
> ...
>
>> /*
>> + * load_moklist_certs() - Load MokList certs
>> + *
>> + * Returns: Summary error status
>> + *
>> + * Load the certs contained in the UEFI MokListRT database into the
>> + * platform trusted keyring.
>> + */
>
> Hmm... Is it intentionally kept out of kernel doc format?

Yes. Since this is a static local routine, I thought that it
shouldn't be included by kerneldoc. But I wanted to generally adhere
to the kernel doc conventions for a routine header. To that end,
in V2 I move the "Return:" section to come after the short description.

>
>> +static int __init load_moklist_certs(void)
>> +{
>> + efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
>> + void *mok = NULL;
>> + unsigned long moksize = 0;
>> + efi_status_t status;
>> + int rc = 0;
>
> Redundant assignment (see below).
>
>> + /* Get MokListRT. It might not exist, so it isn't an error
>> + * if we can't get it.
>> + */
>> + mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
>
>> + if (!mok) {
>
> Why not positive conditional? Sometimes ! is hard to notice.
>
>> + if (status == EFI_NOT_FOUND)
>> + pr_debug("MokListRT variable wasn't found\n");
>> + else
>> + pr_info("Couldn't get UEFI MokListRT\n");
>> + } else {
>> + rc = parse_efi_signature_list("UEFI:MokListRT",
>> + mok, moksize, get_handler_for_db);
>> + if (rc)
>> + pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
>> + kfree(mok);
>
> kfree(...)
> if (rc)
> ...
> return rc;
>
> And with positive conditional there will be no need to have redundant
> 'else' followed by additional level of indentation.
>
>> + }
>
>> + return rc;
>
> return 0;
>
>> +}
>
> P.S. Yes, I see that the above was in the original code, so, consider
> my comments as suggestions to improve the code.
>

I agree that your suggestions improve the code. I've incorporated this
into V2.

-Lenny.

\
 
 \ /
  Last update: 2020-09-05 02:58    [W:0.075 / U:0.972 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site