Messages in this thread | | | Subject | Re: [PATCH 2/3] integrity: Move import of MokListRT certs to a separate routine | From | Lenny Szubowicz <> | Date | Fri, 4 Sep 2020 20:57:57 -0400 |
| |
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.
| |