/snapd

interfaces: add kernel-module interface for module insertion. #1602

Merged
merged 5 commits into from Aug 15, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Reviewed
Projects
None yet
Milestone
No milestone
5 participants
@arges @morphis @jdstrand @mvo5 @mabnhdev
@arges
Contributor

arges commented Jul 29, 2016

This allows for module insertion from another snap given this slot.
In addition add a unit test for this module.

@pedronis pedronis changed the title from interface: add kernel-module interface for module insertion. to interfaces: add kernel-module interface for module insertion. Jul 31, 2016

@morphis
Contributor

morphis commented Aug 1, 2016 •
Edited 1 time

We discussed this already in the past but came to the conclusion that we want to encode loading a particular kernel module always with a specific interface and not to have an interface which allows loading generic kernel modules.

@zyga proposed an addition to the interface system which will allow you to encode modules to load into an interface.

If I missed an further discussion on this topic this is missing

  • unit tests for the interface
  • documentation of a manual integration test
@arges
Contributor

arges commented Aug 1, 2016

Hi, is there a link to this discussion somewhere?
And if we need to encode module names beforehand, why not a pattern of names?
Where do documentation of manual integration tests go?
@jdstrand
Contributor

jdstrand commented Aug 1, 2016

@morphis - that discussion was different and was in support of loading a module for, say, the ppp interface or the firewall-control interface. This interface is supposed to be for loading arbitrary modules AIUI. My comments will be with that in mind.
snap/implicit.go
@@ -56,6 +56,7 @@ var implicitClassicSlots = []string{
"modem-manager",
"optical-drive",
"camera",
+ "kernel-module",
@jdstrand

jdstrand Aug 1, 2016

Contributor

This should be in implicitSlots since it should be available to more than just classic systems, AIUI.

snap/implicit_test.go
@@ -56,7 +56,7 @@ func (s *InfoSnapYamlTestSuite) TestAddImplicitSlotsOnClassic(c *C) {
c.Assert(info.Slots["unity7"].Interface, Equals, "unity7")
c.Assert(info.Slots["unity7"].Name, Equals, "unity7")
c.Assert(info.Slots["unity7"].Snap, Equals, info)
- c.Assert(info.Slots, HasLen, 25)
+ c.Assert(info.Slots, HasLen, 26)
@jdstrand

jdstrand Aug 1, 2016

Contributor

Since moving to implicitSlots, above, should up TestAddImplicitSlotsOutsideClassic length to 16.

interfaces/builtin/kernel_module.go
@@ -0,0 +1,48 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-
@jdstrand

jdstrand Aug 1, 2016

Contributor

Please add tests in kernel_module_test.go

interfaces/builtin/kernel_module.go
+ connectedPlugAppArmor: kernelModuleConnectedPlugAppArmor,
+ connectedPlugSecComp: kernelModuleConnectedPlugSecComp,
+ reservedForOS: true,
+ autoConnect: true,
@jdstrand

jdstrand Aug 1, 2016

Contributor

No! :)

This interface is highly privileged and should not be auto-connected by default. Please use 'autoConnect: false'. Your to-be-written tests should verify that autoConnect is false.

@arges

arges Aug 2, 2016

Contributor

If an application snap is allowed to use the 'kernel-module' interface, is there a way to connect this interface from within the application? Or will that require an additional manual step?

I'm hoping one could just install the application snap and it be able to insert modules as appropriate.

@morphis

morphis Aug 2, 2016

Contributor

No, you have to connect the plug and slot manually. Once hooks are available you can implement a hook in your snap so that you know when the plug got connected and then load the module.

interfaces/builtin/kernel_module.go
+# Description: Allow insertion of modules.
+
+finit_module
+delete_module
@jdstrand

jdstrand Aug 1, 2016

Contributor

I think you also need 'init_module', no?

@jdstrand

jdstrand Aug 1, 2016

Contributor

I suspect you also want to add 'query_module' for completeness.

@arges

arges Aug 2, 2016

Contributor

I kept a very minimal set of syscalls initially. I believe query_module is deprecated post 2.6, so we may need to expose something like /proc/modules instead. init_module is fine, but finit_module will work just as well. Overall we need to load, remove and query modules.

@jdstrand

jdstrand Aug 2, 2016

Contributor

I see in the man page that query_module is deprecated, yes, so fine to leave that out. Thanks for adding init_module and @{PROC}/modules.

@jdstrand
Contributor

jdstrand commented Aug 1, 2016

Isn't it amazing how just a few lines of security policy can be so dangerous? Seriously though, the security policy is fine for loading modules, but I want to stress how privileged this interface is-- if you can insert kernel modules, you completely own the system and security policy can't protect the system (since the module could disable policy). This should only ever be allowed to trusted snaps with responsive upstreams where security vulnerabilities in the code that 'plugs: [ kernel-module ]' are patched quickly.

Also, 'kernel-module' doesn't really fit our current naming convention. It isn't bad per se, but I wonder if something like 'kernel-module-control' would be better? Don't change anything yet on that-- let's wait for @niemeyer to respond on that.
@morphis
Contributor

morphis commented Aug 2, 2016

@jdstrand I wasn't aware that we want to allow such a thing, but ok.
@arges
Contributor

arges commented Aug 2, 2016

For completeness here is an example of a snap that builds and loads a module:
https://github.com/arges/hello-kernel

I'll await comments about naming and push a v2 with changes and tests.
@arges
Contributor

arges commented Aug 2, 2016

Well v2 is already pushed, I forgot that github automatically does the pull request given my personal branch. Oh well, feel free to comment on this rev. : )
@jdstrand
Contributor

jdstrand commented Aug 2, 2016

@arges - today this would need an additional 'snap connect ...' manual step since autoConnect is false. AIUI, in the future, assertions can be used to inform snapd to auto-connect the interface for a particular snap from a particular publisher. I don't know the status of that work - @niemeyer and @zyga can you comment?

I suggest for the moment leaving this as manually connected and at such time that a snap requires it to be auto-connected, we review the status of the assertions work and if it isn't ready, we can move the check to auto-connect and do a quick and dirty check in the review tools to flag for manual review if using the interface but auto-approve certain snaps.
@arges
Contributor

arges commented Aug 3, 2016

Ok I'll post a v3 with my other merges into this interface.
@arges
Contributor

arges commented Aug 4, 2016

One thing to note: when lscpu is run as a normal user the current code works fine. But when lscpu is run as root then it also requires the following:
/sys/firmware/efi/systab
/dev/mem

Should I add these into the profile as well? Is there a way to specify that these are only accessible to root users?
@jdstrand
Contributor

jdstrand commented Aug 4, 2016

Adding /sys/firmware/efi/systab is fine to add to hardware-observe. Not terribly comfortable with giving out /dev/mem in system-observe. Perhaps add read access to /dev/mem to kernel-module-control with a comment: "Note: this is only needed by lscpu. Perhaps this should be moved to system-trace or system-observe, but let's leave here for now"
@arges
Contributor

arges commented Aug 5, 2016 •
Edited 1 time

Even with this commit I still get:
failed to read /proc/version_signature: open /proc/version_signature: no such file or directory

After snapping up my test daemon program.

It now longer denies it, but I can't read the file. Anything else I need to do to enable access to proc?

UPDATE: PEBKEC. Had to restart my daemon program and now it works.
@arges
Contributor

arges commented Aug 8, 2016

The failing integration test doesn't seem to be my code:

createUserSuite.TestCreateUserCreatesUser (from )
Stacktrace
_StringException

This is the extent of info I've been able to extract from Jenkins.
interfaces/apparmor/template.go
@@ -201,8 +201,6 @@ var defaultTemplate = []byte(`
@{PROC}/@{pid}/ r,
@{PROC}/@{pid}/fd/ r,
owner @{PROC}/@{pid}/auxv r,
- @{PROC}/@{pid}/version_signature r,
- @{PROC}/@{pid}/version r,
@jdstrand

jdstrand Aug 8, 2016

Contributor

These should be changed to:

@{PROC}/version_signature r,
@{PROC}/version r,

instead of removed altogether.

@mvo5

mvo5 Aug 12, 2016

Collaborator

This is addressed now (in the lines below).

interfaces/builtin/system_observe.go
@@ -51,6 +51,7 @@ deny ptrace (trace),
@{PROC}/vmstat r,
@{PROC}/diskstats r,
@{PROC}/kallsyms r,
+@{PROC}/version_signature r,
@jdstrand

jdstrand Aug 8, 2016

Contributor

Can you remove this and put in template.go as described in my previous comment a moment ago? This is a common access and we don't want applications to require the privileged system-observe for it.

interfaces/builtin/kernel_module_control.go
+ "github.com/snapcore/snapd/interfaces"
+)
+
+var kernelModuleControlConnectedPlugAppArmor = `
@jdstrand

jdstrand Aug 8, 2016

Contributor

Can you use const here instead of var?

interfaces/builtin/kernel_module_control.go
+ /dev/mem r,
+`
+
+var kernelModuleControlConnectedPlugSecComp = `
@jdstrand

jdstrand Aug 8, 2016

Contributor

Can you use const here instead of var?

interfaces/builtin/all.go
@@ -60,6 +60,7 @@ var allInterfaces = []interfaces.Interface{
NewOpticalDriveInterface(),
NewCameraInterface(),
NewBluetoothControlInterface(),
+ NewKernelModuleControlInterface(),
}
// Interfaces returns all of the built-in interfaces.
@jdstrand

jdstrand Aug 8, 2016

Contributor

Can you also update all_test.go?

@mvo5

mvo5 Aug 12, 2016

Collaborator

This is done now as well.

@jdstrand
Contributor

jdstrand commented Aug 8, 2016

This is lacking an update to docs/interfaces.md. Please address the small changes I suggested above, then policy LGTM.
docs/interfaces.md
+
+Allow to insert kernel modules.
+
+* Auto-Connect: no
@jdstrand

jdstrand Aug 10, 2016

Contributor

Can you insert this alphabetically in the 'Advanced' section? Also, please rephrase to be more like the others: 'Can insert kernel modules. This interface gives privileged access to the device.'

interfaces/apparmor/template.go
@@ -220,6 +218,7 @@ var defaultTemplate = []byte(`
/etc/machine-id r,
/etc/mime.types r,
@{PROC}/ r,
+ @{PROC}/version_signature r,
@jdstrand

jdstrand Aug 10, 2016

Contributor

Please also add back @{PROC}/version r,

@jdstrand

jdstrand Aug 10, 2016

Contributor

It's weird that @{PROC}/version and @{PROC}/version_signature aren't next to each other like they were before but I won't block on that. Feel free to update it without needing me to look at this again.

@jdstrand
Contributor

jdstrand commented Aug 10, 2016

LGTM from a security policy POV (though it would be nice to put @{PROC}/version next to @{PROC}/version_signature-- feel free to do that without needing another review from me).

+1 once tests are passing or a snappy core member says its ok.
@mvo5
Collaborator

mvo5 commented Aug 12, 2016

Looks fine but needs de-conflicting.
interfaces/builtin/hardware_observe.go
@@ -29,12 +29,19 @@ const hardwareObserveConnectedPlugAppArmor = `
# from the system. this is reserved because it allows reading potentially sensitive information.
# Usage: reserved
+# used by lscpu
@mvo5

mvo5 Aug 12, 2016

Collaborator

Changes in this file look a bit unreleated, but they do look fine and its not enough to justify a separate branch I think.

@arges

arges Aug 12, 2016

Contributor

Originally I had separated this into another PR, but was asked to merge it into a single one:
#1612
#1618

@mvo5
Collaborator

mvo5 commented Aug 12, 2016

👍 Feels free to merge once master is merged and tests are green.

@mvo5 mvo5 added the Reviewed label Aug 12, 2016

arges added some commits Aug 3, 2016

@arges
interfaces: Add lscpu to hardware-observe. 
Allow usage of lscpu. In addition allow read of /proc/bus/pci/devices as it is
used by lscpu. When lscpu is run as root it will also try to read from
/sys/firmware/efi/systab. Add permissions here as well.
78505e0
@arges
interfaces: Add version_signature to apparmor template. 
Allow access to /proc/version_signature to be able to easily read kernel version
and build number.

In addition remove /proc/pid/version_signature and /proc/pid/version from the
apparmor template as those paths don't exist. Add back /proc/version because
this does exist.

Some checks were not successful

2 failing and 1 successful checks
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
@snappy-m-o
Integration tests — Success
Details
30ceaec
@arges
interface: add kernel-module-control interface. 
This allows for module insertion, removal and querying from another snap given
this slot. In addition add a unit test for this new interface.

Update existing unit tests and add documentation about this interface.

Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
6a9fcbe
@arges
Merge branch 'master' into kernel-module

Some checks were not successful

1 failing and 2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
@snappy-m-o
Integration tests — Success
Details
@snappy-m-o
autopkgtest — Success
Details
ace01c1
@arges
Merge branch 'master' into kernel-module

All checks have passed

3 successful checks
@snappy-m-o
Integration tests — Success
Details
@snappy-m-o
autopkgtest — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
3f91f8d

@mvo5 mvo5 merged commit a22b514 into snapcore:master Aug 15, 2016
3 checks passed

3 checks passed

Integration tests Success
Details
autopkgtest Success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mabnhdev

mabnhdev commented Aug 24, 2016

Hello. This will be very useful to me, but I worry that it doesn't go quite far enough. I'm trying to snap up OpenSwitch (www.openswitch.net) which provides several kernel drivers to control the network ASIC chip on a target platform. All the code the deals with the ASIC is platform-specific, so there will be a snap for each target platform.

Currently, I'm building a custom kernel snap for each platform that contains the necessary drivers.

I'm desperately hoping to get out of the business of distributing custom kernel snaps and trying to coordinate compatible versions of my OpenSwitch snap and the custom kernel snap.

My problem is after the OpenSwitch snap is installed on a system, lets say the kernel snap is updated in the normal course of snap refreshing. Now, say that the OpenSwitch drivers are not compatible with the new kernel. The drivers will not load and OpenSwitch is broken.

I don't want to tie OpenSwitch versions to the kernel version because I'll lose all the pleasures of transactional update/rollback of both OpenSwitch and the kernel.

Ideally, I think Snappy needs something like DKMS to keep a snap's kernel modules in sync with the currently running kernel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment