Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

interfaces: OpenGL/Vulkan drivers provided by a base should be usable. #7073

Closed

Conversation

@jhenstridge
Copy link
Contributor

commented Jul 8, 2019

If a base snap ships OpenGL or Vulkan drivers, the opengl interface should grant access to them. While the actual shared libraries are already accessible by the base AppArmor profile (they are files matching lib*.so under the /usr/lib hierarchy), we also need to grant access to the configuration files the dispatch library.

The added paths are not provided by the core or core18 snaps, so should not change the behaviour of existing snaps.

interfaces: OpenGL/Vulkan drivers provided by a base should be usable.
If a base snap ships OpenGL or Vulkan drivers, the `opengl` interface
should grant access to them.  While the actual shared libraries are
already accessible by the base AppArmor profile (they are files matching
`lib*.so` under the `/usr/lib` hierarchy), we also need to grant access
to the configuration files the dispatch library.
# Support reading the Vulkan ICD and layer files
/usr/share/vulkan/icd.d/{,*.json} r,
/usr/share/vulkan/explicit_layer.d/{,*.json} r,
/usr/share/vulkan/implicit_layer.d/{,*.json} r,
/var/lib/snapd/lib/vulkan/ r,
/var/lib/snapd/lib/vulkan/** r,
/var/lib/snapd/hostfs/usr/share/vulkan/icd.d/*nvidia*.json r,

This comment has been minimized.

Copy link
@bboozzoo

bboozzoo Jul 8, 2019

Contributor

Wonder whether for symmetry we should allow *.json from hostfs. @zyga given your experiments, does that make sense?

This comment has been minimized.

Copy link
@jhenstridge

jhenstridge Jul 8, 2019

Author Contributor

The reason the hostfs read access is limited to *nvidia*.json is because that is (currently) the only hostfs driver linked to /var/lib/snapd/lib/vulkan. Unless snap-confine was enhanced to expose more host drivers to the sandbox, it doesn't really help much for the sandboxed app to know about other host drivers. And in the specific case of the Mesa/DRI drivers, doing so is particularly difficult due to C++ ABI differences (at least if Mesa is compiled against a newer libstdc++ than exists in the base snap).

So while the two cases are similar, I don't think I'd describe them as symmetric.

This comment has been minimized.

Copy link
@zyga

zyga Jul 8, 2019

Contributor

I agree with @jhenstridge

This comment has been minimized.

Copy link
@bboozzoo

bboozzoo Jul 8, 2019

Contributor

What I meant is that s-c already cherry pick nvidia files only, so the AA rules for egl_vendor.d and Vulkan's icd.d may actually be generic and look like the ones for Mesa

This comment has been minimized.

Copy link
@zyga

zyga Jul 10, 2019

Contributor

@bboozzoo I think we should refrain from reading configuration files from hostfs. The direction we have set is clear: sharing /etc was a mistake we will eventually undo. We need a layer of control and that only works if snapd can re-interpret applicable host configuration and inject it into the snap world. Therefore I would be -1 on sharing additional config files from hostfs this way.

This comment has been minimized.

Copy link
@jdstrand

jdstrand Jul 10, 2019

Contributor

The direction we have set is clear: sharing /etc was a mistake we will eventually undo.

I think this is overstated and not representative of everyone's opinion or considerate of the historical context. There were good reasons to use /etc from the host when the decision was made. We of course may want to revisit this or what to selectively expose, but that discussion can happen elsewhere.

@zyga
zyga approved these changes Jul 8, 2019
@jhenstridge

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

I've pushed up one more related change: give access to Mesa DRI drivers distributed with the base snap. These are not covered by the lib*.so* match glob in the base AppArmor template because they don't have the lib prefix.

@jdstrand
Copy link
Contributor

left a comment

From a security POV, all these accesses are fine and +1. I would like to see the comment suggestions I made as a reminder that a base snap may be in play for any rules where /usr is used. I'm abstaining from approving and will defer to @zyga on the suitability of this wrt future opengl work.

@jdstrand

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

From a security POV, all these accesses are fine and +1. I would like to see the comment suggestions I made as a reminder that a base snap may be in play for any rules where /usr is used. I'm abstaining from approving and will defer to @zyga on the suitability of this wrt future opengl work.

I've withdrawn my inline comments regarding referencing base snaps. I forgot for a moment that /usr/share/vulkan and /usr/share/glvnd are coming from the host, not a base snap.

@jdstrand
Copy link
Contributor

left a comment

I know I said that I would defer to @zyga, but in thinking through the freedesktop base snap some more (see https://forum.snapcraft.io/t/base-runtime-freedesktop-sdk-runtime-19-08/11153/28), I have concerns on the direction of this PR wrt the accesses to # Support loading Mesa DRI drivers from the base snap. Please have @pedronis sign off on the direction before merging.

Again, I'm not so concerned about the security impact of these particular accesses, but I'm concerned about how non-core base snaps intersect with our default and interface policy.

@pedronis pedronis added the ⛔ Blocked label Jul 9, 2019

@pedronis

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@jhenstridge what base concretely will ship soon those drivers in the base (vs host) ?

@jhenstridge

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

We probably don't want to merge this. The intent behind these changes is basically the same as the requests from the Freedesktop SDK folks running into problems with the default AppArmor template, and the fix would be the same too:

https://forum.snapcraft.io/t/base-runtime-freedesktop-sdk-runtime-19-08/11153/40?u=jamesh

If snapd switches to a less restrictive template for apps using non-bootable bases (i.e. anything other than core and core18), then there would be no need for interfaces like opengl to punch additional holes for access to drivers bundled with the base.

@jhenstridge jhenstridge closed this Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.