Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <42cea157-55a2-bd12-335b-6348f0ff6525@immu.ne>
Date:   Tue, 9 Nov 2021 09:52:53 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH] firmware: export x86_64 platform flash bios region via
 sysfs
Content-Language: en-US
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        philipp.deppenwiese@immu.ne, mauro.lima@eclypsium.com,
        hughsient@gmail.com, platform-driver-x86@vger.kernel.org
References: <20211109000130.42361-1-hans-gert.dahmen@immu.ne>
 <YYoSPjF3M05dR0PX@kroah.com>
From:   Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>
In-Reply-To: <YYoSPjF3M05dR0PX@kroah.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

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

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

I ave already proven that it works on all x64 Intel platforms here [1]. 
It nearly impossible to prove for AMD b/c of the lack of documentation, 
but we tested it on several old Bulldozer system and so far the memory 
was always mapped. I feel that adding more hardware detection just adds 
complexity. Anyway, what do you suggest? Use CPUID to check if the 
vendor is AMD or Intel?

Hans-Gert Dahmen