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

[Russ Weight <russell.h.weight@intel.com>]

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.

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

Do you have a preference between (a) and (b)?

Thanks,
- Russ
> thanks,
>
> greg k-h