From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Wed, 3 Jun 2020 11:25:41 +0900 Subject: [PATCH 1/1] efi_loader: validate load option In-Reply-To: <20200531205216.35201-1-xypron.glpk@gmx.de> References: <20200531205216.35201-1-xypron.glpk@gmx.de> Message-ID: <20200603022541.GA21903@laputa> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Heinrich, On Sun, May 31, 2020 at 10:52:16PM +0200, Heinrich Schuchardt wrote: > For passing the optional data of the load option to the loaded imaged > protocol we need its size. > > efi_deserialize_load_option() is changed to return the size of the optional > data. > > As a by-product we get a partial validation of the load option. > Checking the length of the device path remains to be implemented. > > Signed-off-by: Heinrich Schuchardt Please add "Reported-by: Coverity (CID xxx)" for tracing as you requested me before. (even if it doesn't fully address the issue.) Reported-by: Coverity (CID 303760) Reported-by: Coverity (CID 303768) Reported-by: Coverity (CID 303776) -Takahiro Akashi > --- > cmd/efidebug.c | 21 +++++++++++----- > include/efi_loader.h | 3 ++- > lib/efi_loader/efi_bootmgr.c | 48 +++++++++++++++++++++++++++++------- > 3 files changed, 56 insertions(+), 16 deletions(-) > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > index 32430e62f0..58018f700c 100644 > --- a/cmd/efidebug.c > +++ b/cmd/efidebug.c > @@ -694,14 +694,19 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag, > * > * Decode the value of UEFI load option variable and print information. > */ > -static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t size) > +static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) > { > struct efi_load_option lo; > char *label, *p; > size_t label_len16, label_len; > u16 *dp_str; > + efi_status_t ret; > > - efi_deserialize_load_option(&lo, data); > + ret = efi_deserialize_load_option(&lo, data, size); > + if (ret != EFI_SUCCESS) { > + printf("%ls: invalid load option\n", varname16); > + return; > + } > > label_len16 = u16_strlen(lo.label); > label_len = utf16_utf8_strnlen(lo.label, label_len16); > @@ -728,8 +733,7 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t size) > > printf(" data:\n"); > print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > - lo.optional_data, size + (u8 *)data - > - (u8 *)lo.optional_data, true); > + lo.optional_data, *size, true); > free(label); > } > > @@ -759,7 +763,7 @@ static void show_efi_boot_opt(u16 *varname16) > &efi_global_variable_guid, > NULL, &size, data)); > if (ret == EFI_SUCCESS) > - show_efi_boot_opt_data(varname16, data, size); > + show_efi_boot_opt_data(varname16, data, &size); > free(data); > } > } > @@ -920,7 +924,12 @@ static int show_efi_boot_order(void) > goto out; > } > > - efi_deserialize_load_option(&lo, data); > + ret = efi_deserialize_load_option(&lo, data, &size); > + if (ret != EFI_SUCCESS) { > + printf("%ls: invalid load option\n", var_name16); > + ret = CMD_RET_FAILURE; > + goto out; > + } > > label_len16 = u16_strlen(lo.label); > label_len = utf16_utf8_strnlen(lo.label, label_len16); > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 9533df26dc..c2cae814b6 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -708,7 +708,8 @@ struct efi_load_option { > const u8 *optional_data; > }; > > -void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data); > +efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data, > + efi_uintn_t *size); > unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data); > efi_status_t efi_bootmgr_load(efi_handle_t *handle); > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > index fa65445c12..e268e9c4b8 100644 > --- a/lib/efi_loader/efi_bootmgr.c > +++ b/lib/efi_loader/efi_bootmgr.c > @@ -38,24 +38,50 @@ static const struct efi_runtime_services *rs; > * > * @lo: pointer to target > * @data: serialized data > + * @size: size of the load option, on return size of the optional data > + * Return: status code > */ > -void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data) > +efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data, > + efi_uintn_t *size) > { > + efi_uintn_t len; > + > + len = sizeof(u32); > + if (*size < len + 2 * sizeof(u16)) > + return EFI_INVALID_PARAMETER; > lo->attributes = get_unaligned_le32(data); > - data += sizeof(u32); > + data += len; > + *size -= len; > > + len = sizeof(u16); > lo->file_path_length = get_unaligned_le16(data); > - data += sizeof(u16); > + data += len; > + *size -= len; > > - /* FIXME */ > lo->label = (u16 *)data; > - data += (u16_strlen(lo->label) + 1) * sizeof(u16); > - > - /* FIXME */ > + len = u16_strnlen(lo->label, *size / sizeof(u16) - 1); > + if (lo->label[len]) > + return EFI_INVALID_PARAMETER; > + len = (len + 1) * sizeof(u16); > + if (*size < len) > + return EFI_INVALID_PARAMETER; > + data += len; > + *size -= len; > + > + len = lo->file_path_length; > + if (*size < len) > + return EFI_INVALID_PARAMETER; > lo->file_path = (struct efi_device_path *)data; > - data += lo->file_path_length; > + /* > + * TODO: validate device path. There should be an end node within > + * the indicated file_path_length. > + */ > + data += len; > + *size -= len; > > lo->optional_data = data; > + > + return EFI_SUCCESS; > } > > /** > @@ -170,7 +196,11 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle) > if (!load_option) > return EFI_LOAD_ERROR; > > - efi_deserialize_load_option(&lo, load_option); > + ret = efi_deserialize_load_option(&lo, load_option, &size); > + if (ret != EFI_SUCCESS) { > + log_warning("Invalid load option for %ls\n", varname); > + goto error; > + } > > if (lo.attributes & LOAD_OPTION_ACTIVE) { > u32 attributes; > -- > 2.26.2 >