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

[Hans-Gert Dahmen]

On Tue, 9 Nov 2021, 11:28 Greg KH, <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Nov 09, 2021 at 09:52:53AM +0100, Hans-Gert Dahmen wrote:
> > On 09.11.21 07:16, Greg KH 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.
> > >
> > > > 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?
> > Because it is very hard to access the SPI flash to read the BIOS contents
> > for (security) analysis and this works without the more complex and
> > unfinished SPI drivers and it does so on a system where we may not access
> > the full /dev/mem.
>
> Why not fix the spi drivers to do this properly?  What is preventing you
> from doing that instead of adding a new user/kernel api that you will
> have to support for the next 20+ years?
>

Because this interface we want to use is inherently tied to the
workings of x86_64 CPUs. It is very stable and easy to use. The SPI
drivers in contrast have a history of being complex, buggy and in
general not reliable. This module will require very little maintenance
and will probably work in the future without needing modification for
newer platforms. It generally is meant as a fallback to the real SPI
flash drivers so that userspace programs are able to provide essential
functionality.

> > > > +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.
> > >
> >
> > Sorry, but I went through tons of code and could not find a single instance
> > where I can use a default group for creation without using a probe function
> > that does the magic for me. Please help me find the correct way of doing
> > this without manually creating and adding kobjects.
>
> The problem here is that you are abusing the platform driver code and
> making a "fake" device that is not attached to anything real, and
> without a driver.  That should not be done, as you do not know what
> hardware this driver is going to be run on.
>
> Bind to the real hardware resource please.

In a previous mail in June you suggested this is some sort of platform
device, that is why I rewrote it as such. The hardware resource here
is the south bridge for x86_64 CPUs and the module dependencies will
compile it only for these platforms. The situation is like, if you
have that CPU, you have the hardware, so it is implicitly bound or it
just will not execute on a machine code level. I feel like your
requirement is somewhat impossible to satisfy and I really don't know
what to do. Please, can anyone help me by pointing out a proper
example elsewhere in the kernel?

Hans-Gert