diff mbox series

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

Message ID 20210517023200.52707-3-mdf@kernel.org (mailing list archive)
State New
Headers show
Series FPGA Security Manager for 5.14 | expand

Commit Message

Moritz Fischer May 17, 2021, 2:31 a.m. UTC
From: Russ Weight <russell.h.weight@intel.com>

Extend the FPGA Security Manager class driver to
include an update/filename sysfs node that can be used
to initiate a secure update.  The filename of a secure
update file (BMC image, FPGA image, Root Entry Hash image,
or Code Signing Key cancellation image) can be written to
this sysfs entry to cause a secure update to occur.

The write of the filename will return immediately, and the
update will begin in the context of a kernel worker thread.
This tool utilizes the request_firmware framework, which
requires that the image file reside under /lib/firmware.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../ABI/testing/sysfs-class-fpga-sec-mgr      |  13 ++
 drivers/fpga/fpga-sec-mgr.c                   | 160 ++++++++++++++++++
 include/linux/fpga/fpga-sec-mgr.h             |  48 ++++++
 3 files changed, 221 insertions(+)

Comments

Greg Kroah-Hartman May 17, 2021, 5:32 a.m. UTC | #1
On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
> From: Russ Weight <russell.h.weight@intel.com>
> 
> Extend the FPGA Security Manager class driver to
> include an update/filename sysfs node that can be used
> to initiate a secure update.  The filename of a secure
> update file (BMC image, FPGA image, Root Entry Hash image,
> or Code Signing Key cancellation image) can be written to
> this sysfs entry to cause a secure update to occur.

Why is userspace responsible for triggering this?  Passing a "filename"
into the kernel and having it do something with it is ripe for major
problems, please do not.
Russ Weight May 17, 2021, 7:37 p.m. UTC | #2
On 5/16/21 10:32 PM, Greg KH wrote:
> On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
>> From: Russ Weight <russell.h.weight@intel.com>
>>
>> Extend the FPGA Security Manager class driver to
>> include an update/filename sysfs node that can be used
>> to initiate a secure update.  The filename of a secure
>> update file (BMC image, FPGA image, Root Entry Hash image,
>> or Code Signing Key cancellation image) can be written to
>> this sysfs entry to cause a secure update to occur.
> Why is userspace responsible for triggering this?  Passing a "filename"
> into the kernel and having it do something with it is ripe for major
> problems, please do not.
>
I am using the "request_firmware" framework, which accepts a filename
and finds the firmware file under /lib/firmware.

Is this not an acceptable use for request_firmware?

- Russ
Russ Weight July 30, 2021, 1:23 a.m. UTC | #3
On 5/17/21 12:37 PM, Russ Weight wrote:
> On 5/16/21 10:32 PM, Greg KH wrote:
>> On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
>>> From: Russ Weight <russell.h.weight@intel.com>
>>>
>>> Extend the FPGA Security Manager class driver to
>>> include an update/filename sysfs node that can be used
>>> to initiate a secure update.  The filename of a secure
>>> update file (BMC image, FPGA image, Root Entry Hash image,
>>> or Code Signing Key cancellation image) can be written to
>>> this sysfs entry to cause a secure update to occur.
>> Why is userspace responsible for triggering this?  Passing a "filename"
>> into the kernel and having it do something with it is ripe for major
>> problems, please do not.
>>
> I am using the "request_firmware" framework, which accepts a filename
> and finds the firmware file under /lib/firmware.
>
> Is this not an acceptable use for request_firmware?
>
> - Russ

Hi Greg,

The dev_release fixes that you asked for in the FPGA Manager, Bridge, and
Region code are almost complete. I'm trying to get back to the FPGA
security manager patch set. Your previous comments challenged some basic
assumptions. If it is OK, I would like to get some clarity before I rework
the patches.

Overview: The goal is to pass signed data to the PCIe FPGA Card's BMC. BMC
firmware will authenticate the data and disposition it. In our case, FPGA
image data, root entry hashes, and cancellation keys are authenticated
and then stored in FLASH memory. The patchset contains both a class
driver and a platform driver.

Example Output of Current Driver:

        [root@psera2-dell24 update]# echo -n intel/dcp_2_0_page0_0x2020002000000237_signed.bin > filename
        [root@psera2-dell24 update]# while :; do cat status remaining_size ; sleep 3; done
        preparing
        8094720
        <- snip ->
        writing
        8012800
        <- snip ->
        programming
        0
        <- snip ->
        programming
        0
        <- snip ->
        idle
        0
        ^C
        [root@psera2-dell24 update]# cat error
        [root@psera2-dell24 update]#


Assumptions:

(1) request_firmware(). We had assumed that making use of the existing
request_firmware() would be preferred. This requires providing a filename
under /lib/firmware to the framework. You commented (above): "Passing a
'filename' into the kernel and having it do something with it is ripe for
problems, please do not." Unless you have additional comments on this, I
will plan to NOT use the request_firmware framework.

(2) sysfs interface. We had assumed that a sysfs interface would
be preferred. If I am not passing a filename, then I think my only option
with sysfs is to use a binary attribute and cat the data in. Is that
acceptable, or would it be better to use IOCTLs to pass the data?

(3) Platform Driver. This driver is for the BMC on the FPGA Card.
I think that is similar to the SOC model. This is actually a sub-driver
for the MAX10 BMC driver. Other platform drivers (e.g. hwmon) have already
been accepted as subdrivers for the BMC. Is the platform driver the
right approach? If not, can you please point me in the right direction?

(4) Kernel worker thread: The worst case update is currently about 45
minutes (newer implementations are shorter). We chose to do the data
transfer in a kernel worker thread and then make it possible for
userspace to monitor the progress (currently via sysfs). Any concerns
about doing the transfer in a background thread?

(5) New vs modified driver: Perhaps "FPGA Security Manager" is not
a good name. Simply put, the driver passes signed data from the host
to the Card BMC for authentication and disposition. I looked at
merging the class driver with the FPGA Manager, but:

a) Secure updates make no use of the existing FPGA Manager code (FPGA
state management, etc). The only similarity is in the ops data structure.

b) Because of the kernel worker thread the driver remove functionality
adds complexity that is not helpful to the FPGA Manager.

I can, of course, combine the drivers anyway if you think that is better.

I appreciate any feedback/direction you can give.

Thanks,
- Russ
Greg Kroah-Hartman July 30, 2021, 11:18 a.m. UTC | #4
On Thu, Jul 29, 2021 at 06:23:12PM -0700, Russ Weight wrote:
> 
> 
> On 5/17/21 12:37 PM, Russ Weight wrote:
> > On 5/16/21 10:32 PM, Greg KH wrote:
> >> On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
> >>> From: Russ Weight <russell.h.weight@intel.com>
> >>>
> >>> Extend the FPGA Security Manager class driver to
> >>> include an update/filename sysfs node that can be used
> >>> to initiate a secure update.  The filename of a secure
> >>> update file (BMC image, FPGA image, Root Entry Hash image,
> >>> or Code Signing Key cancellation image) can be written to
> >>> this sysfs entry to cause a secure update to occur.
> >> Why is userspace responsible for triggering this?  Passing a "filename"
> >> into the kernel and having it do something with it is ripe for major
> >> problems, please do not.
> >>
> > I am using the "request_firmware" framework, which accepts a filename
> > and finds the firmware file under /lib/firmware.
> >
> > Is this not an acceptable use for request_firmware?
> >
> > - Russ
> 
> Hi Greg,
> 
> The dev_release fixes that you asked for in the FPGA Manager, Bridge, and
> Region code are almost complete. I'm trying to get back to the FPGA
> security manager patch set. Your previous comments challenged some basic
> assumptions. If it is OK, I would like to get some clarity before I rework
> the patches.

Note, I do not have the time, nor the inclination, to help your company
out with design reviews at this point in time.  If you have questions
about this, please discuss it with the open source managers at Intel,
they know the current situation quite well.

I am glad to review patches that have gone through the proper internal
Intel patch review process and then sent out to the community.

That being said, I will make one comment on your questions below:

> (1) request_firmware(). We had assumed that making use of the existing
> request_firmware() would be preferred. This requires providing a filename
> under /lib/firmware to the framework. You commented (above): "Passing a
> 'filename' into the kernel and having it do something with it is ripe for
> problems, please do not." Unless you have additional comments on this, I
> will plan to NOT use the request_firmware framework.

request_firmware() should always be used for requesting firmware for a
device.  Having an api where you write a random filename to a sysfs file
and have that loaded by the kernel seems ripe for disaster though, I can
not think of any other in-kernel user of the firmware api that does
this.  Or are there examples that I have just missed?

thanks,

greg k-h
Russ Weight Aug. 2, 2021, 6:31 p.m. UTC | #5
On 7/30/21 4:18 AM, Greg KH wrote:
> On Thu, Jul 29, 2021 at 06:23:12PM -0700, Russ Weight wrote:
>>
>> On 5/17/21 12:37 PM, Russ Weight wrote:
>>> On 5/16/21 10:32 PM, Greg KH wrote:
>>>> On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
>>>>> From: Russ Weight <russell.h.weight@intel.com>
>>>>>
>>>>> Extend the FPGA Security Manager class driver to
>>>>> include an update/filename sysfs node that can be used
>>>>> to initiate a secure update.  The filename of a secure
>>>>> update file (BMC image, FPGA image, Root Entry Hash image,
>>>>> or Code Signing Key cancellation image) can be written to
>>>>> this sysfs entry to cause a secure update to occur.
>>>> Why is userspace responsible for triggering this?  Passing a "filename"
>>>> into the kernel and having it do something with it is ripe for major
>>>> problems, please do not.
>>>>
>>> I am using the "request_firmware" framework, which accepts a filename
>>> and finds the firmware file under /lib/firmware.
>>>
>>> Is this not an acceptable use for request_firmware?
>>>
>>> - Russ
>> Hi Greg,
>>
>> The dev_release fixes that you asked for in the FPGA Manager, Bridge, and
>> Region code are almost complete. I'm trying to get back to the FPGA
>> security manager patch set. Your previous comments challenged some basic
>> assumptions. If it is OK, I would like to get some clarity before I rework
>> the patches.
> Note, I do not have the time, nor the inclination, to help your company
> out with design reviews at this point in time.  If you have questions
> about this, please discuss it with the open source managers at Intel,
> they know the current situation quite well.
>
> I am glad to review patches that have gone through the proper internal
> Intel patch review process and then sent out to the community.

For what it is worth, these patches _were_ reviewed internally before
they were submitted to the public list. The first public submission
(Sep 2020) included review tags. I was asked to remove them and let
them be added back during the public review:

https://marc.info/?l=linux-fpga&m=159926670526828&w=2

Unfortunately, Intel review tags were not volunteered during the
public review, and it didn't occur to me to solicit the tags before
the patches were forwarded on to you by Moritz.

Most notably, Yilun and Hao contributed to the review both internally
and publicly. They are both listed in the MAINTAINERS file:

Xu Yilun <yilun.xu@intel.com>
Wu Hao <hao.wu@intel.com>

All issues/comments were resolved before the patches were sent to you.

> That being said, I will make one comment on your questions below:
>
>> (1) request_firmware(). We had assumed that making use of the existing
>> request_firmware() would be preferred. This requires providing a filename
>> under /lib/firmware to the framework. You commented (above): "Passing a
>> 'filename' into the kernel and having it do something with it is ripe for
>> problems, please do not." Unless you have additional comments on this, I
>> will plan to NOT use the request_firmware framework.
> request_firmware() should always be used for requesting firmware for a
> device.  Having an api where you write a random filename to a sysfs file
> and have that loaded by the kernel seems ripe for disaster though, I can
> not think of any other in-kernel user of the firmware api that does
> this.  Or are there examples that I have just missed?

I found an instance of a driver that allows the firmware filename
(under /lib/firmware) to be changed via sysfs:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-mic#n107
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-mic#n117

I get why it seems dangerous to provide a /lib/firmware filename via
sysfs and have the system automatically transfer that data to a device.
I'm just trying to figure out if there is a preferred/less-dangerous
way to do what we are trying to do.

Our objective is to allow the user to provide a new, signed FPGA image
that can be loaded on the fly (think cloud environment). The PCIe FPGA
card authenticates the image data with encryption keys; the host-side
software is not trusted by the device. So, other than checking the data
size, host software just passes the data through.

I think our usage is firmware-like, but it doesn't exactly fit the
current usage of request_firmware(). The firmware filename isn't
hardwired into the driver and the image isn't loaded at probe time.

If the request_firmware() implementation is not acceptable, then would
you agree that an IOCTL implementation is our best option?

- Russ

>
> thanks,
>
> greg k-h
Greg Kroah-Hartman Aug. 3, 2021, 5:49 a.m. UTC | #6
On Mon, Aug 02, 2021 at 11:31:47AM -0700, Russ Weight wrote:
> 
> 
> On 7/30/21 4:18 AM, Greg KH wrote:
> > On Thu, Jul 29, 2021 at 06:23:12PM -0700, Russ Weight wrote:
> >>
> >> On 5/17/21 12:37 PM, Russ Weight wrote:
> >>> On 5/16/21 10:32 PM, Greg KH wrote:
> >>>> On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
> >>>>> From: Russ Weight <russell.h.weight@intel.com>
> >>>>>
> >>>>> Extend the FPGA Security Manager class driver to
> >>>>> include an update/filename sysfs node that can be used
> >>>>> to initiate a secure update.  The filename of a secure
> >>>>> update file (BMC image, FPGA image, Root Entry Hash image,
> >>>>> or Code Signing Key cancellation image) can be written to
> >>>>> this sysfs entry to cause a secure update to occur.
> >>>> Why is userspace responsible for triggering this?  Passing a "filename"
> >>>> into the kernel and having it do something with it is ripe for major
> >>>> problems, please do not.
> >>>>
> >>> I am using the "request_firmware" framework, which accepts a filename
> >>> and finds the firmware file under /lib/firmware.
> >>>
> >>> Is this not an acceptable use for request_firmware?
> >>>
> >>> - Russ
> >> Hi Greg,
> >>
> >> The dev_release fixes that you asked for in the FPGA Manager, Bridge, and
> >> Region code are almost complete. I'm trying to get back to the FPGA
> >> security manager patch set. Your previous comments challenged some basic
> >> assumptions. If it is OK, I would like to get some clarity before I rework
> >> the patches.
> > Note, I do not have the time, nor the inclination, to help your company
> > out with design reviews at this point in time.  If you have questions
> > about this, please discuss it with the open source managers at Intel,
> > they know the current situation quite well.
> >
> > I am glad to review patches that have gone through the proper internal
> > Intel patch review process and then sent out to the community.
> 
> For what it is worth, these patches _were_ reviewed internally before
> they were submitted to the public list. The first public submission
> (Sep 2020) included review tags. I was asked to remove them and let
> them be added back during the public review:
> 
> https://marc.info/?l=linux-fpga&m=159926670526828&w=2
> 
> Unfortunately, Intel review tags were not volunteered during the
> public review, and it didn't occur to me to solicit the tags before
> the patches were forwarded on to you by Moritz.
> 
> Most notably, Yilun and Hao contributed to the review both internally
> and publicly. They are both listed in the MAINTAINERS file:
> 
> Xu Yilun <yilun.xu@intel.com>
> Wu Hao <hao.wu@intel.com>
> 
> All issues/comments were resolved before the patches were sent to you.

That's fine and wonderful, but not what I was talking about here at all.

Again, I am willing to review patches, but not take the time out to
comment on general design decisions like this for the reasons stated in
the past.

> > That being said, I will make one comment on your questions below:
> >
> >> (1) request_firmware(). We had assumed that making use of the existing
> >> request_firmware() would be preferred. This requires providing a filename
> >> under /lib/firmware to the framework. You commented (above): "Passing a
> >> 'filename' into the kernel and having it do something with it is ripe for
> >> problems, please do not." Unless you have additional comments on this, I
> >> will plan to NOT use the request_firmware framework.
> > request_firmware() should always be used for requesting firmware for a
> > device.  Having an api where you write a random filename to a sysfs file
> > and have that loaded by the kernel seems ripe for disaster though, I can
> > not think of any other in-kernel user of the firmware api that does
> > this.  Or are there examples that I have just missed?
> 
> I found an instance of a driver that allows the firmware filename
> (under /lib/firmware) to be changed via sysfs:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-mic#n107
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-mic#n117

Ah, another Intel driver wanting to do bad things, nice example :)

> I get why it seems dangerous to provide a /lib/firmware filename via
> sysfs and have the system automatically transfer that data to a device.
> I'm just trying to figure out if there is a preferred/less-dangerous
> way to do what we are trying to do.
> 
> Our objective is to allow the user to provide a new, signed FPGA image
> that can be loaded on the fly (think cloud environment). The PCIe FPGA
> card authenticates the image data with encryption keys; the host-side
> software is not trusted by the device. So, other than checking the data
> size, host software just passes the data through.
> 
> I think our usage is firmware-like, but it doesn't exactly fit the
> current usage of request_firmware(). The firmware filename isn't
> hardwired into the driver and the image isn't loaded at probe time.
> 
> 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.

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?

thanks,

greg k-h
Russ Weight Aug. 3, 2021, 7:02 p.m. UTC | #7
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.

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

- Russ
> thanks,
>
> greg k-h
Greg Kroah-Hartman Aug. 4, 2021, 7:37 a.m. UTC | #8
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?

naming is hard,

greg k-h
Moritz Fischer Aug. 4, 2021, 2:58 p.m. UTC | #9
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.

A similar case that comes to mind would be writing firmware to an
external flash on a Renesas xHCI controller, or updating the BMC
firmware for a plug-in card with BMC.

- Moritz
Greg Kroah-Hartman Aug. 4, 2021, 3:12 p.m. UTC | #10
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...

thanks,

greg k-h
Moritz Fischer Aug. 4, 2021, 7:47 p.m. UTC | #11
On Wed, Aug 04, 2021 at 05:12:41PM +0200, 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...

Even better if we can do that. It would have a limited overlap with
existing functionality, though.

- Moritz
Russ Weight Nov. 2, 2021, 4:25 p.m. UTC | #12
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
Greg Kroah-Hartman Nov. 2, 2021, 5:06 p.m. UTC | #13
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
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
index 2498aef0ac51..36d1b6ba8d76 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
@@ -3,3 +3,16 @@  Date:		June 2021
 KernelVersion:	5.14
 Contact:	Russ Weight <russell.h.weight@intel.com>
 Description:	Name of low level fpga security manager driver.
+
+What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/filename
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Write only. Write the filename of an image
+		file to this sysfs file to initiate a secure
+		update. The file must have an appropriate header
+		which, among other things, identifies the target
+		for the update. This mechanism is used to update
+		BMC images, BMC firmware, Static Region images,
+		and Root Entry Hashes, and to cancel Code Signing
+		Keys (CSK).
diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
index 468379e0c825..bfdb01d2de57 100644
--- a/drivers/fpga/fpga-sec-mgr.c
+++ b/drivers/fpga/fpga-sec-mgr.c
@@ -5,8 +5,11 @@ 
  * Copyright (C) 2019-2020 Intel Corporation, Inc.
  */
 
+#include <linux/delay.h>
+#include <linux/firmware.h>
 #include <linux/fpga/fpga-sec-mgr.h>
 #include <linux/idr.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
@@ -20,6 +23,132 @@  struct fpga_sec_mgr_devres {
 
 #define to_sec_mgr(d) container_of(d, struct fpga_sec_mgr, dev)
 
+static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
+			       enum fpga_sec_err err_code)
+{
+	smgr->err_code = err_code;
+	smgr->sops->cancel(smgr);
+}
+
+static void progress_complete(struct fpga_sec_mgr *smgr)
+{
+	mutex_lock(&smgr->lock);
+	smgr->progress = FPGA_SEC_PROG_IDLE;
+	complete_all(&smgr->update_done);
+	mutex_unlock(&smgr->lock);
+}
+
+static void fpga_sec_mgr_update(struct work_struct *work)
+{
+	struct fpga_sec_mgr *smgr;
+	const struct firmware *fw;
+	enum fpga_sec_err ret;
+	u32 offset = 0;
+
+	smgr = container_of(work, struct fpga_sec_mgr, work);
+
+	get_device(&smgr->dev);
+	if (request_firmware(&fw, smgr->filename, &smgr->dev)) {
+		smgr->err_code = FPGA_SEC_ERR_FILE_READ;
+		goto idle_exit;
+	}
+
+	smgr->data = fw->data;
+	smgr->remaining_size = fw->size;
+
+	if (!try_module_get(smgr->dev.parent->driver->owner)) {
+		smgr->err_code = FPGA_SEC_ERR_BUSY;
+		goto release_fw_exit;
+	}
+
+	smgr->progress = FPGA_SEC_PROG_PREPARING;
+	ret = smgr->sops->prepare(smgr);
+	if (ret != FPGA_SEC_ERR_NONE) {
+		fpga_sec_dev_error(smgr, ret);
+		goto modput_exit;
+	}
+
+	smgr->progress = FPGA_SEC_PROG_WRITING;
+	while (smgr->remaining_size) {
+		ret = smgr->sops->write_blk(smgr, offset);
+		if (ret != FPGA_SEC_ERR_NONE) {
+			fpga_sec_dev_error(smgr, ret);
+			goto done;
+		}
+
+		offset = fw->size - smgr->remaining_size;
+	}
+
+	smgr->progress = FPGA_SEC_PROG_PROGRAMMING;
+	ret = smgr->sops->poll_complete(smgr);
+	if (ret != FPGA_SEC_ERR_NONE)
+		fpga_sec_dev_error(smgr, ret);
+
+done:
+	if (smgr->sops->cleanup)
+		smgr->sops->cleanup(smgr);
+
+modput_exit:
+	module_put(smgr->dev.parent->driver->owner);
+
+release_fw_exit:
+	smgr->data = NULL;
+	release_firmware(fw);
+
+idle_exit:
+	/*
+	 * Note: smgr->remaining_size is left unmodified here to
+	 * provide additional information on errors. It will be
+	 * reinitialized when the next secure update begins.
+	 */
+	kfree(smgr->filename);
+	smgr->filename = NULL;
+	put_device(&smgr->dev);
+	progress_complete(smgr);
+}
+
+static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
+	int ret = count;
+
+	if (!count || count >= PATH_MAX)
+		return -EINVAL;
+
+	mutex_lock(&smgr->lock);
+	if (smgr->driver_unload || smgr->progress != FPGA_SEC_PROG_IDLE) {
+		ret = -EBUSY;
+		goto unlock_exit;
+	}
+
+	smgr->filename = kmemdup_nul(buf, count, GFP_KERNEL);
+	if (!smgr->filename) {
+		ret = -ENOMEM;
+		goto unlock_exit;
+	}
+
+	smgr->err_code = FPGA_SEC_ERR_NONE;
+	smgr->progress = FPGA_SEC_PROG_READING;
+	reinit_completion(&smgr->update_done);
+	schedule_work(&smgr->work);
+
+unlock_exit:
+	mutex_unlock(&smgr->lock);
+	return ret;
+}
+static DEVICE_ATTR_WO(filename);
+
+static struct attribute *sec_mgr_update_attrs[] = {
+	&dev_attr_filename.attr,
+	NULL,
+};
+
+static struct attribute_group sec_mgr_update_attr_group = {
+	.name = "update",
+	.attrs = sec_mgr_update_attrs,
+};
+
 static ssize_t name_show(struct device *dev,
 			 struct device_attribute *attr, char *buf)
 {
@@ -40,6 +169,7 @@  static struct attribute_group sec_mgr_attr_group = {
 
 static const struct attribute_group *fpga_sec_mgr_attr_groups[] = {
 	&sec_mgr_attr_group,
+	&sec_mgr_update_attr_group,
 	NULL,
 };
 
@@ -65,6 +195,12 @@  fpga_sec_mgr_create(struct device *dev, const char *name,
 	struct fpga_sec_mgr *smgr;
 	int id, ret;
 
+	if (!sops || !sops->cancel || !sops->prepare ||
+	    !sops->write_blk || !sops->poll_complete) {
+		dev_err(dev, "Attempt to register without all required ops\n");
+		return NULL;
+	}
+
 	if (!name || !strlen(name)) {
 		dev_err(dev, "Attempt to register with no name!\n");
 		return NULL;
@@ -83,6 +219,10 @@  fpga_sec_mgr_create(struct device *dev, const char *name,
 	smgr->name = name;
 	smgr->priv = priv;
 	smgr->sops = sops;
+	smgr->err_code = FPGA_SEC_ERR_NONE;
+	smgr->progress = FPGA_SEC_PROG_IDLE;
+	init_completion(&smgr->update_done);
+	INIT_WORK(&smgr->work, fpga_sec_mgr_update);
 
 	device_initialize(&smgr->dev);
 	smgr->dev.class = fpga_sec_mgr_class;
@@ -200,11 +340,31 @@  EXPORT_SYMBOL_GPL(fpga_sec_mgr_register);
  *
  * This function is intended for use in an FPGA security manager
  * driver's remove() function.
+ *
+ * For some devices, once the secure update has begun authentication
+ * the hardware cannot be signaled to stop, and the driver will not
+ * exit until the hardware signals completion.  This could be 30+
+ * minutes of waiting. The driver_unload flag enables a force-unload
+ * of the driver (e.g. modprobe -r) by signaling the parent driver to
+ * exit even if the hardware update is incomplete. The driver_unload
+ * flag also prevents new updates from starting once the unregister
+ * process has begun.
  */
 void fpga_sec_mgr_unregister(struct fpga_sec_mgr *smgr)
 {
 	dev_info(&smgr->dev, "%s %s\n", __func__, smgr->name);
 
+	mutex_lock(&smgr->lock);
+	smgr->driver_unload = true;
+	if (smgr->progress == FPGA_SEC_PROG_IDLE) {
+		mutex_unlock(&smgr->lock);
+		goto unregister;
+	}
+
+	mutex_unlock(&smgr->lock);
+	wait_for_completion(&smgr->update_done);
+
+unregister:
 	device_unregister(&smgr->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_sec_mgr_unregister);
diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
index f85665b79b9d..978ab98ffac5 100644
--- a/include/linux/fpga/fpga-sec-mgr.h
+++ b/include/linux/fpga/fpga-sec-mgr.h
@@ -7,16 +7,56 @@ 
 #ifndef _LINUX_FPGA_SEC_MGR_H
 #define _LINUX_FPGA_SEC_MGR_H
 
+#include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/types.h>
 
 struct fpga_sec_mgr;
 
+enum fpga_sec_err {
+	FPGA_SEC_ERR_NONE,
+	FPGA_SEC_ERR_HW_ERROR,
+	FPGA_SEC_ERR_TIMEOUT,
+	FPGA_SEC_ERR_CANCELED,
+	FPGA_SEC_ERR_BUSY,
+	FPGA_SEC_ERR_INVALID_SIZE,
+	FPGA_SEC_ERR_RW_ERROR,
+	FPGA_SEC_ERR_WEAROUT,
+	FPGA_SEC_ERR_FILE_READ,
+	FPGA_SEC_ERR_MAX
+};
+
 /**
  * struct fpga_sec_mgr_ops - device specific operations
+ * @prepare:		    Required: Prepare secure update
+ * @write_blk:		    Required: Write a block of data
+ * @poll_complete:	    Required: Check for the completion of the
+ *			    HW authentication/programming process. This
+ *			    function should check for smgr->driver_unload
+ *			    and abort with FPGA_SEC_ERR_CANCELED when true.
+ * @cancel:		    Required: Signal HW to cancel update
+ * @cleanup:		    Optional: Complements the prepare()
+ *			    function and is called at the completion
+ *			    of the update, whether success or failure,
+ *			    if the prepare function succeeded.
  */
 struct fpga_sec_mgr_ops {
+	enum fpga_sec_err (*prepare)(struct fpga_sec_mgr *smgr);
+	enum fpga_sec_err (*write_blk)(struct fpga_sec_mgr *smgr, u32 offset);
+	enum fpga_sec_err (*poll_complete)(struct fpga_sec_mgr *smgr);
+	enum fpga_sec_err (*cancel)(struct fpga_sec_mgr *smgr);
+	void (*cleanup)(struct fpga_sec_mgr *smgr);
+};
+
+/* Update progress codes */
+enum fpga_sec_prog {
+	FPGA_SEC_PROG_IDLE,
+	FPGA_SEC_PROG_READING,
+	FPGA_SEC_PROG_PREPARING,
+	FPGA_SEC_PROG_WRITING,
+	FPGA_SEC_PROG_PROGRAMMING,
+	FPGA_SEC_PROG_MAX
 };
 
 struct fpga_sec_mgr {
@@ -24,6 +64,14 @@  struct fpga_sec_mgr {
 	struct device dev;
 	const struct fpga_sec_mgr_ops *sops;
 	struct mutex lock;		/* protect data structure contents */
+	struct work_struct work;
+	struct completion update_done;
+	char *filename;
+	const u8 *data;			/* pointer to update data */
+	u32 remaining_size;		/* size remaining to transfer */
+	enum fpga_sec_prog progress;
+	enum fpga_sec_err err_code;	/* security manager error code */
+	bool driver_unload;
 	void *priv;
 };