Message ID | 20210517023200.52707-3-mdf@kernel.org (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | FPGA Security Manager for 5.14 | expand |
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.
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
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
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
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
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
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
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
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
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
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
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
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 --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; };