cmd/snap-confine: bring /lib/firmware from the host #7104

zyga · 2019-07-12T15:46:41Z

The kernel will use the mount namespace of the invoking process to
auto-load firmware on demand. On core18 systems or on systems using core
and core18 base snap apps the /lib/firmware directory is empty present,
causing issues with CUDA and some wifi stacks. This patch fixes this

Fixes: https://bugs.launchpad.net/snapd/+bug/1821023
Signed-off-by: Zygmunt Krynicki me@zygoon.pl

The kernel will use the mount namespace of the invoking process to auto-load firmware on demand. On core18 systems or on systems using core and core18 base snap apps the /lib/firmware directory is empty present, causing issues with CUDA and some wifi stacks. This patch fixes this Fixes: https://bugs.launchpad.net/snapd/+bug/1821023 Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

jdstrand

Assuming tests pass, this looks fine. It is curious that the kernel looks at the namespace of the invoking process for firmware load, but not for module load (as discussed in #7053 (comment)). It seems this should be consistent and possibly a bug for the /lib/firmware case (why wouldn't it load from the host for firmware?).

jdstrand · 2019-07-12T16:39:45Z

This PR is only exposing /lib/firmware from the host to the snap's runtime environment, which is correcting the weird behavior of the kernel (and the snap doesn't have access to /lib/firmware otherwise).

That said, we should blacklist /lib/firmware in layouts so that a snap can't provide firmware itself and then trigger an autoload.

UPDATE: I verified we are already blacklisting /lib/firmware

zyga · 2019-07-12T17:55:19Z

I force pushed to add the apparmor permission. Sorry about that, didn't think about it up front.

codecov-io · 2019-07-12T18:19:54Z

Codecov Report

Merging #7104 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7104      +/-   ##
==========================================
- Coverage    80.4%   80.39%   -0.01%     
==========================================
  Files         627      627              
  Lines       49081    49081              
==========================================
- Hits        39462    39459       -3     
- Misses       6570     6573       +3     
  Partials     3049     3049

Impacted Files	Coverage Δ
httputil/useragent.go	`77.14% <0%> (-5.72%)`	⬇️
cmd/snap-seccomp/main.go	`66.66% <0%> (-0.46%)`	⬇️
overlord/ifacestate/handlers.go	`65.42% <0%> (-0.22%)`	⬇️
overlord/hookstate/hookmgr.go	`73.8% <0%> (+0.95%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bb0643...a6e691c. Read the comment docs.

zyga · 2019-07-12T19:33:38Z

I will need to add the corresponding SELinux permission as well:

grep error: pattern not found, got:
----
type=PROCTITLE msg=audit(07/12/19 18:23:23.496:2273) : proctitle=/usr/libexec/snapd/snap-confine snap.test-snapd-service.test-snapd-sigusr2-service /usr/lib/snapd/snap-exec test-snapd-service.t 
type=SYSCALL msg=audit(07/12/19 18:23:23.496:2273) : arch=x86_64 syscall=mount success=yes exit=0 a0=0x40cc97 a1=0x7ffc8fc21c90 a2=0x0 a3=MS_REC|MS_SLAVE items=0 ppid=1 pid=21712 auid=unset uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=(none) ses=unset comm=snap-confine exe=/usr/libexec/snapd/snap-confine subj=system_u:system_r:snappy_confine_t:s0 key=(null) 
type=AVC msg=audit(07/12/19 18:23:23.496:2273) : avc:  denied  { mounton } for  pid=21712 comm=snap-confine path=/tmp/snap.rootfs_rBIvxg/lib/firmware dev="sda1" ino=16803111 scontext=system_u:system_r:snappy_confine_t:s0 tcontext=system_u:object_r:lib_t:s0 tclass=dir permissive=1

it's unclear if the last sate of the PR was reviewed

mvo5

Thanks for this fix!

mvo5 · 2019-07-15T08:40:22Z

I pushed the SELinux update but ideally someone knowledgable in this area would double check.

Pused my own stuff to the PR now

pedronis

lgtm (non SELinux bits)

Conan-Kudo · 2019-07-15T16:28:30Z

data/selinux/snappy.te

@@ -468,6 +469,7 @@ allow snappy_confine_t etc_t:file mounton;
 allow snappy_confine_t home_root_t:dir mounton;
 allow snappy_confine_t ifconfig_var_run_t:dir mounton;
 allow snappy_confine_t modules_object_t:dir mounton;
+allow snappy_confine_t lib_t:dir mounton;


Is this enough for this functionality to work? It looks okay, but I’m not sure if mounton implies that you can read the stuff inside once it is mounted inside. We did this with several other types, so…

Thanks! It seems to be enough, our tests are happy but maybe they are not deep enough - I am inclined to use as is and let @stolowski review later and tweak policy and tests if needed.

Conan-Kudo · 2019-07-15T16:29:39Z

cmd/snap-confine/snap-confine.apparmor.in

@@ -193,6 +193,9 @@
    mount options=(rw rbind) {,/usr}/lib{,32,64,x32}/modules/ -> /tmp/snap.rootfs_*{,/usr}/lib/modules/,
    mount options=(rw rslave) -> /tmp/snap.rootfs_*{,/usr}/lib/modules/,

+    mount options=(rw rbind) {,/usr}/lib{,32,64,x32}/firmware/ -> /tmp/snap.rootfs_*{,/usr}/lib/firmware/,
+    mount options=(rw rslave) -> /tmp/snap.rootfs_*{,/usr}/lib/firmware/,


Why are we allowing these to be mounted as read-write? These should be only read-only mounted.

Good catch! I think we need to talk to @zyga because e.g. the modules are probably also fine to be mounted RO.

I think the mount options are not super accurate as in, they are not really respected the way they should be. I will look at a pass that changes most of them to read only.

zyga requested review from mvo5 and jdstrand July 12, 2019 15:46

jdstrand previously approved these changes Jul 12, 2019

View reviewed changes

mvo5 added this to the 2.40 milestone Jul 12, 2019

zyga force-pushed the fix/firmware branch from 6f5ecb4 to a6e691c Compare July 12, 2019 17:54

pedronis requested a review from jdstrand July 12, 2019 21:05

jdstrand approved these changes Jul 12, 2019

View reviewed changes

mvo5 previously approved these changes Jul 15, 2019

View reviewed changes

selinux: add rules for bind mounting /lib/firmware/

b2d84c9

pedronis approved these changes Jul 15, 2019

View reviewed changes

Conan-Kudo suggested changes Jul 15, 2019

View reviewed changes

mvo5 merged commit dba8e09 into snapcore:master Jul 16, 2019

mvo5 modified the milestones: 2.40, 2.41 Jul 16, 2019

zyga deleted the fix/firmware branch July 23, 2019 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/snap-confine: bring /lib/firmware from the host #7104

cmd/snap-confine: bring /lib/firmware from the host #7104

zyga commented Jul 12, 2019

jdstrand left a comment •

edited

jdstrand commented Jul 12, 2019 •

edited

zyga commented Jul 12, 2019

codecov-io commented Jul 12, 2019

zyga commented Jul 12, 2019

mvo5 left a comment

mvo5 commented Jul 15, 2019

pedronis left a comment

Conan-Kudo Jul 15, 2019

mvo5 Jul 16, 2019

Conan-Kudo Jul 15, 2019

mvo5 Jul 16, 2019

zyga Jul 23, 2019

cmd/snap-confine: bring /lib/firmware from the host #7104

cmd/snap-confine: bring /lib/firmware from the host #7104

Conversation

zyga commented Jul 12, 2019

jdstrand left a comment • edited

Choose a reason for hiding this comment

jdstrand commented Jul 12, 2019 • edited

zyga commented Jul 12, 2019

codecov-io commented Jul 12, 2019

Codecov Report

zyga commented Jul 12, 2019

mvo5 left a comment

Choose a reason for hiding this comment

mvo5 commented Jul 15, 2019

pedronis left a comment

Choose a reason for hiding this comment

Conan-Kudo Jul 15, 2019

Choose a reason for hiding this comment

mvo5 Jul 16, 2019

Choose a reason for hiding this comment

Conan-Kudo Jul 15, 2019

Choose a reason for hiding this comment

mvo5 Jul 16, 2019

Choose a reason for hiding this comment

zyga Jul 23, 2019

Choose a reason for hiding this comment

jdstrand left a comment •

edited

jdstrand commented Jul 12, 2019 •

edited