On Thu, Jan 15, 2015 at 09:25:55PM +0000, Matt Fleming wrote:
> On Mon, 12 Jan, at 08:42:54AM, Peter Jones wrote:
> > Add sysfs files for the EFI System Resource Table (ESRT) under
> > /sys/firmware/efi/esrt and for each EFI System Resource Entry under
> > entries/ as a subdir.
> >
> > The EFI System Resource Table (ESRT) provides a read-only catalog of
> > system components for which the system accepts firmware upgrades via
> > UEFI's "Capsule Update" feature. This module allows userland utilities
> > to evaluate what firmware updates can be applied to this system, and
> > potentially arrange for those updates to occur.
> >
> > The ESRT is described as part of the UEFI specification, in version 2.5
> > which should be available from http://uefi.org/specifications in early
> > 2015. If you're a member of the UEFI Forum, information about its
> > addition to the standard is available as UEFI Mantis 1090.
> >
> > For some hardware platforms, additional restrictions may be found at
> > http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx ,
> > and additional documentation may be found at
> > http://download.microsoft.com/download/5/F/5/5F5D16CD-2530-4289-8019-94C6A20BED3C/windows-uefi-firmware-update-platform.docx
> > .
> >
> > Signed-off-by: Peter Jones <[email protected]>
> > ---
> > drivers/firmware/efi/Makefile | 2 +-
> > drivers/firmware/efi/efi.c | 63 ++++++-
> > drivers/firmware/efi/esrt.c | 402
> > ++++++++++++++++++++++++++++++++++++++++++
> > include/linux/efi.h | 7 +
> > 4 files changed, 472 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/firmware/efi/esrt.c
>
> [...]
>
> > +/*
> > + * ioremap the table, copy it to kmalloced pages, and unmap it.
> > + */
> > +static int esrt_duplicate_pages(void)
> > +{
> > + struct efi_system_resource_table *tmpesrt;
> > + struct efi_system_resource_entry *entries;
> > + size_t size, max;
> > + efi_memory_desc_t md;
> > + int err = -EINVAL;
> > + int rc;
> > +
> > + if (!esrt_table_exists())
> > + return err;
> > +
> > + rc = efi_mem_desc_lookup(efi.esrt, &md);
> > + if (rc < 0) {
> > + pr_err("ESRT header is not in the memory map.\n");
> > + return err;
>
> Probably wanna return 'rc' here? Since efi_mem_desc_lookup() now may
> return -ENOENT or -EINVAL on failure.
Yeah, sure.
> > + }
> > +
> > + max = efi_mem_desc_end(&md);
> > + if (max < 0) {
> > + pr_err("EFI memory descriptor is invalid.\n");
> > + return err;
> > + }
> > +
> > + size = sizeof(*esrt);
> > +
> > + if (max < size) {
> > + pr_err("ESRT header doen't fit on single memory map entry.\n");
> > + return err;
> > + }
>
> This doesn't look right. You're comparing an address 'max' and a size
> 'size'. Unless we have memory descriptors at address 0x0, this condition
> will never be false.
Right you are.
> > +
> > + tmpesrt = ioremap(efi.esrt, size);
> > + if (!tmpesrt) {
> > + pr_err("ioremap failed.\n");
> > + return -ENOMEM;
> > + }
> > +
> > + if (tmpesrt->fw_resource_count > 0 && max - size < sizeof(*entries)) {
> > + pr_err("ESRT memory map entry can only hold the header.\n");
> > + goto err_iounmap;
> > + }
>
> Ditto. Is this ever going to be false?
Clearly in my mind's eye there was a "max -= efi.esrt;" up above somewhere.
> > + /*
> > + * The format doesn't really give us any boundary to test here,
> > + * so I'm making up 128 as the max number of individually updatable
> > + * components we support.
> > + * 128 should be pretty excessive, but there's still some chance
> > + * somebody will do that someday and we'll need to raise this.
> > + */
> > + if (tmpesrt->fw_resource_count > 128) {
> > + pr_err("ESRT says fw_resource_count has very large value %d.\n",
> > + tmpesrt->fw_resource_count);
> > + goto err_iounmap;
> > + }
> > +
> > + /*
> > + * We know it can't be larger than N * sizeof() here, and N is limited
> > + * by the previous test to a small number, so there's no overflow.
> > + */
> > + size += tmpesrt->fw_resource_count * sizeof(*entries);
> > + if (max < size) {
> > + pr_err("ESRT does not fit on single memory map entry.\n");
> > + goto err_iounmap;
> > + }
>
> Ditto^2
Likewise.
> > + for (i = 0; i < le32_to_cpu(esrt->fw_resource_count); i++) {
>
> This caught my eye. Do any of the architectures with UEFI support
> running in big endian mode?
All EFI data is defined as being little endian unless expressly defined
otherwise by UEFI 1.8.1 , which also defines UEFI implementations as
using little endian processor modes. There's also a lot of text in the
per-arch bits that says the same thing.
So there aren't, and there won't be without some major changes
everywhere else.
--
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
