diff mbox

[2/2] uefi: add in support for new uefivar file system interface

Message ID 1346869717-16964-3-git-send-email-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King Sept. 5, 2012, 6:28 p.m. UTC
From: Colin Ian King <colin.king@canonical.com>

Currently we read uefi variables via a /sys interface, however a
new kernel feature will export these variables via a psuedo file
system, so add support for this.

This is rather a large change.  We abstract out the reading of
uefi variables using two new helper functions:

  fwts_uefi_get_variable_names() and
  fwts_uefi_free_variable_names().

We modify the abstraction of the UEFI variables, before we used
to have a 1-to-1 mapping onto sysfs, now we have to cater for large
sized variable contents, so we now make the data size and variable
name size allocated on the heap.  We also add in support for the
new efifs and abstract this and the /sys variable reading into
two helper functions.

Finally, this patch modifies the uefidump test to use these
changes in the uefi API.
---
 src/lib/include/fwts_uefi.h  |   12 +-
 src/lib/src/fwts_uefi.c      |  339 ++++++++++++++++++++++++++++++++++++++----
 src/uefi/uefidump/uefidump.c |   31 ++--
 3 files changed, 333 insertions(+), 49 deletions(-)

Comments

Keng-Yu Lin Sept. 11, 2012, 6:14 a.m. UTC | #1
On Thu, Sep 6, 2012 at 2:28 AM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently we read uefi variables via a /sys interface, however a
> new kernel feature will export these variables via a psuedo file
> system, so add support for this.
>
> This is rather a large change.  We abstract out the reading of
> uefi variables using two new helper functions:
>
>   fwts_uefi_get_variable_names() and
>   fwts_uefi_free_variable_names().
>
> We modify the abstraction of the UEFI variables, before we used
> to have a 1-to-1 mapping onto sysfs, now we have to cater for large
> sized variable contents, so we now make the data size and variable
> name size allocated on the heap.  We also add in support for the
> new efifs and abstract this and the /sys variable reading into
> two helper functions.
>
> Finally, this patch modifies the uefidump test to use these
> changes in the uefi API.
> ---
>  src/lib/include/fwts_uefi.h  |   12 +-
>  src/lib/src/fwts_uefi.c      |  339 ++++++++++++++++++++++++++++++++++++++----
>  src/uefi/uefidump/uefidump.c |   31 ++--
>  3 files changed, 333 insertions(+), 49 deletions(-)
>
> diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h
> index 763702e..f45027d 100644
> --- a/src/lib/include/fwts_uefi.h
> +++ b/src/lib/include/fwts_uefi.h
> @@ -23,13 +23,13 @@
>  #define FWTS_UEFI_LOAD_ACTIVE 0x00000001
>
>  typedef struct {
> -       uint16_t        varname[512];
> +       uint16_t        *varname;
>         uint8_t         guid[16];
> -       uint64_t        datalen;
> -       uint8_t         data[1024];
> +       size_t          datalen;
> +       uint8_t         *data;
>         uint64_t        status;
>         uint32_t        attributes;
> -} __attribute__((packed)) fwts_uefi_var;
> +} fwts_uefi_var;
>
>  typedef uint8_t  fwts_uefi_mac_addr[32];
>  typedef uint8_t  fwts_uefi_ipv4_addr[4];
> @@ -302,6 +302,8 @@ void fwts_uefi_str16_to_str(char *dst, const size_t len, const uint16_t *src);
>  size_t fwts_uefi_str16len(const uint16_t *str);
>  void fwts_uefi_get_varname(char *varname, const size_t len, const fwts_uefi_var *var);
>  int fwts_uefi_get_variable(const char *varname, fwts_uefi_var *var);
> -int fwts_uefi_set_variable(const char *varname, fwts_uefi_var *var);
> +void fwts_uefi_free_variable(fwts_uefi_var *var);
> +void fwts_uefi_free_variable_names(fwts_list *list);
> +int fwts_uefi_get_variable_names(fwts_list *list);
>
>  #endif
> diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c
> index d4f68f4..c4b2019 100644
> --- a/src/lib/src/fwts_uefi.c
> +++ b/src/lib/src/fwts_uefi.c
> @@ -21,15 +21,129 @@
>  #include <stdio.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> +#include <sys/vfs.h>
>  #include <fcntl.h>
>  #include <unistd.h>
> +#include <dirent.h>
>
>  #include "fwts.h"
>  #include "fwts_uefi.h"
>
> -static inline void fwts_uefi_set_filename(char *filename, const int len, const char *varname)
> +/* Old sysfs uefi packed binary blob variables */
> +typedef struct {
> +       uint16_t        varname[512];
> +       uint8_t         guid[16];
> +       uint64_t        datalen;
> +       uint8_t         data[1024];
> +       uint64_t        status;
> +       uint32_t        attributes;
> +} __attribute__((packed)) fwts_uefi_sys_fs_var;
> +
> +/* New efifs variable */
> +typedef struct {
> +       uint32_t        attributes;
> +       uint8_t         data[0];        /* variable length, depends on file size */
> +} __attribute__((packed)) fwts_uefi_efivars_fs_var;
> +
> +/* Which interface are we using? */
> +#define UEFI_IFACE_UNKNOWN             (0)     /* Not yet known */
> +#define UEFI_IFACE_NONE                        (1)     /* None found */
> +#define UEFI_IFACE_SYSFS               (2)     /* sysfs */
> +#define UEFI_IFACE_EFIVARS             (3)     /* efivar fs */
> +
> +/* File system magic numbers */
> +#define EFIVARS_FS_MAGIC       0x6165676C
> +#define SYS_FS_MAGIC           0x62656572
> +
> +/*
> + *  fwts_uefi_get_interface()
> + *     find which type of EFI variable file system we are using,
> + *     sets path to the name of the file system path, returns
> + *     the file system interface (if found).
> + */
> +static int fwts_uefi_get_interface(char **path)
> +{
> +       static int  efivars_interface = UEFI_IFACE_UNKNOWN;
> +       static char efivar_path[4096];
> +
> +       FILE *fp;
> +       char fstype[1024];
> +       struct statfs statbuf;
> +
> +       if (path == NULL)       /* Sanity check */
> +               return FWTS_ERROR;
> +
> +       /* Already discovered, return the cached values */
> +       if (efivars_interface != UEFI_IFACE_UNKNOWN) {
> +               *path = efivar_path;
> +               return efivars_interface;
> +       }
> +
> +       *efivar_path = '\0';    /* Assume none */
> +
> +       /* Scan mounts to see if sysfs or efivar fs is somewhere else */
> +       if ((fp = fopen("/proc/mounts", "r")) != NULL) {
> +               while (!feof(fp)) {
> +                       char mount[4096];
> +
> +                       if (fscanf(fp, "%*s %4095s %1023s", mount, fstype) == 2) {
> +                               /* Always try to find the newer interface first */
> +                               if (!strcmp(fstype, "efivars")) {
> +                                       strcpy(efivar_path, mount);
> +                                       break;
> +                               }
> +                               /* Failing that, look for sysfs, but don't break out
> +                                  the loop as we need to keep on searching for efivar fs */
> +                               if (!strcmp(fstype, "sysfs"))
> +                                       strcpy(efivar_path, "/sys/firmware/efi/vars");
> +                       }
> +               }
> +       }
> +       fclose(fp);
> +
> +       *path = NULL;
> +
> +       /* No mounted path found, bail out */
> +       if (*efivar_path == '\0') {
> +               efivars_interface = UEFI_IFACE_NONE;
> +               return UEFI_IFACE_NONE;
> +       }
> +
> +       /* Can't stat it, bail out */
> +       if (statfs(efivar_path, &statbuf) < 0) {
> +               efivars_interface = UEFI_IFACE_NONE;
> +               return UEFI_IFACE_NONE;
> +       };
> +
> +       /* We've now found a valid file system we can use */
> +       *path = efivar_path;
> +
> +       if (statbuf.f_type == EFIVARS_FS_MAGIC) {
> +               efivars_interface = UEFI_IFACE_EFIVARS;
> +               return UEFI_IFACE_EFIVARS;
> +       }
> +
> +       if (statbuf.f_type == SYS_FS_MAGIC) {
> +               efivars_interface = UEFI_IFACE_SYSFS;
> +               return UEFI_IFACE_EFIVARS;
> +       }
> +
> +       return UEFI_IFACE_UNKNOWN;
> +}
> +
> +/*
> + *  fwts_uefi_str_to_str16()
> + *     convert 8 bit C string to 16 bit sring.
> + */
> +void fwts_uefi_str_to_str16(uint16_t *dst, const size_t len, const char *src)
>  {
> -       snprintf(filename, len, "/sys/firmware/efi/vars/%s/raw_var", varname);
> +       size_t i = len;
> +
> +       while ((*src) && (i > 1)) {
> +               *dst++ = *src++;
> +               i--;
> +       }
> +       *dst = '\0';
>  }
>
>  /*
> @@ -70,57 +184,230 @@ void fwts_uefi_get_varname(char *varname, const size_t len, const fwts_uefi_var
>  }
>
>  /*
> - *  fwts_uefi_get_variable()
> - *     fetch a UEFI variable given its name.
> + *  fwts_uefi_get_variable_sys_fs()
> + *     fetch a UEFI variable given its name, via sysfs
>   */
> -int fwts_uefi_get_variable(const char *varname, fwts_uefi_var *var)
> +static int fwts_uefi_get_variable_sys_fs(const char *varname, fwts_uefi_var *var, char *path)
>  {
>         int  fd;
> -       int  n;
> -       int  ret = FWTS_OK;
>         char filename[PATH_MAX];
> +       fwts_uefi_sys_fs_var    uefi_sys_fs_var;
>
> -       if ((!varname) || (!var))
> -               return FWTS_ERROR;
> +       memset(var, 0, sizeof(fwts_uefi_var));
>
> -       fwts_uefi_set_filename(filename, sizeof(filename), varname);
> +       snprintf(filename, sizeof(filename), "%s/%s/raw_var", path, varname);
>
>         if ((fd = open(filename, O_RDONLY)) < 0)
>                 return FWTS_ERROR;
>
> -       memset(var, 0, sizeof(fwts_uefi_var));
> -
> -       if ((n = read(fd, var, sizeof(fwts_uefi_var))) != sizeof(fwts_uefi_var))
> -               ret = FWTS_ERROR;
> +       memset(&uefi_sys_fs_var, 0, sizeof(uefi_sys_fs_var));
>
> +       /* Read the raw fixed sized data */
> +       if (read(fd, &uefi_sys_fs_var, sizeof(uefi_sys_fs_var)) != sizeof(uefi_sys_fs_var)) {
> +               close(fd);
> +               return FWTS_ERROR;
> +       }
>         close(fd);
>
> -       return ret;
> +       /* Sanity check datalen is OK */
> +       if (uefi_sys_fs_var.datalen > sizeof(uefi_sys_fs_var.data))
> +               return FWTS_ERROR;
> +
> +       /* Allocate space for the variable name */
> +       var->varname = calloc(1, sizeof(uefi_sys_fs_var.varname));
> +       if (var->varname == NULL)
> +               return FWTS_ERROR;
> +
> +       /* Allocate space for the data */
> +       var->data = calloc(1, (size_t)uefi_sys_fs_var.datalen);
> +       if (var->data == NULL) {
> +               free(var->varname);
> +               return FWTS_ERROR;
> +       }
> +
> +       /* And copy the data */
> +       memcpy(var->varname, uefi_sys_fs_var.varname, sizeof(uefi_sys_fs_var.varname));
> +       memcpy(var->data, uefi_sys_fs_var.data, uefi_sys_fs_var.datalen);
> +       memcpy(var->guid, uefi_sys_fs_var.guid, sizeof(var->guid));
> +       var->datalen = (size_t)uefi_sys_fs_var.datalen;
> +       var->attributes = uefi_sys_fs_var.attributes;
> +       var->status = uefi_sys_fs_var.status;
> +
> +       return FWTS_OK;
>  }
>
>  /*
> - *  fwts_uefi_set_variable()
> - *     write back a UEFI variable given its name and contents in var.
> + *  fwts_uefi_get_variable_efivars_fs()
> + *     fetch a UEFI variable given its name, via efivars fs
>   */
> -int fwts_uefi_set_variable(const char *varname, fwts_uefi_var *var)
> +static int fwts_uefi_get_variable_efivars_fs(const char *varname, fwts_uefi_var *var, char *path)
>  {
>         int  fd;
> -       ssize_t n;
> -       int  ret = FWTS_OK;
>         char filename[PATH_MAX];
> +       struct stat     statbuf;
> +       fwts_uefi_efivars_fs_var        *efivars_fs_var;
> +       size_t varname_len = strlen(varname);
> +
> +       memset(var, 0, sizeof(fwts_uefi_var));
>
> -       if ((!varname) || (!var))
> +       /* Variable names include the GUID, so must be at least 36 chars long */
> +
> +       if (varname_len < 36)
>                 return FWTS_ERROR;
>
> -       fwts_uefi_set_filename(filename, sizeof(filename), varname);
> +       /* Get the GUID */
> +       fwts_guid_str_to_buf(varname + varname_len - 36, var->guid, sizeof(var->guid));
> +
> +       snprintf(filename, sizeof(filename), "%s/%s", path, varname);
>
> -       if ((fd = open(filename, O_WRONLY)) < 0)
> +       if (stat(filename, &statbuf) < 0)
>                 return FWTS_ERROR;
>
> -       if ((n = write(fd, var, sizeof(fwts_uefi_var))) != sizeof(fwts_uefi_var))
> -               ret = FWTS_ERROR;
> +       /* Variable name, less the GUID, in 16 bit ints */
> +       var->varname = calloc(1, (varname_len + 1 - 36)  * sizeof(uint16_t));
> +       if (var->varname == NULL)
> +               return FWTS_ERROR;
>
> +       /* Convert name to internal 16 bit version */
> +       fwts_uefi_str_to_str16(var->varname, varname_len - 36, varname);
> +
> +       /* Need to read the data in one read, so allocate a buffer big enough */
> +       if ((efivars_fs_var = calloc(1, statbuf.st_size)) == NULL) {
> +               free(var->varname);
> +               return FWTS_ERROR;
> +       }
> +
> +       if ((fd = open(filename, O_RDONLY)) < 0) {
> +               free(var->varname);
> +               free(efivars_fs_var);
> +               return FWTS_ERROR;
> +       }
> +
> +       if (read(fd, efivars_fs_var, statbuf.st_size) != statbuf.st_size) {
> +               free(var->varname);
> +               free(efivars_fs_var);
> +               close(fd);
> +               return FWTS_ERROR;
> +       }
>         close(fd);
>
> -       return ret;
> +       var->status = 0;
> +
> +       /*
> +        *  UEFI variable data in efifs is:
> +        *     4 bytes : attributes
> +        *     N bytes : uefi variable contents
> +        */
> +       var->attributes = efivars_fs_var->attributes;
> +
> +       var->datalen = statbuf.st_size - 4;
> +       if ((var->data = calloc(1, var->datalen)) == NULL) {
> +               free(var->varname);
> +               free(efivars_fs_var);
> +               return FWTS_ERROR;
> +       }
> +       memcpy(var->data, efivars_fs_var->data, var->datalen);
> +
> +       free(efivars_fs_var);
> +
> +       return FWTS_OK;
>  }
> +
> +/*
> + *  fwts_uefi_get_variable()
> + *     fetch a UEFI variable given its name.
> + */
> +int fwts_uefi_get_variable(const char *varname, fwts_uefi_var *var)
> +{
> +       char *path;
> +
> +       if ((!varname) || (!var))       /* Sanity checks */
> +               return FWTS_ERROR;
> +
> +       switch (fwts_uefi_get_interface(&path)) {
> +       case UEFI_IFACE_SYSFS:
> +               return fwts_uefi_get_variable_sys_fs(varname, var, path);
> +       case UEFI_IFACE_EFIVARS:
> +               return fwts_uefi_get_variable_efivars_fs(varname, var, path);
> +       default:
> +               return FWTS_ERROR;
> +       }
> +}
> +
> +/*
> + *  fwts_uefi_free_variable()
> + *     free data and varname associated with a UEFI variable as
> + *     fetched by fwts_uefi_get_variable.
> + */
> +void fwts_uefi_free_variable(fwts_uefi_var *var)
> +{
> +       free(var->data);
> +       free(var->varname);
> +}
> +
> +static int fwts_uefi_true_filter(const struct dirent *d)
> +{
> +       return 1;
> +}
> +
> +/*
> + *  fwts_uefi_free_variable_names
> + *     free the list of uefi variable names
> + */
> +void fwts_uefi_free_variable_names(fwts_list *list)
> +{
> +       fwts_list_free_items(list, free);
> +}
> +
> +/*
> + *  fwts_uefi_get_variable_names
> + *     gather a list of all the uefi variable names
> + */
> +int fwts_uefi_get_variable_names(fwts_list *list)
> +{
> +       int i, n;
> +       struct dirent **names = NULL;
> +       char *path;
> +       char *name;
> +       int ret = FWTS_OK;
> +
> +       fwts_list_init(list);
> +
> +       switch (fwts_uefi_get_interface(&path)) {
> +       case UEFI_IFACE_SYSFS:
> +       case UEFI_IFACE_EFIVARS:
> +               break;
> +       default:
> +               return FWTS_ERROR;
> +       }
> +
> +       /* Gather variable names in alphabetical order */
> +       n = scandir(path, &names, fwts_uefi_true_filter, alphasort);
> +       if (n < 0)
> +               return FWTS_ERROR;
> +
> +       for (i = 0; i < n; i++) {
> +               if (names[i]->d_name == NULL)
> +                       continue;
> +               if (!strcmp(names[i]->d_name, "."))
> +                       continue;
> +               if (!strcmp(names[i]->d_name, ".."))
> +                       continue;
> +
> +               name = strdup(names[i]->d_name);
> +               if (name == NULL) {
> +                       ret = FWTS_ERROR;
> +                       fwts_uefi_free_variable_names(list);
> +                       break;
> +                } else
> +                       fwts_list_append(list, name);
> +        }
> +
> +       /* Free dirent names */
> +       for (i = 0; i < n; i++)
> +               free(names[i]);
> +        free(names);
> +
> +        return ret;
> +}
> +
> diff --git a/src/uefi/uefidump/uefidump.c b/src/uefi/uefidump/uefidump.c
> index 1f71107..4b0842f 100644
> --- a/src/uefi/uefidump/uefidump.c
> +++ b/src/uefi/uefidump/uefidump.c
> @@ -17,8 +17,6 @@
>   *
>   */
>
> -#include <dirent.h>
> -
>  #include "fwts.h"
>  #include "fwts_uefi.h"
>
> @@ -466,6 +464,7 @@ static void uefidump_info_bootorder(fwts_framework *fw, fwts_uefi_var *var)
>                         *data++, i < (n - 1) ? "," : "");
>         }
>         fwts_log_info_verbatum(fw, "  Boot Order: %s.", str);
> +       free(str);
>  }
>
>  static void uefidump_info_bootdev(fwts_framework *fw, fwts_uefi_var *var)
> @@ -510,11 +509,6 @@ static uefidump_info uefidump_info_table[] = {
>         { NULL, NULL }
>  };
>
> -static int uefidump_true_filter(const struct dirent *d)
> -{
> -       return 1;
> -}
> -
>  /*
>   *  uefidump_attribute()
>   *     convert attribute into a human readable form
> @@ -568,7 +562,7 @@ static void uefidump_var(fwts_framework *fw, fwts_uefi_var *var)
>
>         /* otherwise just do a plain old hex dump */
>         fwts_log_info_verbatum(fw,  "  Size: %d bytes of data.", (int)var->datalen);
> -       data = (uint8_t*)&var->data;
> +       data = (uint8_t*)var->data;
>
>         for (i=0; i<(int)var->datalen; i+= 16) {
>                 char buffer[128];
> @@ -591,25 +585,26 @@ static int uefidump_init(fwts_framework *fw)
>
>  static int uefidump_test1(fwts_framework *fw)
>  {
> -       int n;
> -       int i;
> -       struct dirent **names = NULL;
> +       fwts_list name_list;
>
> -       n = scandir("/sys/firmware/efi/vars", &names, uefidump_true_filter, alphasort);
> -       if (n <= 0) {
> +       if (fwts_uefi_get_variable_names(&name_list) == FWTS_ERROR) {
>                 fwts_log_info(fw, "Cannot find any UEFI variables.");
>         } else {
> -               for (i=0; i<n; i++) {
> +               fwts_list_link *item;
> +
> +               fwts_list_foreach(item, &name_list) {
>                         fwts_uefi_var var;
> -                       if ((names[i] != NULL) &&
> -                       (fwts_uefi_get_variable(names[i]->d_name, &var) == FWTS_OK)) {
> +                       char *name = fwts_list_data(char *, item);
> +
> +                       if (fwts_uefi_get_variable(name, &var) == FWTS_OK) {
>                                 uefidump_var(fw, &var);
> +                               fwts_uefi_free_variable(&var);
>                                 fwts_log_nl(fw);
>                         }
> -                       free(names[i]);
>                 }
>         }
> -       free(names);
> +
> +       fwts_uefi_free_variable_names(&name_list);
>
>         return FWTS_OK;
>  }
> --
> 1.7.10.4
>
Acked-by: Keng-Yu Lin <kengyu@canonical.com>
Alex Hung Sept. 12, 2012, 7:31 a.m. UTC | #2
On 09/06/2012 02:28 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently we read uefi variables via a /sys interface, however a
> new kernel feature will export these variables via a psuedo file
> system, so add support for this.
>
> This is rather a large change.  We abstract out the reading of
> uefi variables using two new helper functions:
>
>    fwts_uefi_get_variable_names() and
>    fwts_uefi_free_variable_names().
>
> We modify the abstraction of the UEFI variables, before we used
> to have a 1-to-1 mapping onto sysfs, now we have to cater for large
> sized variable contents, so we now make the data size and variable
> name size allocated on the heap.  We also add in support for the
> new efifs and abstract this and the /sys variable reading into
> two helper functions.
>
> Finally, this patch modifies the uefidump test to use these
> changes in the uefi API.
> ---
>   src/lib/include/fwts_uefi.h  |   12 +-
>   src/lib/src/fwts_uefi.c      |  339 ++++++++++++++++++++++++++++++++++++++----
>   src/uefi/uefidump/uefidump.c |   31 ++--
>   3 files changed, 333 insertions(+), 49 deletions(-)
>
> diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h
> index 763702e..f45027d 100644
> --- a/src/lib/include/fwts_uefi.h
> +++ b/src/lib/include/fwts_uefi.h
> @@ -23,13 +23,13 @@
>   #define FWTS_UEFI_LOAD_ACTIVE 0x00000001
>
>   typedef struct {
> -	uint16_t	varname[512];
> +	uint16_t	*varname;
>   	uint8_t		guid[16];
> -	uint64_t	datalen;
> -	uint8_t		data[1024];
> +	size_t		datalen;
> +	uint8_t		*data;
>   	uint64_t	status;
>   	uint32_t	attributes;
> -} __attribute__((packed)) fwts_uefi_var;
> +} fwts_uefi_var;
>
>   typedef uint8_t  fwts_uefi_mac_addr[32];
>   typedef uint8_t  fwts_uefi_ipv4_addr[4];
> @@ -302,6 +302,8 @@ void fwts_uefi_str16_to_str(char *dst, const size_t len, const uint16_t *src);
>   size_t fwts_uefi_str16len(const uint16_t *str);
>   void fwts_uefi_get_varname(char *varname, const size_t len, const fwts_uefi_var *var);
>   int fwts_uefi_get_variable(const char *varname, fwts_uefi_var *var);
> -int fwts_uefi_set_variable(const char *varname, fwts_uefi_var *var);
> +void fwts_uefi_free_variable(fwts_uefi_var *var);
> +void fwts_uefi_free_variable_names(fwts_list *list);
> +int fwts_uefi_get_variable_names(fwts_list *list);
>
>   #endif
> diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c
> index d4f68f4..c4b2019 100644
> --- a/src/lib/src/fwts_uefi.c
> +++ b/src/lib/src/fwts_uefi.c
> @@ -21,15 +21,129 @@
>   #include <stdio.h>
>   #include <sys/types.h>
>   #include <sys/stat.h>
> +#include <sys/vfs.h>
>   #include <fcntl.h>
>   #include <unistd.h>
> +#include <dirent.h>
>
>   #include "fwts.h"
>   #include "fwts_uefi.h"
>
> -static inline void fwts_uefi_set_filename(char *filename, const int len, const char *varname)
> +/* Old sysfs uefi packed binary blob variables */
> +typedef struct {
> +	uint16_t	varname[512];
> +	uint8_t		guid[16];
> +	uint64_t	datalen;
> +	uint8_t		data[1024];
> +	uint64_t	status;
> +	uint32_t	attributes;
> +} __attribute__((packed)) fwts_uefi_sys_fs_var;
> +
> +/* New efifs variable */
> +typedef struct {
> +	uint32_t	attributes;
> +	uint8_t		data[0];	/* variable length, depends on file size */
> +} __attribute__((packed)) fwts_uefi_efivars_fs_var;
> +
> +/* Which interface are we using? */
> +#define UEFI_IFACE_UNKNOWN 		(0)	/* Not yet known */
> +#define UEFI_IFACE_NONE			(1)	/* None found */
> +#define UEFI_IFACE_SYSFS		(2)	/* sysfs */
> +#define UEFI_IFACE_EFIVARS		(3)	/* efivar fs */
> +
> +/* File system magic numbers */
> +#define EFIVARS_FS_MAGIC	0x6165676C
> +#define SYS_FS_MAGIC		0x62656572
> +
> +/*
> + *  fwts_uefi_get_interface()
> + *	find which type of EFI variable file system we are using,
> + *	sets path to the name of the file system path, returns
> + *	the file system interface (if found).
> + */
> +static int fwts_uefi_get_interface(char **path)
> +{
> +	static int  efivars_interface = UEFI_IFACE_UNKNOWN;
> +	static char efivar_path[4096];
> +
> +	FILE *fp;
> +	char fstype[1024];
> +	struct statfs statbuf;
> +
> +	if (path == NULL)	/* Sanity check */
> +		return FWTS_ERROR;
> +
> +	/* Already discovered, return the cached values */
> +	if (efivars_interface != UEFI_IFACE_UNKNOWN) {
> +		*path = efivar_path;
> +		return efivars_interface;
> +	}
> +
> +	*efivar_path = '\0';	/* Assume none */
> +
> +	/* Scan mounts to see if sysfs or efivar fs is somewhere else */
> +	if ((fp = fopen("/proc/mounts", "r")) != NULL) {
> +		while (!feof(fp)) {
> +			char mount[4096];
> +
> +			if (fscanf(fp, "%*s %4095s %1023s", mount, fstype) == 2) {
> +				/* Always try to find the newer interface first */
> +				if (!strcmp(fstype, "efivars")) {
> +					strcpy(efivar_path, mount);
> +					break;
> +				}
> +				/* Failing that, look for sysfs, but don't break out
> +				   the loop as we need to keep on searching for efivar fs */
> +				if (!strcmp(fstype, "sysfs"))
> +					strcpy(efivar_path, "/sys/firmware/efi/vars");
> +			}
> +		}
> +	}
> +	fclose(fp);
> +
> +	*path = NULL;
> +
> +	/* No mounted path found, bail out */
> +	if (*efivar_path == '\0') {
> +		efivars_interface = UEFI_IFACE_NONE;
> +		return UEFI_IFACE_NONE;
> +	}
> +
> +	/* Can't stat it, bail out */
> +	if (statfs(efivar_path, &statbuf) < 0) {
> +		efivars_interface = UEFI_IFACE_NONE;
> +		return UEFI_IFACE_NONE;
> +	};
> +
> +	/* We've now found a valid file system we can use */
> +	*path = efivar_path;
> +
> +	if (statbuf.f_type == EFIVARS_FS_MAGIC) {
> +		efivars_interface = UEFI_IFACE_EFIVARS;
> +		return UEFI_IFACE_EFIVARS;
> +	}
> +
> +	if (statbuf.f_type == SYS_FS_MAGIC) {
> +		efivars_interface = UEFI_IFACE_SYSFS;
> +		return UEFI_IFACE_EFIVARS;
> +	}
> +
> +	return UEFI_IFACE_UNKNOWN;
> +}
> +
> +/*
> + *  fwts_uefi_str_to_str16()
> + *	convert 8 bit C string to 16 bit sring.
> + */
> +void fwts_uefi_str_to_str16(uint16_t *dst, const size_t len, const char *src)
>   {
> -	snprintf(filename, len, "/sys/firmware/efi/vars/%s/raw_var", varname);
> +	size_t i = len;
> +
> +	while ((*src) && (i > 1)) {
> +		*dst++ = *src++;
> +		i--;
> +	}
> +	*dst = '\0';
>   }
>
>   /*
> @@ -70,57 +184,230 @@ void fwts_uefi_get_varname(char *varname, const size_t len, const fwts_uefi_var
>   }
>
>   /*
> - *  fwts_uefi_get_variable()
> - *	fetch a UEFI variable given its name.
> + *  fwts_uefi_get_variable_sys_fs()
> + *	fetch a UEFI variable given its name, via sysfs
>    */
> -int fwts_uefi_get_variable(const char *varname, fwts_uefi_var *var)
> +static int fwts_uefi_get_variable_sys_fs(const char *varname, fwts_uefi_var *var, char *path)
>   {
>   	int  fd;
> -	int  n;
> -	int  ret = FWTS_OK;
>   	char filename[PATH_MAX];
> +	fwts_uefi_sys_fs_var	uefi_sys_fs_var;
>
> -	if ((!varname) || (!var))
> -		return FWTS_ERROR;
> +	memset(var, 0, sizeof(fwts_uefi_var));
>
> -	fwts_uefi_set_filename(filename, sizeof(filename), varname);
> +	snprintf(filename, sizeof(filename), "%s/%s/raw_var", path, varname);
>
>   	if ((fd = open(filename, O_RDONLY)) < 0)
>   		return FWTS_ERROR;
>
> -	memset(var, 0, sizeof(fwts_uefi_var));
> -
> -	if ((n = read(fd, var, sizeof(fwts_uefi_var))) != sizeof(fwts_uefi_var))
> -		ret = FWTS_ERROR;
> +	memset(&uefi_sys_fs_var, 0, sizeof(uefi_sys_fs_var));
>
> +	/* Read the raw fixed sized data */
> +	if (read(fd, &uefi_sys_fs_var, sizeof(uefi_sys_fs_var)) != sizeof(uefi_sys_fs_var)) {
> +		close(fd);
> +		return FWTS_ERROR;
> +	}
>   	close(fd);
>
> -	return ret;
> +	/* Sanity check datalen is OK */
> +	if (uefi_sys_fs_var.datalen > sizeof(uefi_sys_fs_var.data))
> +		return FWTS_ERROR;
> +
> +	/* Allocate space for the variable name */
> +	var->varname = calloc(1, sizeof(uefi_sys_fs_var.varname));
> +	if (var->varname == NULL)
> +		return FWTS_ERROR;
> +
> +	/* Allocate space for the data */
> +	var->data = calloc(1, (size_t)uefi_sys_fs_var.datalen);
> +	if (var->data == NULL) {
> +		free(var->varname);
> +		return FWTS_ERROR;
> +	}
> +
> +	/* And copy the data */
> +	memcpy(var->varname, uefi_sys_fs_var.varname, sizeof(uefi_sys_fs_var.varname));
> +	memcpy(var->data, uefi_sys_fs_var.data, uefi_sys_fs_var.datalen);
> +	memcpy(var->guid, uefi_sys_fs_var.guid, sizeof(var->guid));
> +	var->datalen = (size_t)uefi_sys_fs_var.datalen;
> +	var->attributes = uefi_sys_fs_var.attributes;
> +	var->status = uefi_sys_fs_var.status;
> +
> +	return FWTS_OK;
>   }
>
>   /*
> - *  fwts_uefi_set_variable()
> - *	write back a UEFI variable given its name and contents in var.
> + *  fwts_uefi_get_variable_efivars_fs()
> + *	fetch a UEFI variable given its name, via efivars fs
>    */
> -int fwts_uefi_set_variable(const char *varname, fwts_uefi_var *var)
> +static int fwts_uefi_get_variable_efivars_fs(const char *varname, fwts_uefi_var *var, char *path)
>   {
>   	int  fd;
> -	ssize_t n;
> -	int  ret = FWTS_OK;
>   	char filename[PATH_MAX];
> +	struct stat	statbuf;
> +	fwts_uefi_efivars_fs_var	*efivars_fs_var;
> +	size_t varname_len = strlen(varname);
> +
> +	memset(var, 0, sizeof(fwts_uefi_var));
>
> -	if ((!varname) || (!var))
> +	/* Variable names include the GUID, so must be at least 36 chars long */
> +
> +	if (varname_len < 36)
>   		return FWTS_ERROR;
>
> -	fwts_uefi_set_filename(filename, sizeof(filename), varname);
> +	/* Get the GUID */
> +	fwts_guid_str_to_buf(varname + varname_len - 36, var->guid, sizeof(var->guid));
> +
> +	snprintf(filename, sizeof(filename), "%s/%s", path, varname);
>
> -	if ((fd = open(filename, O_WRONLY)) < 0)
> +	if (stat(filename, &statbuf) < 0)
>   		return FWTS_ERROR;
>
> -	if ((n = write(fd, var, sizeof(fwts_uefi_var))) != sizeof(fwts_uefi_var))
> -		ret = FWTS_ERROR;
> +	/* Variable name, less the GUID, in 16 bit ints */
> +	var->varname = calloc(1, (varname_len + 1 - 36)  * sizeof(uint16_t));
> +	if (var->varname == NULL)
> +		return FWTS_ERROR;
>
> +	/* Convert name to internal 16 bit version */
> +	fwts_uefi_str_to_str16(var->varname, varname_len - 36, varname);
> +
> +	/* Need to read the data in one read, so allocate a buffer big enough */
> +	if ((efivars_fs_var = calloc(1, statbuf.st_size)) == NULL) {
> +		free(var->varname);
> +		return FWTS_ERROR;
> +	}
> +
> +	if ((fd = open(filename, O_RDONLY)) < 0) {
> +		free(var->varname);
> +		free(efivars_fs_var);
> +		return FWTS_ERROR;
> +	}
> +
> +	if (read(fd, efivars_fs_var, statbuf.st_size) != statbuf.st_size) {
> +		free(var->varname);
> +		free(efivars_fs_var);
> +		close(fd);
> +		return FWTS_ERROR;
> +	}
>   	close(fd);
>
> -	return ret;
> +	var->status = 0;
> +
> +	/*
> +	 *  UEFI variable data in efifs is:
> +	 *     4 bytes : attributes
> +	 *     N bytes : uefi variable contents
> + 	 */
> +	var->attributes = efivars_fs_var->attributes;
> +
> +	var->datalen = statbuf.st_size - 4;
> +	if ((var->data = calloc(1, var->datalen)) == NULL) {
> +		free(var->varname);
> +		free(efivars_fs_var);
> +		return FWTS_ERROR;
> +	}
> +	memcpy(var->data, efivars_fs_var->data, var->datalen);
> +
> +	free(efivars_fs_var);
> +
> +	return FWTS_OK;
>   }
> +
> +/*
> + *  fwts_uefi_get_variable()
> + *	fetch a UEFI variable given its name.
> + */
> +int fwts_uefi_get_variable(const char *varname, fwts_uefi_var *var)
> +{
> +	char *path;
> +
> +	if ((!varname) || (!var))	/* Sanity checks */
> +		return FWTS_ERROR;
> +
> +	switch (fwts_uefi_get_interface(&path)) {
> +	case UEFI_IFACE_SYSFS:
> +		return fwts_uefi_get_variable_sys_fs(varname, var, path);
> +	case UEFI_IFACE_EFIVARS:
> +		return fwts_uefi_get_variable_efivars_fs(varname, var, path);
> +	default:
> +		return FWTS_ERROR;
> +	}
> +}
> +
> +/*
> + *  fwts_uefi_free_variable()
> + *	free data and varname associated with a UEFI variable as
> + *	fetched by fwts_uefi_get_variable.
> + */
> +void fwts_uefi_free_variable(fwts_uefi_var *var)
> +{
> +	free(var->data);
> +	free(var->varname);
> +}
> +
> +static int fwts_uefi_true_filter(const struct dirent *d)
> +{
> +	return 1;
> +}
> +
> +/*
> + *  fwts_uefi_free_variable_names
> + *	free the list of uefi variable names
> + */
> +void fwts_uefi_free_variable_names(fwts_list *list)
> +{
> +	fwts_list_free_items(list, free);
> +}
> +
> +/*
> + *  fwts_uefi_get_variable_names
> + *	gather a list of all the uefi variable names
> + */
> +int fwts_uefi_get_variable_names(fwts_list *list)
> +{
> +	int i, n;
> +	struct dirent **names = NULL;
> +	char *path;
> +	char *name;
> +	int ret = FWTS_OK;
> +
> +	fwts_list_init(list);
> +
> +	switch (fwts_uefi_get_interface(&path)) {
> +	case UEFI_IFACE_SYSFS:
> +	case UEFI_IFACE_EFIVARS:
> +		break;
> +	default:
> +		return FWTS_ERROR;
> +	}
> +
> +	/* Gather variable names in alphabetical order */
> +	n = scandir(path, &names, fwts_uefi_true_filter, alphasort);
> +	if (n < 0)
> +		return FWTS_ERROR;
> +
> +	for (i = 0; i < n; i++) {
> +		if (names[i]->d_name == NULL)
> +			continue;
> +		if (!strcmp(names[i]->d_name, "."))
> +			continue;
> +		if (!strcmp(names[i]->d_name, ".."))
> +			continue;
> +
> +		name = strdup(names[i]->d_name);
> +		if (name == NULL) {
> +			ret = FWTS_ERROR;
> +			fwts_uefi_free_variable_names(list);
> +			break;
> +                } else
> +			fwts_list_append(list, name);
> +        }
> +
> +	/* Free dirent names */
> + 	for (i = 0; i < n; i++)
> +		free(names[i]);
> +        free(names);
> +
> +        return ret;
> +}
> +
> diff --git a/src/uefi/uefidump/uefidump.c b/src/uefi/uefidump/uefidump.c
> index 1f71107..4b0842f 100644
> --- a/src/uefi/uefidump/uefidump.c
> +++ b/src/uefi/uefidump/uefidump.c
> @@ -17,8 +17,6 @@
>    *
>    */
>
> -#include <dirent.h>
> -
>   #include "fwts.h"
>   #include "fwts_uefi.h"
>
> @@ -466,6 +464,7 @@ static void uefidump_info_bootorder(fwts_framework *fw, fwts_uefi_var *var)
>   			*data++, i < (n - 1) ? "," : "");
>   	}
>   	fwts_log_info_verbatum(fw, "  Boot Order: %s.", str);
> +	free(str);
>   }
>
>   static void uefidump_info_bootdev(fwts_framework *fw, fwts_uefi_var *var)
> @@ -510,11 +509,6 @@ static uefidump_info uefidump_info_table[] = {
>   	{ NULL, NULL }
>   };
>
> -static int uefidump_true_filter(const struct dirent *d)
> -{
> -	return 1;
> -}
> -
>   /*
>    *  uefidump_attribute()
>    *	convert attribute into a human readable form
> @@ -568,7 +562,7 @@ static void uefidump_var(fwts_framework *fw, fwts_uefi_var *var)
>
>   	/* otherwise just do a plain old hex dump */
>   	fwts_log_info_verbatum(fw,  "  Size: %d bytes of data.", (int)var->datalen);
> -	data = (uint8_t*)&var->data;
> +	data = (uint8_t*)var->data;
>
>   	for (i=0; i<(int)var->datalen; i+= 16) {
>   		char buffer[128];
> @@ -591,25 +585,26 @@ static int uefidump_init(fwts_framework *fw)
>
>   static int uefidump_test1(fwts_framework *fw)
>   {
> -	int n;
> -	int i;
> -	struct dirent **names = NULL;
> +	fwts_list name_list;
>
> -	n = scandir("/sys/firmware/efi/vars", &names, uefidump_true_filter, alphasort);
> -	if (n <= 0) {
> +	if (fwts_uefi_get_variable_names(&name_list) == FWTS_ERROR) {
>   		fwts_log_info(fw, "Cannot find any UEFI variables.");
>   	} else {
> -		for (i=0; i<n; i++) {
> +		fwts_list_link *item;
> +
> +		fwts_list_foreach(item, &name_list) {
>   			fwts_uefi_var var;
> -			if ((names[i] != NULL) &&
> -		    	(fwts_uefi_get_variable(names[i]->d_name, &var) == FWTS_OK)) {
> +			char *name = fwts_list_data(char *, item);
> +
> +			if (fwts_uefi_get_variable(name, &var) == FWTS_OK) {
>   				uefidump_var(fw, &var);
> +				fwts_uefi_free_variable(&var);
>   				fwts_log_nl(fw);
>   			}
> -			free(names[i]);
>   		}
>   	}
> -	free(names);
> +
> +	fwts_uefi_free_variable_names(&name_list);
>
>   	return FWTS_OK;
>   }
>

Acked-by: Alex Hung <alex.hung@canonical.com>
diff mbox

Patch

diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h
index 763702e..f45027d 100644
--- a/src/lib/include/fwts_uefi.h
+++ b/src/lib/include/fwts_uefi.h
@@ -23,13 +23,13 @@ 
 #define FWTS_UEFI_LOAD_ACTIVE 0x00000001
 
 typedef struct {
-	uint16_t	varname[512];
+	uint16_t	*varname;
 	uint8_t		guid[16];
-	uint64_t	datalen;
-	uint8_t		data[1024];
+	size_t		datalen;
+	uint8_t		*data;
 	uint64_t	status;
 	uint32_t	attributes;
-} __attribute__((packed)) fwts_uefi_var;
+} fwts_uefi_var;
 
 typedef uint8_t  fwts_uefi_mac_addr[32];
 typedef uint8_t  fwts_uefi_ipv4_addr[4];
@@ -302,6 +302,8 @@  void fwts_uefi_str16_to_str(char *dst, const size_t len, const uint16_t *src);
 size_t fwts_uefi_str16len(const uint16_t *str);
 void fwts_uefi_get_varname(char *varname, const size_t len, const fwts_uefi_var *var);
 int fwts_uefi_get_variable(const char *varname, fwts_uefi_var *var);
-int fwts_uefi_set_variable(const char *varname, fwts_uefi_var *var);
+void fwts_uefi_free_variable(fwts_uefi_var *var);
+void fwts_uefi_free_variable_names(fwts_list *list);
+int fwts_uefi_get_variable_names(fwts_list *list);
 
 #endif
diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c
index d4f68f4..c4b2019 100644
--- a/src/lib/src/fwts_uefi.c
+++ b/src/lib/src/fwts_uefi.c
@@ -21,15 +21,129 @@ 
 #include <stdio.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/vfs.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <dirent.h>
 
 #include "fwts.h"
 #include "fwts_uefi.h"
 
-static inline void fwts_uefi_set_filename(char *filename, const int len, const char *varname)
+/* Old sysfs uefi packed binary blob variables */
+typedef struct {
+	uint16_t	varname[512];
+	uint8_t		guid[16];
+	uint64_t	datalen;
+	uint8_t		data[1024];
+	uint64_t	status;
+	uint32_t	attributes;
+} __attribute__((packed)) fwts_uefi_sys_fs_var;
+
+/* New efifs variable */
+typedef struct {
+	uint32_t	attributes;
+	uint8_t		data[0];	/* variable length, depends on file size */
+} __attribute__((packed)) fwts_uefi_efivars_fs_var;
+
+/* Which interface are we using? */
+#define UEFI_IFACE_UNKNOWN 		(0)	/* Not yet known */
+#define UEFI_IFACE_NONE			(1)	/* None found */
+#define UEFI_IFACE_SYSFS		(2)	/* sysfs */
+#define UEFI_IFACE_EFIVARS		(3)	/* efivar fs */
+
+/* File system magic numbers */
+#define EFIVARS_FS_MAGIC	0x6165676C
+#define SYS_FS_MAGIC		0x62656572
+
+/*
+ *  fwts_uefi_get_interface()
+ *	find which type of EFI variable file system we are using,
+ *	sets path to the name of the file system path, returns
+ *	the file system interface (if found).
+ */
+static int fwts_uefi_get_interface(char **path)
+{
+	static int  efivars_interface = UEFI_IFACE_UNKNOWN;
+	static char efivar_path[4096];
+
+	FILE *fp;
+	char fstype[1024];
+	struct statfs statbuf;
+
+	if (path == NULL)	/* Sanity check */
+		return FWTS_ERROR;
+
+	/* Already discovered, return the cached values */
+	if (efivars_interface != UEFI_IFACE_UNKNOWN) {
+		*path = efivar_path;
+		return efivars_interface;
+	}
+
+	*efivar_path = '\0';	/* Assume none */
+
+	/* Scan mounts to see if sysfs or efivar fs is somewhere else */
+	if ((fp = fopen("/proc/mounts", "r")) != NULL) {
+		while (!feof(fp)) {
+			char mount[4096];
+
+			if (fscanf(fp, "%*s %4095s %1023s", mount, fstype) == 2) {
+				/* Always try to find the newer interface first */
+				if (!strcmp(fstype, "efivars")) {
+					strcpy(efivar_path, mount);
+					break;
+				}
+				/* Failing that, look for sysfs, but don't break out
+				   the loop as we need to keep on searching for efivar fs */
+				if (!strcmp(fstype, "sysfs"))
+					strcpy(efivar_path, "/sys/firmware/efi/vars");
+			}
+		}
+	}
+	fclose(fp);
+
+	*path = NULL;
+
+	/* No mounted path found, bail out */
+	if (*efivar_path == '\0') {
+		efivars_interface = UEFI_IFACE_NONE;
+		return UEFI_IFACE_NONE;
+	}
+
+	/* Can't stat it, bail out */
+	if (statfs(efivar_path, &statbuf) < 0) {
+		efivars_interface = UEFI_IFACE_NONE;
+		return UEFI_IFACE_NONE;
+	};
+
+	/* We've now found a valid file system we can use */
+	*path = efivar_path;
+
+	if (statbuf.f_type == EFIVARS_FS_MAGIC) {
+		efivars_interface = UEFI_IFACE_EFIVARS;
+		return UEFI_IFACE_EFIVARS;
+	}
+
+	if (statbuf.f_type == SYS_FS_MAGIC) {
+		efivars_interface = UEFI_IFACE_SYSFS;
+		return UEFI_IFACE_EFIVARS;
+	}
+
+	return UEFI_IFACE_UNKNOWN;
+}
+
+/*
+ *  fwts_uefi_str_to_str16()
+ *	convert 8 bit C string to 16 bit sring.
+ */
+void fwts_uefi_str_to_str16(uint16_t *dst, const size_t len, const char *src)
 {
-	snprintf(filename, len, "/sys/firmware/efi/vars/%s/raw_var", varname);
+	size_t i = len;
+
+	while ((*src) && (i > 1)) {
+		*dst++ = *src++;
+		i--;
+	}
+	*dst = '\0';
 }
 
 /*
@@ -70,57 +184,230 @@  void fwts_uefi_get_varname(char *varname, const size_t len, const fwts_uefi_var
 }
 
 /*
- *  fwts_uefi_get_variable()
- *	fetch a UEFI variable given its name.
+ *  fwts_uefi_get_variable_sys_fs()
+ *	fetch a UEFI variable given its name, via sysfs
  */
-int fwts_uefi_get_variable(const char *varname, fwts_uefi_var *var)
+static int fwts_uefi_get_variable_sys_fs(const char *varname, fwts_uefi_var *var, char *path)
 {
 	int  fd;
-	int  n;
-	int  ret = FWTS_OK;
 	char filename[PATH_MAX];
+	fwts_uefi_sys_fs_var	uefi_sys_fs_var;
 
-	if ((!varname) || (!var))
-		return FWTS_ERROR;
+	memset(var, 0, sizeof(fwts_uefi_var));
 
-	fwts_uefi_set_filename(filename, sizeof(filename), varname);
+	snprintf(filename, sizeof(filename), "%s/%s/raw_var", path, varname);
 
 	if ((fd = open(filename, O_RDONLY)) < 0)
 		return FWTS_ERROR;
 
-	memset(var, 0, sizeof(fwts_uefi_var));
-
-	if ((n = read(fd, var, sizeof(fwts_uefi_var))) != sizeof(fwts_uefi_var))
-		ret = FWTS_ERROR;
+	memset(&uefi_sys_fs_var, 0, sizeof(uefi_sys_fs_var));
 
+	/* Read the raw fixed sized data */
+	if (read(fd, &uefi_sys_fs_var, sizeof(uefi_sys_fs_var)) != sizeof(uefi_sys_fs_var)) {
+		close(fd);
+		return FWTS_ERROR;
+	}
 	close(fd);
 
-	return ret;
+	/* Sanity check datalen is OK */
+	if (uefi_sys_fs_var.datalen > sizeof(uefi_sys_fs_var.data))
+		return FWTS_ERROR;
+
+	/* Allocate space for the variable name */
+	var->varname = calloc(1, sizeof(uefi_sys_fs_var.varname));
+	if (var->varname == NULL)
+		return FWTS_ERROR;
+
+	/* Allocate space for the data */
+	var->data = calloc(1, (size_t)uefi_sys_fs_var.datalen);
+	if (var->data == NULL) {
+		free(var->varname);
+		return FWTS_ERROR;
+	}
+
+	/* And copy the data */
+	memcpy(var->varname, uefi_sys_fs_var.varname, sizeof(uefi_sys_fs_var.varname));
+	memcpy(var->data, uefi_sys_fs_var.data, uefi_sys_fs_var.datalen);
+	memcpy(var->guid, uefi_sys_fs_var.guid, sizeof(var->guid));
+	var->datalen = (size_t)uefi_sys_fs_var.datalen;
+	var->attributes = uefi_sys_fs_var.attributes;
+	var->status = uefi_sys_fs_var.status;
+
+	return FWTS_OK;
 }
 
 /*
- *  fwts_uefi_set_variable()
- *	write back a UEFI variable given its name and contents in var.
+ *  fwts_uefi_get_variable_efivars_fs()
+ *	fetch a UEFI variable given its name, via efivars fs
  */
-int fwts_uefi_set_variable(const char *varname, fwts_uefi_var *var)
+static int fwts_uefi_get_variable_efivars_fs(const char *varname, fwts_uefi_var *var, char *path)
 {
 	int  fd;
-	ssize_t n;
-	int  ret = FWTS_OK;
 	char filename[PATH_MAX];
+	struct stat	statbuf;
+	fwts_uefi_efivars_fs_var	*efivars_fs_var;
+	size_t varname_len = strlen(varname);
+
+	memset(var, 0, sizeof(fwts_uefi_var));
 
-	if ((!varname) || (!var))
+	/* Variable names include the GUID, so must be at least 36 chars long */
+
+	if (varname_len < 36)
 		return FWTS_ERROR;
 
-	fwts_uefi_set_filename(filename, sizeof(filename), varname);
+	/* Get the GUID */
+	fwts_guid_str_to_buf(varname + varname_len - 36, var->guid, sizeof(var->guid));
+
+	snprintf(filename, sizeof(filename), "%s/%s", path, varname);
 
-	if ((fd = open(filename, O_WRONLY)) < 0)
+	if (stat(filename, &statbuf) < 0)
 		return FWTS_ERROR;
 
-	if ((n = write(fd, var, sizeof(fwts_uefi_var))) != sizeof(fwts_uefi_var))
-		ret = FWTS_ERROR;
+	/* Variable name, less the GUID, in 16 bit ints */
+	var->varname = calloc(1, (varname_len + 1 - 36)  * sizeof(uint16_t));
+	if (var->varname == NULL)
+		return FWTS_ERROR;
 
+	/* Convert name to internal 16 bit version */
+	fwts_uefi_str_to_str16(var->varname, varname_len - 36, varname);
+
+	/* Need to read the data in one read, so allocate a buffer big enough */
+	if ((efivars_fs_var = calloc(1, statbuf.st_size)) == NULL) {
+		free(var->varname);
+		return FWTS_ERROR;
+	}
+
+	if ((fd = open(filename, O_RDONLY)) < 0) {
+		free(var->varname);
+		free(efivars_fs_var);
+		return FWTS_ERROR;
+	}
+
+	if (read(fd, efivars_fs_var, statbuf.st_size) != statbuf.st_size) {
+		free(var->varname);
+		free(efivars_fs_var);
+		close(fd);
+		return FWTS_ERROR;
+	}
 	close(fd);
 
-	return ret;
+	var->status = 0;
+
+	/*
+	 *  UEFI variable data in efifs is:
+	 *     4 bytes : attributes
+	 *     N bytes : uefi variable contents
+ 	 */
+	var->attributes = efivars_fs_var->attributes;
+
+	var->datalen = statbuf.st_size - 4;
+	if ((var->data = calloc(1, var->datalen)) == NULL) {
+		free(var->varname);
+		free(efivars_fs_var);
+		return FWTS_ERROR;
+	}
+	memcpy(var->data, efivars_fs_var->data, var->datalen);
+
+	free(efivars_fs_var);
+
+	return FWTS_OK;
 }
+
+/*
+ *  fwts_uefi_get_variable()
+ *	fetch a UEFI variable given its name.
+ */
+int fwts_uefi_get_variable(const char *varname, fwts_uefi_var *var)
+{
+	char *path;
+
+	if ((!varname) || (!var))	/* Sanity checks */
+		return FWTS_ERROR;
+
+	switch (fwts_uefi_get_interface(&path)) {
+	case UEFI_IFACE_SYSFS:
+		return fwts_uefi_get_variable_sys_fs(varname, var, path);
+	case UEFI_IFACE_EFIVARS:
+		return fwts_uefi_get_variable_efivars_fs(varname, var, path);
+	default:
+		return FWTS_ERROR;
+	}
+}
+
+/*
+ *  fwts_uefi_free_variable()
+ *	free data and varname associated with a UEFI variable as
+ *	fetched by fwts_uefi_get_variable.
+ */
+void fwts_uefi_free_variable(fwts_uefi_var *var)
+{
+	free(var->data);
+	free(var->varname);
+}
+
+static int fwts_uefi_true_filter(const struct dirent *d)
+{
+	return 1;
+}
+
+/*
+ *  fwts_uefi_free_variable_names
+ *	free the list of uefi variable names
+ */
+void fwts_uefi_free_variable_names(fwts_list *list)
+{
+	fwts_list_free_items(list, free);
+}
+
+/*
+ *  fwts_uefi_get_variable_names
+ *	gather a list of all the uefi variable names
+ */
+int fwts_uefi_get_variable_names(fwts_list *list)
+{
+	int i, n;
+	struct dirent **names = NULL;
+	char *path;
+	char *name;
+	int ret = FWTS_OK;
+
+	fwts_list_init(list);
+
+	switch (fwts_uefi_get_interface(&path)) {
+	case UEFI_IFACE_SYSFS:
+	case UEFI_IFACE_EFIVARS:
+		break;
+	default:
+		return FWTS_ERROR;
+	}
+
+	/* Gather variable names in alphabetical order */
+	n = scandir(path, &names, fwts_uefi_true_filter, alphasort);
+	if (n < 0)
+		return FWTS_ERROR;
+
+	for (i = 0; i < n; i++) {
+		if (names[i]->d_name == NULL)
+			continue;
+		if (!strcmp(names[i]->d_name, "."))
+			continue;
+		if (!strcmp(names[i]->d_name, ".."))
+			continue;
+
+		name = strdup(names[i]->d_name);
+		if (name == NULL) {
+			ret = FWTS_ERROR;
+			fwts_uefi_free_variable_names(list);
+			break;
+                } else
+			fwts_list_append(list, name);
+        }
+
+	/* Free dirent names */
+ 	for (i = 0; i < n; i++)
+		free(names[i]);
+        free(names);
+
+        return ret;
+}
+
diff --git a/src/uefi/uefidump/uefidump.c b/src/uefi/uefidump/uefidump.c
index 1f71107..4b0842f 100644
--- a/src/uefi/uefidump/uefidump.c
+++ b/src/uefi/uefidump/uefidump.c
@@ -17,8 +17,6 @@ 
  *
  */
 
-#include <dirent.h>
-
 #include "fwts.h"
 #include "fwts_uefi.h"
 
@@ -466,6 +464,7 @@  static void uefidump_info_bootorder(fwts_framework *fw, fwts_uefi_var *var)
 			*data++, i < (n - 1) ? "," : "");
 	}
 	fwts_log_info_verbatum(fw, "  Boot Order: %s.", str);
+	free(str);
 }
 
 static void uefidump_info_bootdev(fwts_framework *fw, fwts_uefi_var *var)
@@ -510,11 +509,6 @@  static uefidump_info uefidump_info_table[] = {
 	{ NULL, NULL }
 };
 
-static int uefidump_true_filter(const struct dirent *d)
-{
-	return 1;
-}
-
 /*
  *  uefidump_attribute()
  *	convert attribute into a human readable form
@@ -568,7 +562,7 @@  static void uefidump_var(fwts_framework *fw, fwts_uefi_var *var)
 
 	/* otherwise just do a plain old hex dump */
 	fwts_log_info_verbatum(fw,  "  Size: %d bytes of data.", (int)var->datalen);
-	data = (uint8_t*)&var->data;
+	data = (uint8_t*)var->data;
 
 	for (i=0; i<(int)var->datalen; i+= 16) {
 		char buffer[128];
@@ -591,25 +585,26 @@  static int uefidump_init(fwts_framework *fw)
 
 static int uefidump_test1(fwts_framework *fw)
 {
-	int n;
-	int i;
-	struct dirent **names = NULL;
+	fwts_list name_list;
 
-	n = scandir("/sys/firmware/efi/vars", &names, uefidump_true_filter, alphasort);
-	if (n <= 0) {
+	if (fwts_uefi_get_variable_names(&name_list) == FWTS_ERROR) {
 		fwts_log_info(fw, "Cannot find any UEFI variables.");
 	} else {
-		for (i=0; i<n; i++) {
+		fwts_list_link *item;
+
+		fwts_list_foreach(item, &name_list) {
 			fwts_uefi_var var;
-			if ((names[i] != NULL) && 
-		    	(fwts_uefi_get_variable(names[i]->d_name, &var) == FWTS_OK)) {
+			char *name = fwts_list_data(char *, item);
+
+			if (fwts_uefi_get_variable(name, &var) == FWTS_OK) {
 				uefidump_var(fw, &var);
+				fwts_uefi_free_variable(&var);
 				fwts_log_nl(fw);
 			}
-			free(names[i]);
 		}
 	}
-	free(names);
+
+	fwts_uefi_free_variable_names(&name_list);
 
 	return FWTS_OK;
 }