Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Hans de Goede]

Hi,

On 7/28/22 14:07, Greg KH wrote:
> On Thu, Jul 28, 2022 at 01:57:25PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 7/10/22 18:59, Greg KH wrote:
>>> On Sun, Jul 10, 2022 at 09:00:11AM -0700, Jithu Joseph wrote:
>>>> Existing implementation limits IFS images to be loaded only from
>>>> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
>>>
>>> That was by design, why is this suddenly not acceptable?
>>>
>>>> But there are situations where there may be multiple scan files
>>>> that can be run on a particular system stored in /lib/firmware/intel/ifs
>>>
>>> Again, this was by design.
>>>
>>>> E.g.
>>>> 1. Because test contents are larger than the memory reserved for IFS by BIOS
>>>
>>> What does the BIOS have to do with this?
>>>
>>>> 2. To provide increased test coverage
>>>
>>> Test coverage of what?
>>>
>>>> 3. Custom test files to debug certain specific issues in the field
>>>
>>> Why can't you rename the existing file?
>>>
>>>> Renaming each of these to ff-mm-ss.scan and then loading might be
>>>> possible in some environments. But on systems where /lib is read-only
>>>> this is not a practical solution.
>>>
>>> What system puts /lib/ as read-only that you want to have loading
>>> hardware firmware?  That kind of defeats the security implications of
>>> having a read-only /lib/, right?
>>>
>>>> Modify the semantics of the driver file
>>>> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
>>>> it interprets the input as the filename to be loaded.
>>>
>>> So you are now going to allow any file to be read from the system, in an
>>> unknown filesystem namespace, by this driver?
>>
>> @Intel folks to me this is the big blocker which needs to be solved before
>> we can move forward with figuring out how to allow loading multiple
>> different sets of test patterns through IFS.
>>
>> Which in turn is required to remove the broken marking...
>>
>> How about echoing a suffix to be appended to the default filename to
>> the reload attribute? This suffix would then need to be sanity checked
>> to only contain [a-z][0-9] (we don't want '/' but it seems better to
>> limit this further) to avoid directory traversal attacks. 
>>
>> (and echoing an empty suffix can be used to force reloading the
>> default test-patterns after a linux-firmware pkg upgrade)
>>
>> This way we avoid the allowing user to load an arbitrary file issue.
>>
>> Greg, would using a suffix appended to the default filename be
>> acceptable to you as a solution to solving the load arbitrary
>> file issue?
> 
> Not really, a suffix doesn't protect much at all.

?

Currently the driver will always load:

/lib/firmware/intel/ifs/%02x-%02x-%02x.scan

with the "%02x" bits being fixed parts of the CPU model.

My suggestion is to make it:

/lib/firmware/intel/ifs/%02x-%02x-%02x%s.scan

Where the "%s" bit can be supplied by userspace, but it may
only contain [a-z] and [0-9] so no '/' (or other special chars)
so that an attacker cannot use directory traversal to get a file
from below the /lib/firmware/intel/ifs/ dir.

This means that an attacker can only cause a bad file to be
loaded if they have write access to /lib/firmware/intel/ifs/  at
which point they can also just replace the default file. So I
don't see how just allowing userspace to just add a suffix
is a possible problem. Where as the previous arbitrary filename
approach obviously was a problem ?

> This really feels like a "test the product in the factory" type of
> option that someone seems to want to do without just renaming the
> firmware file.  Why this is unique from all other firmware file loading
> in the kernel needs to really be explained here in order to even
> consider justifying this type of change.

First of all I really wish some of the Intel folks would elaborate
more on why multiple test-pattern files are necessary. Ping
anyone@intel, you have all been very quiet in this thread which
is not helpful (not helpful at all really).

Speculating myself as far as I understand IFS is not for factory
tests but for testing in the fields since big cloud vendors have
found that sometimes there are hard to catch CPU defects which
they only find out by running statistics which show that certain
tasks only crash when run on machine a, socket b, core c.

IFS allows them to periodically run a set of CPU selftests to
detect CPU defects (which may sometimes also only show up
over time). AFAIK the multiple test-pattern files thing is
necessary now because it is not possible to put all
possible tests on a single file due to size constraints of
the special RAM into which the test-patterns are loaded.

So the default firmware file will contain a set of defaiult
tests. But there will also be e.g. a special file which
only exercises the avx512 parts of a core, but then more
thoroughly. Again this is all speculation from my side, but
this is my understanding of this all.

Regards,

Hans