linux-kernel - Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAArk9MN99YjKV2AKCYsUqh7LNVCb2ddvcSnRgGGsXePkM6Q86Q@mail.gmail.com>
Date:   Tue, 9 Nov 2021 10:55:54 -0300
From:   Mauro Lima <mauro.lima@...ypsium.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Hans-Gert Dahmen <hans-gert.dahmen@...u.ne>,
        akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        philipp.deppenwiese@...u.ne, Richard Hughes <hughsient@...il.com>,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

Hi all,

On Tue, Nov 9, 2021 at 3:16 AM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
> > Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> > on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> > for pen-testing, security analysis and malware detection on kernels
> > which restrict module loading and/or access to /dev/mem.
>
> That feels like a big security hole we would be opening up for no good
> reason.
Please, can you explain why this could be a security hole?
IMO if the host is compromised the attacker already has information
about the BIOS version, and after a quick lookup they know the BIOS
vulnerabilities or the lack of them.
>
> > It will be used by the open source Converged Security Suite.
> > https://github.com/9elements/converged-security-suite
>
> What is the reason for this, and what use is this new interface?
In Eclypsium we are also interested in being able to dump the actual
binary from hosts and compare them to see if they are corrupted
somehow.
>
> >
> > Signed-off-by: Hans-Gert Dahmen <hans-gert.dahmen@...u.ne>
> > ---
> >  drivers/firmware/Kconfig             |  9 +++
> >  drivers/firmware/Makefile            |  1 +
> >  drivers/firmware/x86_64_flash_mmap.c | 86 ++++++++++++++++++++++++++++
>
> You forgot to document the new sysfs files in Documentation/ABI/ :(
>
>
> >  3 files changed, 96 insertions(+)
> >  create mode 100644 drivers/firmware/x86_64_flash_mmap.c
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index 75cb91055c17..27c2d0b074e0 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -293,6 +293,15 @@ config TURRIS_MOX_RWTM
> >         other manufacturing data and also utilize the Entropy Bit Generator
> >         for hardware random number generation.
> >
> > +config X86_64_FLASH_MMAP
> > +     tristate "Export platform flash memory-mapped BIOS region"
> > +     depends on X86_64
> > +     help
> > +       Export the memory-mapped BIOS region of the platform SPI flash as
> > +       a read-only sysfs binary attribute on X86_64 systems. The first 16MiB
> > +       will be accessible via /sys/devices/platform/flash_mmap/bios_region
> > +       for security and malware analysis for example.
> > +
> >  source "drivers/firmware/arm_ffa/Kconfig"
> >  source "drivers/firmware/broadcom/Kconfig"
> >  source "drivers/firmware/cirrus/Kconfig"
> > diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> > index 4e58cb474a68..60dc4ea08705 100644
> > --- a/drivers/firmware/Makefile
> > +++ b/drivers/firmware/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_SYSFB_SIMPLEFB)        += sysfb_simplefb.o
> >  obj-$(CONFIG_TI_SCI_PROTOCOL)        += ti_sci.o
> >  obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
> >  obj-$(CONFIG_TURRIS_MOX_RWTM)        += turris-mox-rwtm.o
> > +obj-$(CONFIG_X86_64_FLASH_MMAP)      += x86_64_flash_mmap.o
> >
> >  obj-y                                += arm_ffa/
> >  obj-y                                += arm_scmi/
> > diff --git a/drivers/firmware/x86_64_flash_mmap.c b/drivers/firmware/x86_64_flash_mmap.c
> > new file mode 100644
> > index 000000000000..23d8655d17bb
> > --- /dev/null
> > +++ b/drivers/firmware/x86_64_flash_mmap.c
> > @@ -0,0 +1,86 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Export the memory-mapped BIOS region of the platform SPI flash as
> > + * a read-only sysfs binary attribute on X86_64 systems.
> > + *
> > + * Copyright © 2021 immune GmbH
> > + */
> > +
> > +#include <linux/version.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define FLASH_REGION_START 0xFF000000ULL
> > +#define FLASH_REGION_SIZE 0x1000000ULL
> > +#define FLASH_REGION_MASK (FLASH_REGION_SIZE - 1)
> > +
> > +struct platform_device *pdev;
> > +
> > +static ssize_t bios_region_read(struct file *file, struct kobject *kobj,
> > +                             struct bin_attribute *bin_attr, char *buffer,
> > +                             loff_t offset, size_t count)
> > +{
> > +     resource_size_t pa;
> > +     size_t copysize, remapsize;
> > +     void __iomem *va;
> > +
> > +     offset = offset & FLASH_REGION_MASK;
> > +     pa = (FLASH_REGION_START + offset) & PAGE_MASK;
> > +
> > +     if ((offset + count) > FLASH_REGION_SIZE)
> > +             copysize = FLASH_REGION_SIZE - offset;
> > +     else
> > +             copysize = min(count, PAGE_SIZE);
> > +
> > +     if (((offset & ~PAGE_MASK) + copysize) > PAGE_SIZE)
> > +             remapsize = 2 * PAGE_SIZE;
> > +     else
> > +             remapsize = PAGE_SIZE;
> > +
> > +     va = ioremap(pa, remapsize);
> > +     memcpy_fromio(buffer, va, copysize);
> > +     iounmap(va);
> > +
> > +     return copysize;
> > +}
> > +
> > +static BIN_ATTR_RO(bios_region, FLASH_REGION_SIZE);
> > +
> > +static struct bin_attribute *flash_mmap_attrs[] = { &bin_attr_bios_region,
> > +                                                 NULL };
> > +
> > +static const struct attribute_group flash_mmap_group = {
> > +     .bin_attrs = flash_mmap_attrs,
> > +};
> > +
> > +static int __init flash_mmap_init(void)
> > +{
> > +     int ret;
> > +
> > +     pdev = platform_device_register_simple("flash_mmap", -1, NULL, 0);
> > +     if (IS_ERR(pdev))
> > +             return PTR_ERR(pdev);
> > +
> > +     ret = sysfs_create_group(&pdev->dev.kobj, &flash_mmap_group);
>
> You just raced with userspace and lost :(
>
> Please set the attribute to the platform driver before you create the
> device.
>
> Also, you just bound this driver to ANY platform that it was loaded on,
> with no actual detection of the hardware present, which feels like it
> could cause big problems on all platforms.  Please, if you really want
> to do this, restrict it to hardware that actually has the hardware you
> are wanting to access, not all machines in the world.
>
> thanks,
>
> greg k-h