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

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jul 28, 2022 at 02:52:06PM +0200, Hans de Goede wrote:
> 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

Ah, sorry, I skimmed that, you are right, that would be fine.  But still
odd to ever be needed in a real system.

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

Who knows, Intel doesn't say so we can't really guess :(

greg k-h