[3/3] uefi: uefidump: add support for Key#### (LP:#1237263)

Add support for Key#### - The Key#### variable associates a key press
with a single boot option. Each Key#### variable is the name "Key"
appended with a unique four digit hexadecimal number. For example,
Key0001, Key0002, Key00A0, etc.

The Key#### variables have the format, EFI_KEY_OPTION.

Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
---
 src/lib/include/fwts_uefi.h  |   26 +++++++++++++
 src/uefi/uefidump/uefidump.c |   85 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)

Thanks for the patch Ivan, just a few comments..

On 09/10/13 09:48, Ivan Hu wrote:
> Add support for Key#### - The Key#### variable associates a key press
> with a single boot option. Each Key#### variable is the name "Key"
> appended with a unique four digit hexadecimal number. For example,
> Key0001, Key0002, Key00A0, etc.
> 
> The Key#### variables have the format, EFI_KEY_OPTION.
> 
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>  src/lib/include/fwts_uefi.h  |   26 +++++++++++++
>  src/uefi/uefidump/uefidump.c |   85 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 111 insertions(+)
> 
> diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h
> index d38903e..bb5d513 100644
> --- a/src/lib/include/fwts_uefi.h
> +++ b/src/lib/include/fwts_uefi.h
> @@ -115,6 +115,32 @@ typedef struct {
>          fwts_uefi_dev_path unused_file_path_list[1];
>  } __attribute__((packed)) fwts_uefi_load_option;
>  
> +typedef union {
> +	struct {
> +		uint32_t revision	: 8;
> +		uint32_t shiftpressed	: 1;
> +		uint32_t controlpressed	: 1;
> +		uint32_t altpressed	: 1;
> +		uint32_t logopressed	: 1;
> +		uint32_t menupressed	: 1;
> +		uint32_t sysreqpressed	: 1;
> +		uint32_t reserved	: 16;
> +		uint32_t inputkeycount	: 2;
> +	} options;
> +	uint32_t packedvalue;
> +} __attribute__((packed)) fwts_uefi_boot_key_data;

I'd rather not use C bitfields when mapping to an external data object.
Various reasons why (compiler is not obliged to map it the way you want,
endianness, etc).


> +
> +typedef struct {
> +        fwts_uefi_boot_key_data keydata;
> +        uint32_t bootoptioncrc;
> +        uint16_t bootoption;
> +} __attribute__((packed)) fwts_uefi_key_option;
> +
> +typedef struct {
> +        uint16_t scancode;
> +        uint16_t unicodechar;
> +} __attribute__((packed)) fwts_uefi_input_key;
> +
>  typedef enum {
>  	FWTS_UEFI_HARDWARE_DEV_PATH_TYPE =		(0x01),
>  	FWTS_UEFI_ACPI_DEVICE_PATH_TYPE =		(0x02),
> diff --git a/src/uefi/uefidump/uefidump.c b/src/uefi/uefidump/uefidump.c
> index 4a736c9..de2bd4e 100644
> --- a/src/uefi/uefidump/uefidump.c
> +++ b/src/uefi/uefidump/uefidump.c
> @@ -768,6 +768,83 @@ static void uefidump_info_driverdev(fwts_framework *fw, fwts_uefi_var *var)
>  	free(path);
>  }
>  
> +static void uefidump_info_keyoption(fwts_framework *fw, fwts_uefi_var *var)
> +{
> +	fwts_uefi_key_option *key_option = (fwts_uefi_key_option *)var->data;
> +  	fwts_uefi_input_key *inputkey = (fwts_uefi_input_key *) (((uint8_t *) var->data) + sizeof (fwts_uefi_key_option));
> +	char str[300];
> +	uint16_t keyoptionsize;
> +	int index = 0;
> +
> +	*str = 0;
> +	keyoptionsize = sizeof (fwts_uefi_key_option) + key_option->keydata.options.inputkeycount * sizeof (fwts_uefi_input_key);

You are assuming that the variable size is big enough to do these
deferences into key_option (and also inputkey). Please check that var is
long enough before deferencing like this to avoid deferencing bugs.

> +
> +	if (var->datalen != keyoptionsize) {
> +		uefidump_var_hexdump(fw, var);
> +		return;
> +	}

On my x220 test machine I have data structures larger than you test for,
which are valid, e.g.:


  Data: 0000: 00 00 00 40 d2 80 05 95 05 00 15 00 00 00 00 00
...@............
  Data: 0010: 00 00 00 00 00 00 56 00 00 00 00 00 00 00 01 00
......V.........
  Data: 0020: 00 00 00 00 00 00 18 00 00 00 00 00 00 00 00 00
................
  Data: 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
................
  Data: 0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
................
  Data: 0050: 00 00 42 00 35 00 4c 00 61 00 75 00 6e 00 63 00
..B.5.L.a.u.n.c.
  Data: 0060: 68 00 69 00 6e 00 67 00 20 00 74 00 68 00 65 00  h.i.n.g.
.t.h.e.
  Data: 0070: 20 00 72 00 65 00 63 00 6f 00 76 00 65 00 72 00
.r.e.c.o.v.e.r.
  Data: 0080: 79 00 20 00 75 00 74 00 69 00 6c 00 69 00 74 00  y.
.u.t.i.l.i.t.
  Data: 0090: 79 00 00 00                                      y...

it seems that extra data is following the keyoption data structure which
the firmware is using. Hence perhaps the check should only hexdump the
variable if it is less than the expected size.

> +
> +	if (key_option->keydata.options.shiftpressed)
> +		strcat(str, "ShiftPressed");
> +
> +	if (key_option->keydata.options.controlpressed) {
> +		if (*str)
> +			strcat(str, ",");
> +		strcat(str, "ControlPressed");
> +	}
> +
> +	if (key_option->keydata.options.altpressed) {
> +		if (*str)
> +			strcat(str, ",");
> +		strcat(str, "AltPressed");
> +	}
> +
> +	if (key_option->keydata.options.logopressed) {
> +		if (*str)
> +			strcat(str, ",");
> +		strcat(str, "LogoPressed");
> +	}
> +
> +	if (key_option->keydata.options.menupressed) {
> +		if (*str)
> +			strcat(str, ",");
> +		strcat(str, "MenuPressed");
> +	}
> +
> +	if (key_option->keydata.options.sysreqpressed) {
> +		if (*str)
> +			strcat(str, ",");
> +		strcat(str, "SysReqPressed");
> +	}
> +
> +	if (*str != 0)
> +		fwts_log_info_verbatum(fw, "  PackedValue: 0x%8.8" PRIx32 " (%s).", key_option->keydata.packedvalue, str);
> +	else
> +		fwts_log_info_verbatum(fw, "  PackedValue: 0x%8.8" PRIx32 ".", key_option->keydata.packedvalue);
> +	
> +	fwts_log_info_verbatum(fw, "  BootOptionCrc: 0x%8.8" PRIx32 ".", key_option->bootoptioncrc);
> +	fwts_log_info_verbatum(fw, "  BootOption: %4.4" PRIx16 ".", key_option->bootoption);
> +
> +	do {
> +		fwts_log_info_verbatum(fw, "  ScanCode: 0x%4.4" PRIx16 ".", inputkey[index].scancode);
> +
> +		/*
> +		 * The UnicodeChar is the actual printable character or is zero if the key does not represent a 
> +		 * printable character (control key, function key, etc.).
> +		 */
> +		if (inputkey[index].unicodechar == 0)
> +			fwts_log_info_verbatum(fw, "  UnicodeChar: 0x%4.4" PRIx16 ".", inputkey[index].unicodechar);
> +		else
> +			fwts_log_info_verbatum(fw, "  UnicodeChar: 0x%4.4" PRIx16 " (%c).",
> +										inputkey[index].unicodechar,
> +										(char)inputkey[index].unicodechar);

The keycode's aren't ASCII, you should be referring to EFI Scan Codes
for EFI_SIMPLE_TEXT_INPUT_PROTOCOL in the EFI spec if you want to be
printing these out.  For example, I see:

  UnicodeChar: 0x0010 (^P).

Rather than:

  UnicodeChar: 0x0010 (F6)

> +
> +		index++;
> +
> +	} while (index < key_option->keydata.options.inputkeycount);
> +
> +}
> +
>  static uefidump_info uefidump_info_table[] = {
>  	{ "PlatformLangCodes",	uefidump_info_platform_langcodes },
>  	{ "PlatformLang",	uefidump_info_platform_lang },
> @@ -835,6 +912,14 @@ static void uefidump_var(fwts_framework *fw, fwts_uefi_var *var)
>  		return;
>  	}
>  
> +	/* Check the key option key####. #### is a printed hex value */
> +	if ((strlen(varname) == 7) && (strncmp(varname, "Key", 3) == 0)
> +			&& isxdigit(varname[3]) && isxdigit(varname[4])
> +			&& isxdigit(varname[5]) && isxdigit(varname[6])) {
> +		uefidump_info_keyoption(fw, var);
> +		return;
> +	}
> +
>  	/* otherwise just do a plain old hex dump */
>  	uefidump_var_hexdump(fw, var);
>  }
>