Message-ID: <YYpsq/umygfTb8mM@kroah.com>
Date:   Tue, 9 Nov 2021 13:42:19 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Hans-Gert Dahmen <hans-gert.dahmen@...u.ne>
Cc:     akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        Philipp Deppenwiese <philipp.deppenwiese@...u.ne>,
        Mauro Lima <mauro.lima@...ypsium.com>,
        Richard Hughes <hughsient@...il.com>,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] firmware: export x86_64 platform flash bios region via
 sysfs

On Tue, Nov 09, 2021 at 01:32:48PM +0100, Hans-Gert Dahmen wrote:
> On Tue, 9 Nov 2021, 11:28 Greg KH, <gregkh@...uxfoundation.org> 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.

Do you have a pointer to these complex and buggy drivers anywhere?

> 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.

If it is a "fallback", how is it going to interact on a system where the
SPI driver is loaded?

> > > > > +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.

That's fine, but bind to the real platform device that describes this
hardware!  You are not doing anything like this at all here, you are
just creating a random device in the sysfs tree and instantly allowing
userspace access to raw memory.  You are not actually controlling the
platform device at all.

> The hardware resource here
> is the south bridge for x86_64 CPUs and the module dependencies will
> compile it only for these platforms.

What "module dependencies"?  You do realize that distro kernels enable
all modules to be built. We have an "autoload only the modules that this
hardware needs" infrastructure that you need to tie into here, you are
totally ignoring it (despite it being present for almost 20 years
now...)

> 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.

What do you mean "implicitly bound"?  How does this tie into the driver
model such that it will only be autoloaded, and have the driver bind to
the hardware, that it knows it can control?

That is what needs to be fixed here, you are just claiming that it can
talk to the hardware without having any check at all for this.

> 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?

What resource in the system describes the hardware you wish to bind to?
Write a driver for _that_ resource, and have it be properly autoloaded
when that resource is present in the system.

You have hundreds of examples running on your system today, but as I do
not know where this hardware is described specifically, it's hard to be
able to point you to an example that works like that.

So again, what hardware signature describes the resource you wish to
bind to?

thanks,

greg k-h

Powered by blists - more mailing lists