All of
 help / color / mirror / Atom feed
From: Russ Weight <>
To: Greg KH <>
Cc: Tom Rix <>, Moritz Fischer <>,
	<>, <>
Subject: Re: [PATCH 02/12] fpga: sec-mgr: enable secure updates
Date: Mon, 2 Aug 2021 11:31:47 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <YQPgFn/>

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

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 <>
Wu Hao <>

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:

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

  reply	other threads:[~2021-08-02 18:32 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-17  2:31 [PATCH 00/12] FPGA Security Manager for 5.14 Moritz Fischer
2021-05-17  2:31 ` [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver Moritz Fischer
2021-05-17  5:18   ` Greg KH
2021-05-17 17:45     ` Russ Weight
2021-05-17 17:55       ` Greg KH
2021-05-17 18:25         ` Russ Weight
2021-05-19 20:42           ` Tom Rix
2021-05-21  1:10             ` Russ Weight
2021-05-21  4:58               ` Greg KH
2021-05-21 15:15                 ` Russ Weight
2021-05-17  2:31 ` [PATCH 02/12] fpga: sec-mgr: enable secure updates Moritz Fischer
2021-05-17  5:32   ` Greg KH
2021-05-17 19:37     ` Russ Weight
2021-07-30  1:23       ` Russ Weight
2021-07-30 11:18         ` Greg KH
2021-08-02 18:31           ` Russ Weight [this message]
2021-08-03  5:49             ` Greg KH
2021-08-03 19:02               ` Russ Weight
2021-08-04  7:37                 ` Greg KH
2021-08-04 14:58                   ` Moritz Fischer
2021-08-04 15:12                     ` Greg KH
2021-08-04 19:47                       ` Moritz Fischer
2021-11-02 16:25                       ` Russ Weight
2021-11-02 17:06                         ` Greg KH
2021-05-17  2:31 ` [PATCH 03/12] fpga: sec-mgr: expose sec-mgr update status Moritz Fischer
2021-05-17  2:31 ` [PATCH 04/12] fpga: sec-mgr: expose sec-mgr update errors Moritz Fischer
2021-05-17  2:31 ` [PATCH 05/12] fpga: sec-mgr: expose sec-mgr update size Moritz Fischer
2021-05-17  2:31 ` [PATCH 06/12] fpga: sec-mgr: enable cancel of secure update Moritz Fischer
2021-05-17  2:31 ` [PATCH 07/12] fpga: sec-mgr: expose hardware error info Moritz Fischer
2021-05-17  7:10   ` Greg KH
2021-05-17 19:49     ` Russ Weight
2021-05-17  2:31 ` [PATCH 08/12] fpga: m10bmc-sec: create max10 bmc secure update driver Moritz Fischer
2021-05-17  5:30   ` Greg KH
2021-05-17 20:09     ` Russ Weight
2021-05-17  2:31 ` [PATCH 09/12] fpga: m10bmc-sec: expose max10 flash update count Moritz Fischer
2021-05-17  2:31 ` [PATCH 10/12] fpga: m10bmc-sec: expose max10 canceled keys in sysfs Moritz Fischer
2021-05-17  2:31 ` [PATCH 11/12] fpga: m10bmc-sec: add max10 secure update functions Moritz Fischer
2021-05-17  2:32 ` [PATCH 12/12] fpga: m10bmc-sec: add max10 get_hw_errinfo callback func Moritz Fischer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.