On Fri, Apr 10, 2020 at 06:53:49PM +0100, Ignat Korchagin wrote:
> On Fri, Apr 10, 2020 at 6:38 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> >
> > Hello Ignat,
> >
> > Thanks for the patch.
> >
> > On Fri, 10 Apr 2020 at 19:28, Ignat Korchagin <ignat@xxxxxxxxxxxxxx> wrote:
> > >
> > > Currently Linux exposes the physical address of the EFI configuration table via
> > > sysfs, but not the number of entries.
> > >
> >
> > It does so on x86 only, and the purpose is specifically defined as
> > kexec. This is for a good reason: kexec on x86 EFI machines has
> > accumulated some historical quirks dealing with issues that do not
> > exist on other architectures.
> >
> > > The number of entries for the EFI configuration table is located in the EFI
> > > system table and the EFI system table is not exposed, so there is no way for
> > > a userspace application to reliably navigate the EFI configuration table.
> > >
> > > One potential use case for such a userspace program would be a monitoring agent,
> > > which parses Image Execution Information Table from the EFI configuration table
> > > and reports all the UEFI executables, which have been denied execution due to
> > > the enforced Secure Boot policy thus providing intrusion detection capabilities.
> > >
> >
> > Exposing a physical address via syfs and using /dev/mem to scoop up
> > the data is not a robust, secure or portable interface, especially in
> > the quoted context of a UEFI secure boot enabled system.
>
> TBH, it is not hard to find this number by scanning the same mapped region for
> EFI system table (which is easily identifiable by its signature). So
> security is not an
> issue here, although robustness and portability indeed.
If security was an issue, /dev/mem would be disabled.
>
> > If you need to access this table from userland, I suggest we come up
> > with a generic method that does not rely on /dev/mem. It would be even
> > better if we could come up with some infrastructure that makes this
> > easily extendable to other configuration tables. But simply exposing
> > the address and size of the config table array in memory is not the
> > right way.
>
> Would you prefer something closer to the efivars filesystem then?
Maybe something like DMI_SYSFS?
>
> > > Signed-off-by: Ignat Korchagin <ignat@xxxxxxxxxxxxxx>
> > > ---
> > > Documentation/ABI/testing/sysfs-firmware-efi | 12 +++++++++-
> > > arch/x86/platform/efi/efi.c | 24 +++++++++++++++-----
> > > drivers/firmware/efi/efi.c | 2 ++
> > > 3 files changed, 31 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> > > index 5e4d0b27cdfe..37d0364c16cb 100644
> > > --- a/Documentation/ABI/testing/sysfs-firmware-efi
> > > +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> > > @@ -17,7 +17,9 @@ Date: December 2013
> > > Contact: Dave Young <dyoung@xxxxxxxxxx>
> > > Description: It shows the physical address of config table entry in the EFI
> > > system table.
> > > -Users: Kexec
> > > +Users: Kexec, userspace tools for reading information from UEFI
> > > + configuration table (for example, Image Execution Information
> > > + Table)
> > >
> > > What: /sys/firmware/efi/systab
> > > Date: April 2005
> > > @@ -36,3 +38,11 @@ Description: Displays the content of the Runtime Configuration Interface
> > > Table version 2 on Dell EMC PowerEdge systems in binary format
> > > Users: It is used by Dell EMC OpenManage Server Administrator tool to
> > > populate BIOS setup page.
> > > +
> > > +What: /sys/firmware/efi/nr_tables
> > > +Date: April 2020
> > > +Contact: linux-efi@xxxxxxxxxxxxxxx
> > > +Description: It shows number of entries in the config table entry in the EFI
> > > + system table.
> > > +Users: Userspace tools for reading information from UEFI configuration
> > > + table (for example, Image Execution Information Table)
> > > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > > index 1aae5302501d..83574db489d4 100644
> > > --- a/arch/x86/platform/efi/efi.c
> > > +++ b/arch/x86/platform/efi/efi.c
> > > @@ -57,9 +57,9 @@
> > > static unsigned long efi_systab_phys __initdata;
> > > static unsigned long prop_phys = EFI_INVALID_TABLE_ADDR;
> > > static unsigned long uga_phys = EFI_INVALID_TABLE_ADDR;
> > > -static unsigned long efi_runtime, efi_nr_tables;
> > > +static unsigned long efi_runtime;
> > >
> > > -unsigned long efi_fw_vendor, efi_config_table;
> > > +unsigned long efi_fw_vendor, efi_config_table, efi_nr_tables;
> > >
> > > static const efi_config_table_type_t arch_tables[] __initconst = {
> > > {EFI_PROPERTIES_TABLE_GUID, "PROP", &prop_phys},
> > > @@ -963,20 +963,29 @@ char *efi_systab_show_arch(char *str)
> > >
> > > #define EFI_FIELD(var) efi_ ## var
> > >
> > > -#define EFI_ATTR_SHOW(name) \
> > > +#define EFI_ATTR_SHOW_ADDR(name) \
> > > static ssize_t name##_show(struct kobject *kobj, \
> > > struct kobj_attribute *attr, char *buf) \
> > > { \
> > > return sprintf(buf, "0x%lx\n", EFI_FIELD(name)); \
> > > }
> > >
> > > -EFI_ATTR_SHOW(fw_vendor);
> > > -EFI_ATTR_SHOW(runtime);
> > > -EFI_ATTR_SHOW(config_table);
> > > +#define EFI_ATTR_SHOW_ULONG(name) \
> > > +static ssize_t name##_show(struct kobject *kobj, \
> > > + struct kobj_attribute *attr, char *buf) \
> > > +{ \
> > > + return sprintf(buf, "%lu\n", EFI_FIELD(name)); \
> > > +}
> > > +
> > > +EFI_ATTR_SHOW_ADDR(fw_vendor);
> > > +EFI_ATTR_SHOW_ADDR(runtime);
> > > +EFI_ATTR_SHOW_ADDR(config_table);
> > > +EFI_ATTR_SHOW_ULONG(nr_tables);
> > >
> > > struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > > struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > > struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > > +struct kobj_attribute efi_attr_nr_tables = __ATTR_RO(nr_tables);
> > >
> > > umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
> > > {
> > > @@ -990,6 +999,9 @@ umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
> > > } else if (attr == &efi_attr_config_table.attr) {
> > > if (efi_config_table == EFI_INVALID_TABLE_ADDR)
> > > return 0;
> > > + } else if (attr == &efi_attr_nr_tables.attr) {
> > > + if (efi_config_table == EFI_INVALID_TABLE_ADDR)
> > > + return 0;
> > > }
> > > return attr->mode;
> > > }
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index 911a2bd0f6b7..0fe064582f33 100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -150,6 +150,7 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
> > > extern __weak struct kobj_attribute efi_attr_fw_vendor;
> > > extern __weak struct kobj_attribute efi_attr_runtime;
> > > extern __weak struct kobj_attribute efi_attr_config_table;
> > > +extern __weak struct kobj_attribute efi_attr_nr_tables;
> > > static struct kobj_attribute efi_attr_fw_platform_size =
> > > __ATTR_RO(fw_platform_size);
> > >
> > > @@ -159,6 +160,7 @@ static struct attribute *efi_subsys_attrs[] = {
> > > &efi_attr_fw_vendor.attr,
> > > &efi_attr_runtime.attr,
> > > &efi_attr_config_table.attr,
> > > + &efi_attr_nr_tables.attr,
> > > NULL,
> > > };
> > >
> > > --
> > > 2.20.1
> > >
