On Thu, 27 Jan 2022 06:11:03 -0800 (PST) Hernan Gatta <hega...@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> wrote:
> > > On Mon, 24 Jan 2022, Glenn Washburn wrote: > > > On Mon, 24 Jan 2022 06:12:16 -0800 > > Hernan Gatta <[email protected]> wrote: > > > >> From: Hernan Gatta <[email protected]> > >> > >> The TPM2 key protector is a module that enables the automatic retrieval of > >> a > >> fully-encrypted disk's unlocking key from a TPM 2.0. > >> > >> The theory of operation is such that the module accepts various arguments, > >> most > >> of which are optional therefore possess reasonable defaults. One of these > >> arguments is the keyfile parameter, which is mandatory. > >> > >> The value of this parameter must be a path to a sealed key file (e.g., > >> (hd0,gpt1)/boot/grub2/sealed_key). This sealed key file is created via the > >> grub-protect tool. The tool utilizes the TPM's sealing functionality to > >> seal > >> (i.e., encrypt) an unlocking key using a Storage Root Key (SRK) to the > >> values of > >> various Platform Configuration Registers (PCRs). These PCRs reflect the > >> state of > >> the system as it boots. If the values are as expected, the system may be > >> considered trustworthy, at which point the TPM allows for a caller to > >> utilize > >> the private component of the SRK to unseal (i.e., decrypt) the sealed key > >> file. > >> The caller, in this case, is this key protector. > >> > > > > There are a lot of points in this code where an error can be returned. > > In this case, all I'm going to get is a non-descript error code (almost > > always GRUB_ERR_BAD_ARGUMENT). So as a user, how am I to know where the > > error is actually coming from? For this reason, I like the use of > > grub_error(error_code, error_format_string, fmtstr_args...), which will > > set grub_errno and an error message, which can be retrieved up the stack > > and presented to the user. I'm not sure that every return of a naked > > error code should be replaced by grub_error(), depends on how granular > > of an error message you want to see. I would error on the side of more > > granular. For instance, its really nice that GCC will tell me which > > format code type specifier and a argument its choking on, rather than a > > catch all "I'm failing to compile your code because you have an > > incorrect format code somewhere in your format string". > > > > Agreed. I'll add calls to grub_error to indicate why things fail and, when > they do, capture the error up the stack (I was thinking in the command > handler for cryptomount), and print it. IIRC, you shouldn't have to do anything, just return from cryptomount with the error code and have grub_errno and grub_errmsg set. If you swallow any errors, you should print the swallowed errors in grub_dprintf() (maybe not always, judgement required). > > >> Signed-off-by: Hernan Gatta <[email protected]> > >> --- > >> grub-core/Makefile.core.def | 9 + > >> grub-core/tpm2/module.c | 742 > >> ++++++++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 751 insertions(+) > >> create mode 100644 grub-core/tpm2/module.c > >> > >> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def > >> index e4ae78b..8691440 100644 > >> --- a/grub-core/Makefile.core.def > >> +++ b/grub-core/Makefile.core.def > >> @@ -2498,6 +2498,15 @@ module = { > >> }; > >> > >> module = { > >> + name = tpm2; > >> + common = tpm2/buffer.c; > >> + common = tpm2/module.c; > >> + common = tpm2/mu.c; > >> + common = tpm2/tpm2.c; > >> + efi = tpm2/tcg2.c; > >> +}; > >> + > >> +module = { > >> name = tr; > >> common = commands/tr.c; > >> }; > >> diff --git a/grub-core/tpm2/module.c b/grub-core/tpm2/module.c > >> new file mode 100644 > >> index 0000000..cd97452 > >> --- /dev/null > >> +++ b/grub-core/tpm2/module.c > >> @@ -0,0 +1,742 @@ > >> +/* > >> + * GRUB -- GRand Unified Bootloader > >> + * Copyright (C) 2022 Microsoft Corporation > >> + * > >> + * GRUB is free software: you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License as published by > >> + * the Free Software Foundation, either version 3 of the License, or > >> + * (at your option) any later version. > >> + * > >> + * GRUB is distributed in the hope that it will be useful, > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >> + * GNU General Public License for more details. > >> + * > >> + * You should have received a copy of the GNU General Public License > >> + * along with GRUB. If not, see <http://www.gnu.org/licenses/>. > >> + */ > >> + > >> +#include <grub/dl.h> > >> +#include <grub/file.h> > >> +#include <grub/misc.h> > >> +#include <grub/mm.h> > >> +#include <grub/protector.h> > >> +#include <grub/tpm2/buffer.h> > >> +#include <grub/tpm2/mu.h> > >> +#include <grub/tpm2/tpm2.h> > >> +#include <grub/types.h> > >> + > >> +GRUB_MOD_LICENSE ("GPLv3+"); > >> + > >> +typedef enum grub_tpm2_protector_args > >> +{ > >> + GRUB_TPM2_PROTECTOR_ARG_MODE = 1 << 0, > >> + GRUB_TPM2_PROTECTOR_ARG_PCRS = 1 << 1, > >> + GRUB_TPM2_PROTECTOR_ARG_ASYMMETRIC = 1 << 2, > >> + GRUB_TPM2_PROTECTOR_ARG_BANK = 1 << 3, > >> + GRUB_TPM2_PROTECTOR_ARG_KEYFILE = 1 << 4, > >> + GRUB_TPM2_PROTECTOR_ARG_SRK = 1 << 5, > >> + GRUB_TPM2_PROTECTOR_ARG_NV = 1 << 6 > >> +} grub_tpm2_protector_args_t; > >> + > >> +typedef enum grub_tpm2_protector_mode > >> +{ > >> + GRUB_TPM2_PROTECTOR_MODE_SRK = 1, > >> + GRUB_TPM2_PROTECTOR_MODE_NV = 2 > >> +} grub_tpm2_protector_mode_t; > >> + > >> +struct grub_tpm2_protector_context > >> +{ > >> + grub_tpm2_protector_args_t args; > >> + grub_tpm2_protector_mode_t mode; > >> + grub_uint8_t pcrs[TPM_MAX_PCRS]; > >> + grub_uint8_t pcr_count; > >> + TPM_ALG_ID asymmetric; > >> + TPM_ALG_ID bank; > >> + const char *keyfile; > >> + TPM_HANDLE srk; > >> + TPM_HANDLE nv; > >> +}; > >> + > >> +static grub_err_t > >> +grub_tpm2_protector_parse_mode (struct grub_tpm2_protector_context *ctx, > >> + const char *value) > >> +{ > >> + if (grub_strcmp (value, "srk") == 0) > >> + ctx->mode = GRUB_TPM2_PROTECTOR_MODE_SRK; > >> + else if (grub_strcmp (value, "nv") == 0) > >> + ctx->mode = GRUB_TPM2_PROTECTOR_MODE_NV; > >> + else > >> + return GRUB_ERR_OUT_OF_RANGE; > >> + > >> + return GRUB_ERR_NONE; > >> +} > >> + > >> +static grub_err_t > >> +grub_tpm2_protector_parse_pcrs (struct grub_tpm2_protector_context *ctx, > >> + char *value) > >> +{ > >> + grub_err_t err; > >> + char *current_pcr = value; > >> + char *next_pcr; > >> + unsigned long pcr; > >> + grub_uint8_t i; > >> + > >> + if (grub_strlen (value) == 0) > >> + return GRUB_ERR_BAD_ARGUMENT; > >> + > >> + ctx->pcr_count = 0; > >> + for (i = 0; i < TPM_MAX_PCRS; i++) > >> + { > >> + next_pcr = grub_strchr (current_pcr, ','); > >> + if (next_pcr) > >> + *next_pcr = '\0'; > >> + if (next_pcr == current_pcr) > >> + return GRUB_ERR_BAD_ARGUMENT; > >> + > >> + grub_errno = GRUB_ERR_NONE; > >> + pcr = grub_strtoul (current_pcr, NULL, 10); > >> + if (grub_errno != GRUB_ERR_NONE) > >> + { > >> + err = grub_errno; > >> + grub_errno = GRUB_ERR_NONE; > >> + return err; > >> + } > >> + > >> + if (pcr > GRUB_UCHAR_MAX) > >> + return GRUB_ERR_OUT_OF_RANGE; > >> + > >> + ctx->pcrs[i] = (grub_uint8_t)pcr; > >> + ctx->pcr_count++; > >> + > >> + if (!next_pcr) > >> + break; > >> + > >> + current_pcr = next_pcr + 1; > >> + if (*current_pcr == '\0') > >> + return GRUB_ERR_BAD_ARGUMENT; > >> + } > >> + > >> + if (i == TPM_MAX_PCRS) > >> + return GRUB_ERR_OUT_OF_RANGE; > >> + > >> + return GRUB_ERR_NONE; > >> +} > >> + > >> +static grub_err_t > >> +grub_tpm2_protector_parse_asymmetric (struct grub_tpm2_protector_context > >> *ctx, > >> + const char *value) > >> +{ > >> + if (grub_strcasecmp (value, "ECC") == 0) > >> + ctx->asymmetric = TPM_ALG_ECC; > >> + else if (grub_strcasecmp (value, "RSA") == 0) > >> + ctx->asymmetric = TPM_ALG_RSA; > >> + else > >> + return GRUB_ERR_OUT_OF_RANGE; > >> + > >> + return GRUB_ERR_NONE; > >> +} > >> + > >> +static grub_err_t > >> +grub_tpm2_protector_parse_bank (struct grub_tpm2_protector_context *ctx, > >> + const char *value) > >> +{ > >> + if (grub_strcasecmp (value, "SHA1") == 0) > >> + ctx->bank = TPM_ALG_SHA1; > >> + else if (grub_strcasecmp (value, "SHA256") == 0) > >> + ctx->bank = TPM_ALG_SHA256; > >> + else if (grub_strcasecmp (value, "SHA384") == 0) > >> + ctx->bank = TPM_ALG_SHA384; > >> + else > >> + return GRUB_ERR_OUT_OF_RANGE; > >> + > >> + return GRUB_ERR_NONE; > >> +} > >> + > >> +static grub_err_t > >> +grub_tpm2_protector_parse_keyfile (struct grub_tpm2_protector_context > >> *ctx, > >> + const char *value) > >> +{ > >> + if (grub_strlen (value) == 0) > >> + return GRUB_ERR_BAD_ARGUMENT; > >> + > >> + ctx->keyfile = value; > >> + > >> + return GRUB_ERR_NONE; > >> +} > >> + > >> +static grub_err_t > >> +grub_tpm2_protector_parse_tpm_handle (const char *value, TPM_HANDLE > >> *handle) > >> +{ > >> + grub_err_t err; > >> + unsigned long num; > >> + > >> + grub_errno = GRUB_ERR_NONE; > >> + num = grub_strtoul (value, NULL, 0); > >> + if (grub_errno != GRUB_ERR_NONE) > >> + { > >> + err = grub_errno; > >> + grub_errno = GRUB_ERR_NONE; > >> + return err; > >> + } > >> + > >> + if (num > GRUB_UINT_MAX) > >> + return GRUB_ERR_OUT_OF_RANGE; > >> + > >> + *handle = (TPM_HANDLE)num; > >> + > >> + return GRUB_ERR_NONE; > >> +} > >> + > >> +static grub_err_t > >> +grub_tpm2_protector_parse_srk (struct grub_tpm2_protector_context *ctx, > >> + const char *value) > >> +{ > >> + return grub_tpm2_protector_parse_tpm_handle (value, &ctx->srk); > >> +} > >> + > >> +static grub_err_t > >> +grub_tpm2_protector_parse_nvindex (struct grub_tpm2_protector_context > >> *ctx, > >> + const char *value) > >> +{ > >> + return grub_tpm2_protector_parse_tpm_handle (value, &ctx->nv); > >> +} > >> + > >> +static grub_err_t > >> +grub_tpm2_protector_match_arg (struct grub_tpm2_protector_context *ctx, > >> + const char *key, char *value) > >> +{ > >> + grub_err_t err; > >> + > >> + if (grub_strlen (key) == 0 || grub_strlen (value) == 0) > >> + return GRUB_ERR_BAD_ARGUMENT; > >> + > >> + if (grub_strcmp (key, "mode") == 0) > >> + { > >> + if (ctx->args & GRUB_TPM2_PROTECTOR_ARG_MODE) > >> + return GRUB_ERR_BAD_ARGUMENT; > >> + > >> + err = grub_tpm2_protector_parse_mode (ctx, value); > >> + if (!err) > >> + ctx->args |= GRUB_TPM2_PROTECTOR_ARG_MODE; > >> + > >> + return err; > >> + } > >> + > >> + if (grub_strcmp (key, "pcrs") == 0) > > > > Minor nit, why wouldn't this be better as an "else if"? Seems > > unnecessary to check all the "if" statements if we've already found a > > match. > > > > The reason why I wrote this in this way without an "else if" is because > each "if" clause has an unconditional return. So, in the end, the entire > sequence behaves in the manner you describe. > > Would you prefer that I add the "else if"'s anyway for readability? Somehow I missed the returns. I don't care as much now. Maybe Daniel will have an opinion when he gets to this. Glenn _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
