interfaces/fpga: add fpga interface #9263

alfonsosanchezbeato · 2020-09-02T14:52:23Z

The fpga interface will allow programming fpgas for different
vendors. It lets plug users access the standard fpga kernel sysfs
interface [1-3] and also vendor specific interfaces.

[1] https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-fpga-manager
[2] https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-fpga-region
[3] https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-fpga-bridge

interfaces/builtin/fpga.go

alexmurray · 2020-11-12T03:49:33Z

interfaces/builtin/fpga.go

+# Devices
+/dev/fpga[0-9]* rw,
+
+# /sys/class/fpga_* specified by the kernel docs


Can you update this comment to include links to the associated kernel docs?

alexmurray · 2020-11-12T03:50:12Z

interfaces/builtin/fpga.go

+/sys/class/fpga_region/region[0-9]*/compat_id r,
+/sys/class/fpga_bridge/bridge[0-9]*/{name,state} r,
+
+# Xilinx zynqmp FPGA


Can you please also update this to add links to upstream documentation regarding these paths?

alexmurray · 2020-11-12T03:56:29Z

interfaces/builtin/fpga.go

+/sys/module/zynqmp_fpga/parameters/readback_type rw,
+
+# configfs interface
+/sys/kernel/config/device-tree/overlays/full{,1}/ rw,


My understanding of device-tree overlays is that a user can specify any path under the /sys/kernel/config/device-tree/overlays directory to load their overlay at - but the above only allows /sys/kernel/config/device-tree/overlays/full or /sys/kernel/config/device-tree/overlays/full1 to be used - can you elaborate?

The driver only uses these paths to load the overlay. "full" refers to fully program the fpga, apparently. For an example on how this is used: https://github.com/Xilinx/meta-xilinx-tools/blob/master/recipes-bsp/fpga-manager-script/files/fpgautil.c

Yes but my point is that there is no requirement to use the name/path "full" here - a user could just as easily use "foo" to load a device-tree overlay - it is just that the existing xilinx tool uses the paths "full" and "full1" - so I think this rule should instead be

# device-tree overlay configfs interface # https://kernel.ubuntu.com/git/ubuntu/ubuntu-groovy.git/tree/Documentation/devicetree/configfs-overlays.txt?h=raspi&id=ca36a8933cceb1c42ba2be7796e7a151e62a97cc /sys/kernel/config/device-tree/overlays/* rw, /sys/kernel/config/device-tree/overlays/*/dtbo rw, /sys/kernel/config/device-tree/overlays/*/path rw, /sys/kernel/config/device-tree/overlays/*/status r,

alexmurray · 2020-11-12T03:58:14Z

interfaces/builtin/fpga.go

+/sys/devices/platform/firmware:zynqmp-firmware/firmware:zynqmp-firmware:pcap/fpga_manager/fpga[0-9]*/{flags,key} rw,
+/sys/devices/platform/fpga-full/fpga_region/region[0-9]*/compat_id r,
+
+# Xilinx zynqmp module parameters


Again can you please add a link in this comment to some upstream documentation regarding this module + parameter?

interfaces/builtin/fpga.go

anonymouse64

I think the base declaration bits need to be changed more

anonymouse64 · 2020-11-13T02:56:20Z

interfaces/builtin/fpga.go

+`
+
+var fpgaConnectedPlugUDev = []string{
+	`SUBSYSTEM=="misc", KERNEL=="fpga[0-9]*"`,


this looks fine to me

interfaces/builtin/fpga.go

pedronis

It seems given the device tree loading this should be superprivileged, I suppose that's what @anonymouse64 considerations were getting to.

interfaces/builtin/fpga.go

The fpga interface will allow programming fpgas for different vendors. It lets plug users access the standard fpga kernel sysfs interface [1-3] and also vendor specific interfaces. [1] https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-fpga-manager [2] https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-fpga-region [3] https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-fpga-bridge

The connected plug will make sure that snapd creates a cgroup for the snap for the /dev/fpga[0-9]* devices.

alfonsosanchezbeato · 2020-11-16T14:20:16Z

@pedronis @anonymouse64 @alexmurray thanks for your comments - I have addressed them and re-based the branch, which is ready again for review.

anonymouse64

LGTM, thanks for the base-declaration changes

interfaces/builtin/fpga.go

jdstrand · 2020-11-17T14:16:28Z

interfaces/builtin/fpga.go

+
+# configfs interface
+/sys/kernel/config/device-tree/overlays/full{,1}/ rw,
+/sys/kernel/config/device-tree/overlays/full{,1}/path rw,


I don't care for the device-tree access in this PR, especially after reading @alexmurray's assessment. There were arguments for adding something like this to the TPM interface and also, IIRC, for creating a generic device-tree interface and we said 'no'. While putting these accesses in a superprivileged interface (like the base decl is now) does protect against widespread abuse, looking at the accesses, there is nothing in there that says 'fpga' - they are just mostly generic accesses that allow changing anything so IMO, they don't belong in this interface.

I suggest we remove the device-tree overlays accesses and then make this interface a normal manually-connected interface instead of superprivileged. For the device-tree bits, I suggest one of:

leaving the device-tree stuff out of the interfaces and let device makers use gadget.yaml to program the fpga for the device and/or use a management snap (eg, in their brand store) that plugs system-files to program the device

we create a 'device-tree' superprivileged interface. I worry about this since AIUI changes to the device-tree could impact the operational status of the device in a way that cannot be undone by Ubuntu Core

we come up with some 'snap set system' options for fpga that gadgets and management snaps with snapd-control can use to have snapd make changes on the snap's behalf

'1' is my preferred short term solution. '2' I don't care for, but is possible. '3' is interesting on a theoretical level since allows a command to validate what will be overlayed, though reading a bit about FPGA, this seems impractical.

We need @pedronis to weigh in on the approach. IME, going with '1' is not controversial and would at least unblock device makers immediately.

@jdstrand @alfonsosanchezbeato 1. seems fine to me if that's the security team recommendation. We should explore 3. at some point, it has appeared, been mentioned already more than a couple of times in different context, but I don't think any of the relevant teams have time ATM for it.

@pedronis @jdstrand thanks for the review, I have removed now the permissions to load device tree overlays and converted this to a manually connected interface. I completely agree on that this generic access to overlays belongs elsewhere. Option 1 seems feasible to me.

the issue with the base-declaration had been addressed

Instead, remove device tree overlays bits. This reverts commit d4db775.

… interface As per review. Access to device tree overlays would be done by system-files assertion.

jdstrand

Thank you for updating the AppArmor accesses. With that and the base declaration change you made, I'm approving. Please have @alexmurray give his final review before removing the 'Needs Security Review' tag (I only looked at this at a high-level).

interfaces/builtin/fpga.go

alexmurray

Security review ack from me.

jdstrand · 2020-11-19T23:11:54Z

Security review ack from me.

Thanks! Removed the 'Needs Security Review' tag.

alfonsosanchezbeato · 2020-11-20T14:47:15Z

\ø/ Thanks to all reviewers!

anonymouse64 reviewed Sep 2, 2020

View reviewed changes

interfaces/builtin/fpga.go Show resolved Hide resolved

mvo5 added the Needs security review Can only be merged once security gave a :+1: label Sep 3, 2020

alexmurray requested changes Nov 12, 2020

View reviewed changes

anonymouse64 self-requested a review November 12, 2020 04:17

anonymouse64 reviewed Nov 13, 2020

View reviewed changes

anonymouse64 requested a review from pedronis November 13, 2020 03:05

anonymouse64 added the Needs Samuele review Needs a review from Samuele before it can land label Nov 13, 2020

pedronis previously requested changes Nov 16, 2020

View reviewed changes

interfaces/builtin/fpga.go Show resolved Hide resolved

alfonsosanchezbeato added 8 commits November 16, 2020 15:16

tests: add high-level test for fpga interface

389351f

interfaces/fpga: deny auto-connect

8a87802

interfaces/fpga: add udev connected plug

de60543

The connected plug will make sure that snapd creates a cgroup for the snap for the /dev/fpga[0-9]* devices.

interfaces/fpga: add test for udev connected plug

9c0669a

Address review comments

305e2c3

Fix base declaration test

96427c3

Make interface supre-privileged

d4db775

alfonsosanchezbeato force-pushed the add-fpga-interface branch from a501884 to d4db775 Compare November 16, 2020 14:17

anonymouse64 approved these changes Nov 16, 2020

View reviewed changes

jdstrand reviewed Nov 17, 2020

View reviewed changes

interfaces/builtin/fpga.go Show resolved Hide resolved

jdstrand suggested changes Nov 17, 2020

View reviewed changes

pedronis requested review from pedronis and removed request for pedronis November 17, 2020 16:12

alfonsosanchezbeato added 2 commits November 19, 2020 14:04

Revert "Make interface supre-privileged"

b1e3a46

Instead, remove device tree overlays bits. This reverts commit d4db775.

Remove access to device-tree an make this a normal manually-connected…

09ec01c

… interface As per review. Access to device tree overlays would be done by system-files assertion.

pedronis self-requested a review November 19, 2020 13:20

anonymouse64 added the Squash-merge Please squash this PR when merging. label Nov 19, 2020

jdstrand approved these changes Nov 19, 2020

View reviewed changes

interfaces/builtin/fpga.go Show resolved Hide resolved

alexmurray approved these changes Nov 19, 2020

View reviewed changes

jdstrand removed the Needs security review Can only be merged once security gave a :+1: label Nov 19, 2020

pedronis merged commit 76b2bcf into snapcore:master Nov 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interfaces/fpga: add fpga interface #9263

interfaces/fpga: add fpga interface #9263

alfonsosanchezbeato commented Sep 2, 2020

alexmurray Nov 12, 2020

alfonsosanchezbeato Nov 12, 2020

alexmurray Nov 12, 2020

alfonsosanchezbeato Nov 12, 2020

alexmurray Nov 12, 2020

alfonsosanchezbeato Nov 12, 2020

alexmurray Nov 17, 2020

alexmurray Nov 12, 2020

alfonsosanchezbeato Nov 12, 2020

anonymouse64 left a comment

anonymouse64 Nov 13, 2020

pedronis left a comment

alfonsosanchezbeato commented Nov 16, 2020

anonymouse64 left a comment

jdstrand Nov 17, 2020 •

edited

pedronis Nov 18, 2020

alfonsosanchezbeato Nov 19, 2020

jdstrand left a comment

alexmurray left a comment

jdstrand commented Nov 19, 2020

alfonsosanchezbeato commented Nov 20, 2020

interfaces/fpga: add fpga interface #9263

interfaces/fpga: add fpga interface #9263

Conversation

alfonsosanchezbeato commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonymouse64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Nov 16, 2020

anonymouse64 left a comment

Choose a reason for hiding this comment

jdstrand Nov 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand left a comment

Choose a reason for hiding this comment

alexmurray left a comment

Choose a reason for hiding this comment

jdstrand commented Nov 19, 2020

alfonsosanchezbeato commented Nov 20, 2020

jdstrand Nov 17, 2020 •

edited