[PATCH] efi/x86: Expose number of entries in the EFI configuration table via sysfs

[PATCH] efi/x86: Expose number of entries in the EFI configuration table via sysfs

[Ignat Korchagin]

Currently Linux exposes the physical address of the EFI configuration table via
sysfs, but not the number of entries.

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.

Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 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@redhat.com>
 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@vger.kernel.org
+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

Re: [PATCH] efi/x86: Expose number of entries in the EFI configuration table via sysfs

[Ard Biesheuvel]

Hello Ignat,

Thanks for the patch.

On Fri, 10 Apr 2020 at 19:28, Ignat Korchagin <ignat@cloudflare.com> 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.

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.

> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
> ---
>  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@redhat.com>
>  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@vger.kernel.org
> +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
>

Re: [PATCH] efi/x86: Expose number of entries in the EFI configuration table via sysfs

[Ignat Korchagin]

On Fri, Apr 10, 2020 at 6:38 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Hello Ignat,
>
> Thanks for the patch.
>
> On Fri, 10 Apr 2020 at 19:28, Ignat Korchagin <ignat@cloudflare.com> 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 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?

> > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
> > ---
> >  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@redhat.com>
> >  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@vger.kernel.org
> > +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
> >

Re: [PATCH] efi/x86: Expose number of entries in the EFI configuration table via sysfs

[Ard Biesheuvel]

(+ Peter)

On Fri, 10 Apr 2020 at 19:54, Ignat Korchagin <ignat@cloudflare.com> wrote:
>
> On Fri, Apr 10, 2020 at 6:38 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Hello Ignat,
> >
> > Thanks for the patch.
> >
> > On Fri, 10 Apr 2020 at 19:28, Ignat Korchagin <ignat@cloudflare.com> 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 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?
>

The problem with EFI configuration tables is that there is no standard
way to discover their size. Each GUID denotes a different type of
table, each of which has its own structure and layout in memory.

We already record and expose a substantial set of config tables, but
perhaps it is time to add a generic layer as well. I have cc'ed Peter,
which may have some observations of his own to share on this topic.

A pseudo file system might work, as long as it only exposes tables
that have been explicitly registered for this, along with the size of
each individual table. But then it becomes complicated if you want to
expose things like ACPI or SMBIOS, which are not flat tables, but sets
of tables pointing to other tables etc etc.

Re: [PATCH] efi/x86: Expose number of entries in the EFI configuration table via sysfs

[Arvind Sankar]

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@kernel.org> wrote:
> >
> > Hello Ignat,
> >
> > Thanks for the patch.
> >
> > On Fri, 10 Apr 2020 at 19:28, Ignat Korchagin <ignat@cloudflare.com> 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@cloudflare.com>
> > > ---
> > >  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@redhat.com>
> > >  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@vger.kernel.org
> > > +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
> > >