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

cmd,interfaces: add initial support for Solus #3720

Closed
wants to merge 3 commits into from

Conversation

ikeydoherty
Copy link
Contributor

@ikeydoherty ikeydoherty commented Aug 11, 2017

  • Do not use re-exec on Solus
  • Allow autogen to pass $@ arguments to configure script
  • Recognise Solus as a "classic" distribution
  • Support usr-merge/bi-arch confinement library paths
  • Fix confinement support for "nvidia-arch" GL functionality
  • Support Solus's GL "linux-driver-management" layout
  • Fix bind-mount confinement issues for snapd directory

Signed-off-by: Ikey Doherty ikey@solus-project.com

Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial feeling is LGTM though please check the libraries and remove any unneeded cruft that is just there because we also support a weird hybrid ubuntu 14.04 with deputy systemd that you don't need.

Some of the apparmor rules may need tweaking later on, specifically we might make the opengl parts conditional on solus but I don't think we ought to block on that.

/usr/lib64/libcap.so* mr,
/usr/lib64/libcgmanager.so* mr,
/usr/lib64/libdl-[0-9]*.so* mr,
/usr/lib64/libnih.so* mr,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libnih is I think only used on 14.04 systems, you may not need those unless they are really present on solus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll drop nih

/usr/lib64/libgcc_s.so* mr,

# normal libs in order biarch + usr-merged
/usr/lib64/libapparmor.so* mr,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above (may need pruning)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment from me on these. Instead of adding yours, can you instead change this:

        /lib/@{multiarch}/ld-*.so mr,
        # libc, you are funny
        /lib/@{multiarch}/libc{,-[0-9]*}.so* mr,
        /lib/@{multiarch}/libpthread{,-[0-9]*}.so* mr,
        /lib/@{multiarch}/librt{,-[0-9]*}.so* mr,
        /lib/@{multiarch}/libgcc_s.so* mr,
        # normal libs in order
        /lib/@{multiarch}/libapparmor.so* mr,
        /lib/@{multiarch}/libcgmanager.so* mr,
        /lib/@{multiarch}/libnih.so* mr,
        /lib/@{multiarch}/libnih-dbus.so* mr,
        /lib/@{multiarch}/libdbus-1.so* mr,
        /lib/@{multiarch}/libudev.so* mr,
        /usr/lib/@{multiarch}/libseccomp.so* mr,
        /lib/@{multiarch}/libseccomp.so* mr,

to:

    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}ld-*.so mrix,
    # libc, you are funny
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libc{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libpthread{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}librt{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libgcc_s.so* mr,
    # normal libs in order
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libapparmor.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libcgmanager.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libnih.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libnih-dbus.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libdbus-1.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libudev.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libseccomp.so* mr,

mount options=(rw rslave) -> /tmp/snap.rootfs_*/lib/modules/,

mount options=(rw rbind) /var/log/ -> /tmp/snap.rootfs_*/var/log/,
mount options=(rw rslave) -> /tmp/snap.rootfs_*/var/log/,

mount options=(rw rbind) /usr/src/ -> /tmp/snap.rootfs_*/usr/src/,
mount options=(rw rslave) -> /tmp/snap.rootfs_*/usr/src/,

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about those two rules, they look fine but I wasn't aware we need them or that we perform those operations.

Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better but one more try maybe :)

/usr/lib64/libdbus-1.so* mr,
/usr/lib64/libudev.so* mr,
/usr/lib64/libseccomp.so* mr,
/usr/lib64/libseccomp.so* mr,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate

@zyga zyga changed the title Add initial support for Solus cmd,interfaces: add initial support for Solus Aug 11, 2017
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one question.

@@ -45,6 +45,9 @@ case "$ID" in
# patches find their way into the distribution.
extra_opts="--libexecdir=/usr/lib/snapd --disable-apparmor"
;;
solus)
extra_opts="--with-snap-mount-dir=/snap --enable-merged-usr --enable-nvidia-arch $@"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious about the $@ I didn't notice it on my first read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Solus package we do:

%autogen which actually calls ./autogen.sh %CONFOPTS% which passes all the configure arguments along, this makes autogen.sh complete the chain and pass them along to configure so that the whole thing is configured properly in one go

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anyone actually use this script in packaging? AFAIK, it was originally created for the test harness...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - it lets you configure the snap confine part, I'm using that within the package.yml ...

@ikeydoherty
Copy link
Contributor Author

I'd agree that as the Solus support gets fleshed out that we might want that stuff to be conditional to save up spamming the rules/filters, and once we're at the multilib cross-roads we might want some Solus helper code to correctly append to the rules

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable. One question, one problem we need to solve, and we need a review from jdstrand on those rule changes.

/usr/lib64/libc{,-[0-9]*}.so* mr,
/usr/lib64/libpthread{,-[0-9]*}.so* mr,
/usr/lib64/librt{,-[0-9]*}.so* mr,
/usr/lib64/libgcc_s.so* mr,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please get a pass by @jdstrand on these rules.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These rules are fine on their own, but I think we want to make this more futureproof since we're starting to bring on more and more apparmor-enabled distros that don't follow Debian/Ubuntu directories. I suggest instead of your changes, changing these:

    /lib/@{multiarch}/ld-*.so mr,
    # libc, you are funny
    /lib/@{multiarch}/libc{,-[0-9]*}.so* mr,
    /lib/@{multiarch}/libpthread{,-[0-9]*}.so* mr,
    /lib/@{multiarch}/librt{,-[0-9]*}.so* mr,
    /lib/@{multiarch}/libgcc_s.so* mr,
    # normal libs in order
    /lib/@{multiarch}/libapparmor.so* mr,
    /lib/@{multiarch}/libcgmanager.so* mr,
    /lib/@{multiarch}/libdl-[0-9]*.so* mr,
    /lib/@{multiarch}/libnih.so* mr,
    /lib/@{multiarch}/libnih-dbus.so* mr,
    /lib/@{multiarch}/libdbus-1.so* mr,
    /lib/@{multiarch}/libudev.so* mr,
    /usr/lib/@{multiarch}/libseccomp.so* mr,
    /lib/@{multiarch}/libseccomp.so* mr,

to:

    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}ld-*.so mrix,
    # libc, you are funny
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libc{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libpthread{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}librt{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libgcc_s.so* mr,
    # normal libs in order
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libapparmor.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libcgmanager.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libdl-[0-9]*.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libnih.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libnih-dbus.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libdbus-1.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libudev.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libseccomp.so* mr,

/var/lib/snapd/hostfs/usr/lib*/glx-provider/** rm,
/var/lib/snapd/hostfs/usr/lib*/libcuda* rm,
/var/lib/snapd/hostfs/usr/lib*/libn* rm,
/var/lib/snapd/hostfs/usr/lib*/lib{GL,EGL}* rm,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that necessary now and wasn't necessary before? @zyga, @mvo?

Note we have some logic under cmd/snap-confine/mount-support-nvidia.c related to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the comment in the context does explain that, its required for Solus NVIDIA libraries, which are accessed under /usr/lib{,64}/glx-provider due to the /usr/lib*/libGL type symlinks, and the rest of the libs (actual NVIDIA ones, not conflicting OpenGL ones) live in the main library directory. And the non-Solus part was never present because the rules were created for Arch Linux which doesn't actually have full apparmor confinement, so it was never exposed until now.

Copy link
Collaborator

@zyga zyga Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly as @ikeydoherty explained. In solus nvidia layout is similar to arch so snap-confine was built with --with-nvidia-arch which was never confined to begin with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zyga Sorry for being a bit slow, here but the question is this: how are people able to use nvidia libraries on Ubuntu if we don't have those rules, and where is the rule in the nvidia support of snap-confine that puts those libraries into /var/lib/snapd/lib/gl as they should be?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two implementations of nvidia support, one for "ubuntu" like systems and one for "arch" like systems. The ubuntu part had the correct apparmor rules because we had to write those. The arch part never had because there was no good place to actually explore and test those. Solus is "arch" like and with enabled apparmor was the first distribution to explore this combination.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that on "arch" like systems /var/lib/snapd/lib/gl contains a tmpfs with a symlink farm so the effective files we access are different from what we did on Ubuntu.

mkversion.sh Outdated
@@ -32,6 +32,12 @@ if [ ! -z "$1" ]; then
o=shell
fi

# If the version is passed in the environment during build, let's use that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't look right. It implies the logic that is telling the code what its version is will be outside the code itself, which means there's a very realistic probability of updating the code without changing the version. That will create a lot of very confusing issues, including serious breakages as we do take the version into account in some places.

The right way of doing this is running mkversion.sh when the code is extracted from git. The result will then have the version encoded statically and may be shipped anywhere without the git metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but this isn't being built from git, it's being built from the vendor tarball (which apparently doesn't know its own version) - thus passing the version in from the package metadata during build

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niemeyer this matches what we do elsewhere but since we don't want to force people to run dpkg-parsechangelogs we offer a simple "this is the version right here" already (via command line argument). This patch extends the feature to environment value since it is easier to integrate in souls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's a good idea for mkversion to just be pulling random shell variables. I added the ability to pass it as a parameter so that you can control the input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But mkversion is within the autogen pipeline, so to support correct and proper autogen invocation, the call to mkversion has to "learn" it from somewhere.. I also wouldn't really call it "random" given that its part of the documented ypkg build environment, which is sanely controlled. If it's gonna be that much of a problem I can split it to call mkversion prior to the autogen call, but I'd still need the parameters passed to configure like I'm doing now (Otherwise, it's a pretty pointless autogen)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest calling it before the autogen call, as I do for the Fedora packaging.

Copy link
Contributor

@niemeyer niemeyer Aug 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want environment variable, positional parameters, or flag parameters that provide a version into this script. We want this script to tell the version based on the git data, and we want this version to be statically saved into the filesystem so that it can be packed. If we have vendor tarballs that do not contain their own version today, these tarballs are broken and should be fixed. We want the source code to know its version, instead of expecting the person that builds the tarball to keep its version in mind and pass it in via a parameter. That latter approach will definitely result in binaries reporting their version incorrectly eventually.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please fix any such broken tarballs now, while it's early and we haven't suffered any major pain due to that.

@codecov-io
Copy link

codecov-io commented Aug 14, 2017

Codecov Report

Merging #3720 into master will decrease coverage by <.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3720      +/-   ##
==========================================
- Coverage    75.7%   75.69%   -0.01%     
==========================================
  Files         391      391              
  Lines       33936    33938       +2     
==========================================
- Hits        25690    25689       -1     
- Misses       6420     6423       +3     
  Partials     1826     1826
Impacted Files Coverage Δ
interfaces/builtin/opengl.go 100% <ø> (ø) ⬆️
dirs/dirs.go 95.06% <0%> (-2.41%) ⬇️
cmd/cmd.go 83.87% <100%> (ø) ⬆️
cmd/snap/cmd_aliases.go 93.33% <0%> (-1.67%) ⬇️
interfaces/sorting.go 98.71% <0%> (ø) ⬆️

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 bdd3e0a...b5c1804. Read the comment docs.

@ikeydoherty
Copy link
Contributor Author

I've rebased the PR on master (dropping the commits/merges added on top of the commit) - and removed the mkversion changes, we'll invoke that in packaging prior to calling %autogen

 - Do not use re-exec on Solus
 - Allow autogen to pass `$@` arguments to `configure` script
 - Recognise Solus as a "classic" distribution
 - Support usr-merge/bi-arch confinement library paths
 - Fix confinement support for "nvidia-arch" GL functionality
 - Support Solus's GL "linux-driver-management" layout
 - Fix bind-mount confinement issues for snapd directory

Signed-off-by: Ikey Doherty <ikey@solus-project.com>
This hasn't _yet_ presented an issue in full confinement environments,
as the only fully confined environment available is Ubuntu, and the snaps
being tested are also built using the Ubuntu libraries.

This change will now only make the NVIDIA libraries available if the nvidia
kernel module is loaded and present, to ensure that confined snaps running
on open source drivers will not run into any linking issues by exposing the
host GL library into the runtime.

Signed-off-by: Ikey Doherty <ikey@solus-project.com>
Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The security policy overall looks good, but I think we can future-proof a bit more (while still being cautious with our snap-confine's rules) and have a few small changes I'd like to see.

@@ -67,7 +67,7 @@ func distroSupportsReExec() bool {
return false
}
switch release.ReleaseInfo.ID {
case "fedora", "centos", "rhel", "opensuse", "suse", "poky", "arch":
case "fedora", "centos", "rhel", "opensuse", "suse", "poky", "arch", "solus":

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not (necessarily) to be fixed in this PR, but only seeing this code for the first time, I was surprised this list wasn't alphabetized.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll adjust this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why we don't just have a list where re-exec is enabled rather than a disable list. The enable list is many times shorter (it's just Ubuntu and Debian).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought the same thing tbh

/usr/lib64/libc{,-[0-9]*}.so* mr,
/usr/lib64/libpthread{,-[0-9]*}.so* mr,
/usr/lib64/librt{,-[0-9]*}.so* mr,
/usr/lib64/libgcc_s.so* mr,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These rules are fine on their own, but I think we want to make this more futureproof since we're starting to bring on more and more apparmor-enabled distros that don't follow Debian/Ubuntu directories. I suggest instead of your changes, changing these:

    /lib/@{multiarch}/ld-*.so mr,
    # libc, you are funny
    /lib/@{multiarch}/libc{,-[0-9]*}.so* mr,
    /lib/@{multiarch}/libpthread{,-[0-9]*}.so* mr,
    /lib/@{multiarch}/librt{,-[0-9]*}.so* mr,
    /lib/@{multiarch}/libgcc_s.so* mr,
    # normal libs in order
    /lib/@{multiarch}/libapparmor.so* mr,
    /lib/@{multiarch}/libcgmanager.so* mr,
    /lib/@{multiarch}/libdl-[0-9]*.so* mr,
    /lib/@{multiarch}/libnih.so* mr,
    /lib/@{multiarch}/libnih-dbus.so* mr,
    /lib/@{multiarch}/libdbus-1.so* mr,
    /lib/@{multiarch}/libudev.so* mr,
    /usr/lib/@{multiarch}/libseccomp.so* mr,
    /lib/@{multiarch}/libseccomp.so* mr,

to:

    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}ld-*.so mrix,
    # libc, you are funny
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libc{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libpthread{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}librt{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libgcc_s.so* mr,
    # normal libs in order
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libapparmor.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libcgmanager.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libdl-[0-9]*.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libnih.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libnih-dbus.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libdbus-1.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libudev.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libseccomp.so* mr,

/usr/lib64/libdl-[0-9]*.so* mr,
/usr/lib64/libdbus-1.so* mr,
/usr/lib64/libudev.so* mr,
/usr/lib64/libseccomp.so* mr,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@@ -154,13 +168,15 @@
mount options=(rw rslave) -> /tmp/snap.rootfs_*/run/,

mount options=(rw rbind) {/usr,}/lib/modules/ -> /tmp/snap.rootfs_*/lib/modules/,
mount options=(rw rbind) {/usr,}/lib64/modules/ -> /tmp/snap.rootfs_*/lib/modules/,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding your rule, can you change this:

mount options=(rw rbind) {/usr,}/lib/modules/ -> /tmp/snap.rootfs_*/lib/modules/,

to:

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

/usr/lib64/libgcc_s.so* mr,

# normal libs in order biarch + usr-merged
/usr/lib64/libapparmor.so* mr,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment from me on these. Instead of adding yours, can you instead change this:

        /lib/@{multiarch}/ld-*.so mr,
        # libc, you are funny
        /lib/@{multiarch}/libc{,-[0-9]*}.so* mr,
        /lib/@{multiarch}/libpthread{,-[0-9]*}.so* mr,
        /lib/@{multiarch}/librt{,-[0-9]*}.so* mr,
        /lib/@{multiarch}/libgcc_s.so* mr,
        # normal libs in order
        /lib/@{multiarch}/libapparmor.so* mr,
        /lib/@{multiarch}/libcgmanager.so* mr,
        /lib/@{multiarch}/libnih.so* mr,
        /lib/@{multiarch}/libnih-dbus.so* mr,
        /lib/@{multiarch}/libdbus-1.so* mr,
        /lib/@{multiarch}/libudev.so* mr,
        /usr/lib/@{multiarch}/libseccomp.so* mr,
        /lib/@{multiarch}/libseccomp.so* mr,

to:

    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}ld-*.so mrix,
    # libc, you are funny
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libc{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libpthread{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}librt{,-[0-9]*}.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libgcc_s.so* mr,
    # normal libs in order
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libapparmor.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libcgmanager.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libnih.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libnih-dbus.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libdbus-1.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libudev.so* mr,
    /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libseccomp.so* mr,

@@ -235,6 +251,9 @@
/sys/module/nvidia/version r,
/usr/** r,
mount options=(rw bind) /usr/lib/nvidia-*/ -> /{tmp/snap.rootfs_*/,}var/lib/snapd/lib/gl/,
/tmp/snap.rootfs_*/var/lib/snapd/lib/gl/* w,
mount fstype=tmpfs none -> /tmp/snap.rootfs_*/var/lib/snapd/lib/gl/,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be able to change this rule to be:

mount fstype=tmpfs options=(rw nodev nosuid) none -> /tmp/snap.rootfs_*/var/lib/snapd/lib/gl/,

Can you do so and verify it still works?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I originally had, which is why I stripped options from it (didn't work, I'd copy-edited the other tmpfs one i'd found)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you paste the denial when it didn't work?

@@ -235,6 +251,9 @@
/sys/module/nvidia/version r,
/usr/** r,
mount options=(rw bind) /usr/lib/nvidia-*/ -> /{tmp/snap.rootfs_*/,}var/lib/snapd/lib/gl/,
/tmp/snap.rootfs_*/var/lib/snapd/lib/gl/* w,
mount fstype=tmpfs none -> /tmp/snap.rootfs_*/var/lib/snapd/lib/gl/,
mount options=(remount,ro) -> /tmp/snap.rootfs_*/var/lib/snapd/lib/gl/,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with other mount rules in the policy, can you change this rule of yours to be this equivalent rule (options may be space or comma separated):

mount options=(remount ro) -> /tmp/snap.rootfs_*/var/lib/snapd/lib/gl/,

/usr/lib64/libdbus-1.so* mr,
/usr/lib64/libudev.so* mr,
/usr/lib64/libseccomp.so* mr,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

/var/lib/snapd/hostfs/usr/lib*/glx-provider/** rm,
/var/lib/snapd/hostfs/usr/lib*/libcuda* rm,
/var/lib/snapd/hostfs/usr/lib*/libn* rm,
/var/lib/snapd/hostfs/usr/lib*/lib{GL,EGL}* rm,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want something more like this:

# Solus LDM locations
/var/lib/snapd/hostfs/usr/lib{,32,64}/glx-provider/**.so{,.*} rm,
/var/lib/snapd/hostfs/usr/lib{,32,64}/libcuda*.so{,.*} rm,
/var/lib/snapd/hostfs/usr/lib{,32,64}/libnvidia*.so{,.*} rm,
/var/lib/snapd/hostfs/usr/lib{,32,64}/lib{GL,EGL}.so{,.*} rm,

Can you test and report back any errors? I thought using lib{,32,64} is good for future-proofing, but I also wanted to tighten up the rules to only mmap() .so files. Also, libn* seemed too generic so I guessed you wanted libnvidia*, but please adjust as necessary. If the above doesn't work for you, feel free to adjust and/or paste the apparmor denials.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well libnv maybe, there are a few in there but yeah we'll tighten them up and allow the biarch approach (We do have the lib32 dirs too fwiw)

@jdstrand
Copy link

Note if you merge with upstream/master, you'll get #3789 which will address my comments regarding future-proofing the policy. Other comments remain.

@zyga
Copy link
Collaborator

zyga commented Aug 30, 2017

I'll try to pick this up and get it merged.

@niemeyer
Copy link
Contributor

niemeyer commented Sep 1, 2017

I've heard from @zyga today in the standup that this is being broken down into smaller pieces, so I'm closing this one.

@zyga, please keep us posted about the progress on this.

@niemeyer niemeyer closed this Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants