Re: [PATCH 02/12] fpga: sec-mgr: enable secure updates

From: Greg KH <gregkh@linuxfoundation.org>
To: Russ Weight <russell.h.weight@intel.com>
Cc: Moritz Fischer <mdf@kernel.org>, Tom Rix <trix@redhat.com>,
	linux-fpga@vger.kernel.org, moritzf@google.com
Subject: Re: [PATCH 02/12] fpga: sec-mgr: enable secure updates
Date: Tue, 2 Nov 2021 18:06:30 +0100	[thread overview]
Message-ID: <YYFwFleJXqqc9ZW/@kroah.com> (raw)
In-Reply-To: <62ae6249-8561-7e85-2aa8-3dd49646180a@intel.com>

On Tue, Nov 02, 2021 at 09:25:10AM -0700, Russ Weight wrote:
> 
> 
> On 8/4/21 8:12 AM, Greg KH wrote:
> > On Wed, Aug 04, 2021 at 07:58:34AM -0700, Moritz Fischer wrote:
> >> On Wed, Aug 04, 2021 at 09:37:45AM +0200, Greg KH wrote:
> >>> On Tue, Aug 03, 2021 at 12:02:24PM -0700, Russ Weight wrote:
> >>>>
> >>>> On 8/2/21 10:49 PM, Greg KH wrote:
> >>>>>> If the request_firmware() implementation is not acceptable, then would
> >>>>>> you agree that an IOCTL implementation is our best option?
> >>>>> There is no difference in the end between using an ioctl, or a sysfs
> >>>>> file, to provide the filename of your firmware, don't get hung up on
> >>>>> that.
> >>>> I meant to suggest that passing file data (not a filename) through an
> >>>> IOCTL might be better for this use case than trying to use request_firmware.
> >>>> We have to, somehow, allow the user to point us to the desired image
> >>>> data (which could be a root-entry-hash, or an FPGA image). We can't
> >>>> really use a fixed filename modified by device version as many of
> >>>> the devices do.
> >>> Ah, yes, a "normal" write command might be best for this as that can be
> >>> properly containerized and controlled.
> >>>
> >>>>> By providing a "filename", you are going around all of the namespace and
> >>>>> other "container" protection that the kernel provides, and allowing
> >>>>> processes to potentially load files that are normally outside of their
> >>>>> scope to the hardware.  If you are willing to allow that security
> >>>>> "escape", wonderful, but you better document the heck out of it and
> >>>>> explain why this is allowed for your special hardware and use case.
> >>>>>
> >>>>> As you are expecting this to work "in the cloud", I do not think that
> >>>>> the operators of such hardware are really going to be all that happy to
> >>>>> see this type of interface given these reasons.
> >>>>>
> >>>>> What is wrong with the current fpga firmware api that somehow is lacking
> >>>>> for your special hardware, that other devices do not have to worry
> >>>>> about?
> >>>> The existing framework wants to update the live image in the FPGA,
> >>>> whereas for this device, we are passing signed data to BMC firmware
> >>>> which will store it in FLASH to be loaded on a subsequent boot of
> >>>> the card.
> >>>>
> >>>> The existing framework needs to manage FPGA state, whereas for this
> >>>> device, it is just a transfer of signed data. We also have to handle
> >>>> a total transfer/authentication time of up to 45 minutes, so we are
> >>>> using a kernel worker thread for the update.
> >>>>
> >>>> Perhaps the name, fpga security manager, is wrong? Maybe something
> >>>> like fpga_sec_image_xfer is better?
> >>> It does not sound like this has anything to do with "security", and
> >>> rather is just a normal firmware upload, so "fpga_image_upload()"
> >>> perhaps?
> >> I had originally suggested 'load' and 'persist' or 'load' and 'update or
> >> something of that sort.
> >>
> >> Taking one step back, maybe the case could be made for a generic
> >> 'persistent firmware' update framework that addresses use-cases that
> >> require updating firmware that may take extended periods of time.
> > There should not be a problem with using the existing firmware layer for
> > images that take long periods of time, as long as you are not wanting to
> > see any potential progress :)
> >
> > So how about just adding anything missing to the existing firmware
> > subsystem.  It's attempting to handle all use cases already, if it is
> > missing one, no harm in adding more options there...
> Hi Greg,
> 
> We have had a lot of internal (to Intel) discussion about how to
> organize the support for uploading FPGA images. It would be helpful
> to know which of the following two options you find the least
> disturbing :-)
> 
> Background: We are uploading signed, self-describing images that are
> authenticated and dispositioned by the Card BMC. These could result
> in FLASH updates for FPGA images, BMC images, firmware, or security
> keys.  They could also result in a temporary authentication
> certificate being loaded into RAM as part of a multi-step key
> provisioning process.
> 
> Options:
> (a) A single API that facilitates the upload of a data stream
> without analyzing the stream contents, relying on the lower-level
> driver and/or HW to accept or reject the data.

That is the firmware api we have today, please use that like all other
drivers should be using.

> (b) Multiple, targeted APIs (e.g. IOCTL_FPGA_IMAGE_UPDATE,
> IOCTL_BMC_IMAGE_UPDATE, IOCTL_KEY_UPDATE, IOCTL_KEY_CANCEL) that
> each interpret the stream type and reject them if they don't
> correspond to the API target.

Please no, do not make a zillion "custom" ioctls.  That way lies
madness.  Will you want to maintain them all for the next 30+ years?

thanks,

greg k-h