* [PATCH 1/1] efi_loader: validate load option
@ 2020-05-31 20:52 Heinrich Schuchardt
2020-06-03 2:25 ` AKASHI Takahiro
0 siblings, 1 reply; 2+ messages in thread
From: Heinrich Schuchardt @ 2020-05-31 20:52 UTC (permalink / raw)
To: u-boot
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 <xypron.glpk@gmx.de>
---
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* [PATCH 1/1] efi_loader: validate load option
2020-05-31 20:52 [PATCH 1/1] efi_loader: validate load option Heinrich Schuchardt
@ 2020-06-03 2:25 ` AKASHI Takahiro
0 siblings, 0 replies; 2+ messages in thread
From: AKASHI Takahiro @ 2020-06-03 2:25 UTC (permalink / raw)
To: u-boot
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 <xypron.glpk@gmx.de>
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-06-03 2:25 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-31 20:52 [PATCH 1/1] efi_loader: validate load option Heinrich Schuchardt
2020-06-03 2:25 ` AKASHI Takahiro
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox