Make sure gpio exported before apparmor setup #2

jocave · 2016-10-05T16:34:06Z

No description provided.

jdstrand

+1 overall, but have a question that needs answering.

jdstrand · 2016-10-05T17:35:52Z

interfaces/builtin/gpio.go

-}
-
-// ConnectedSlotSnippet - no slot snippets provided
-func (iface *GpioInterface) ConnectedSlotSnippet(plug *interfaces.Plug, slot *interfaces.Slot, securitySystem interfaces.SecuritySystem) ([]byte, error) {
 	switch securitySystem {


Moving to PermanentSlotSnippet is fine. It'll be the core or gadget snap that is the providing side, so no big deal moving the exports IMO.

jdstrand · 2016-10-05T17:36:11Z

interfaces/builtin/gpio.go

 	switch securitySystem {
 	case interfaces.SecurityUDev:
 		// NOTE: nothing unexports this GPIO when the slot is disconnected but
 		// AFAIK this doesn't hurt.
-		snippet := fmt.Sprintf(`ACTION=="add", SUBSYSTEM=="gpio", RUN+="/bin/echo %v > /syc/class/gpio/export"`, slot.Attrs["number"])
+		snippet := fmt.Sprintf(`SUBSYSTEM=="gpio", RUN+="/bin/sh -c '/bin/echo %v > /sys/class/gpio/export'"`, slot.Attrs["number"])


Why was ACTION=="add" removed?

I removed ACTION==add because:

the udev backend uses the command "udevadm trigger" (Note: no flags) to reload udev rules

the default ACTION sent by the trigger command is "change" so the command in the original rule would not be matched on install of the snap

we need to ensure the gpio is exported before any connection is made so the apparmor snippet can be created

By not specifying an action at all I think the rule is processed immediately as it only requires the device to be present. The guarantees that the gpio will be exported. The cost is that I don't believe it is actually possible to unexport a gpio with a rule like this in place. This may be a reasonable trade-off to guarantee the apparmor snippet can be created.

jocave · 2016-10-06T09:12:32Z

@jdstrand Could you also comment on whether you think it is acceptable for the gpio to be exported even when there is no plug connected to the slot, which implies the resource is not in use. I think this is not ideal, but unfortunately required if we want to guarantee it is exported before ConnectedPlugSnippet is processed.

jdstrand · 2016-10-06T15:52:35Z

@jocave - in terms of security policy-- I think exported by snapd anywhere is pretty much ok since the plugging snap just needs the gpio to be there and the security policy will block access to an exported gpio to gpio-disconnected plugged snaps. As for whether it is a problem for the kernel or the system to have it exported even if unused, I have no opinion. It doesn't seem like it would be a big issue, but I'll defer a definitive answer to a gpio expert.

Rename README->README.md

small tweaks for the store branch

zyga · 2016-12-12T15:04:04Z

Closing as I don't think we need this anymore,

Make sure that we pick up nvidia TLS libs otherwise applications using libGLX will fail in most mysterious ways. The problem was debugged in detail in this forum topic [1]. A simple test case using glxinfo would segfault both inside the snap mount namespace and in chroot, the backtrace pointing back to pthread_mutex_lock(): (gdb) bt #0 0x00007ffff7559fb4 in pthread_mutex_lock () from target:/lib/x86_64-linux-gnu/libc.so.6 #1 0x00007ffff6f5eddb in mt_mutex_lock (mutex=0x7ffff71e5180 <dispatchLock>) at glvnd_pthread.c:317 #2 0x00007ffff6f23f77 in LockDispatch () at GLdispatch.c:144 #3 0x00007ffff6f24115 in __glDispatchNewVendorID () at GLdispatch.c:198 #4 0x00007ffff7212607 in __glXLookupVendorByName (vendorName=0x60d160 "nvidia") at libglxmapping.c:442 #5 0x00007ffff7213811 in __glXLookupVendorByScreen (dpy=0x60aab0, screen=0) at libglxmapping.c:574 #6 0x00007ffff7213966 in __glXGetDynDispatch (dpy=0x60aab0, screen=0) at libglxmapping.c:608 #7 0x00007ffff7209563 in glXChooseVisual (dpy=0x60aab0, screen=0, attrib_list=0x609200) at libglx.c:215 snapcore#8 0x00007ffff7b89d58 in glXChooseVisual (dpy=0x60aab0, screen=0, attribList=0x609200) at g_libglglxwrapper.c:183 snapcore#9 0x0000000000401741 in ?? () snapcore#10 0x00007ffff7465830 in __libc_start_main () from target:/lib/x86_64-linux-gnu/libc.so.6 snapcore#11 0x0000000000401ea9 in ?? () Further debugging revealed that this may be related to libc/pthread & TLS handling. [1]. https://forum.snapcraft.io/t/nvidia-acceleration-on-chrome-and-firefox/4532/ Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Thank you for your contribution, the implemetnation is valid and have no negative impact on the existing code, go merge ! Cheers Fixes #1

* o/devicestate: add ensureCloudInitRestricted to DeviceManager This function will disable or restrict cloud-init after seeding is complete on an Ubuntu Core system. This is the full implementation of the mitigation of CVE-2020-11933 and https://bugs.launchpad.net/snapd/+bug/1879530. We want to allow some uses of cloud-init to continue to work, but we don't want cloud-init to be able to trigged arbitrarily after first boot because at that point the device should be considered "locked down". Signed-off-by: Ian Johnson <ian.johnson@canonical.com> * o/devicestate/devicemgr: set cloudInitAlreadyRestricted bool, use to skip early If we already restricted cloud-init we can set a manager bool to prevent unnecessary checking again for the duration of the manager. If snapd is restarted for example, it will ping cloud-init again to check the status and re-set the variable. Also add tests around this, including a full call to devicemgr.Ensure() to test that Ensure() calls ensureCloudInitRestricted(). Signed-off-by: Ian Johnson <ian.johnson@canonical.com> * o/devicestate/devicemgr, tests: adjust error msg, test names Signed-off-by: Ian Johnson <ian.johnson@canonical.com> * tests/o/devicestate: disable device registration in cloud-init Ensure() test Signed-off-by: Ian Johnson <ian.johnson@canonical.com> * tests/o/devicestate/cloudinit: simplify tests * Move duplicated setup to the SetUpTest preamble * Use a shared mockLogger instead of creating individual ones * Fix import ordering * Add missing MockCommand restores Signed-off-by: Ian Johnson <ian.johnson@canonical.com> * o/devicestate/devicemgr: adjust EnsureBefore logic to use time instead of count Previously, we were using the number of times we were called with a counter to determine if a specific time had elapsed, but this is fragile since EnsureBefore does not guarantee that we run in the specified time exactly, it ensures only that we will get re-run at some point before the specified time. This means that we could get scheduled to re-run very soon in time and incorrectly conclude that a significant amount of time had elapsed when in fact it had not. As a matter of fact, this bug was actually being relied upon (unknowingly in my defense) in the unit tests, where a call to settle() would just call Ensure repeatedly and cause the logic to assume that the specific time had elapsed. The solution to all of this is to keep track of time instead of count. Here, we save the first time we encounter cloud-init in one of the states we track it in, and check in all future instances if the time has actually elapsed before continuing. This does require significant mocking of time in the unit tests, but otherwise is a good solution. Signed-off-by: Ian Johnson <ian.johnson@canonical.com> * o/devicestate/devicemgr: while waiting in error state, only output msg once This previously would output a message every time EnsureCloudInitRestricted was called before hitting the timeout, but really we should only output it on the first time we find cloud-init in this state. Also add a test for this scenario, with Ensure getting scheduled before the 3 minute timeout has elapsed, so that we also have some test coverage of the timeout logic for error, previously we only had this logic covered for the CloudInitEnabled state. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

Merge pull request snapcore#9025 from mvo5/feature/cloud-init-disable This is #2 from the snapd-private repo used to address the cloud-init fix. Built on top of snapcore#9024.

This change adds a new line in the /etc/sudoers file to prevent an error which is happening in debian and fedora after latest updates: # syntax error, unexpected WORD, expecting END or ':' or '\n' The error: Error: 2020-09-28 21:54:59 Error executing google:debian-sid-64:tests/main/sudo-env (sep282050-048295) : ----- ... + tests.session -u test exec sudo sh -c 'echo :$PATH:' /etc/sudoers:28: syntax error, unexpected WORD, expecting END or ':' or '\n' test ALL=(ALL) NOPASSWD:ALL ^~~~ We trust you have received the usual lecture from the local System Administrator. It usually boils down to these three things: #1) Respect the privacy of others. #2) Think before you type. #3) With great power comes great responsibility. sudo: a terminal is required to read the password; either use the -S option to read from standard input or configure an askpass helper

Make sure gpio exported before apparmor setup

e25fe97

jdstrand reviewed Oct 5, 2016

View reviewed changes

zyga added a commit that referenced this pull request Nov 7, 2016

Merge pull request #2 from mvo5/master

7178271

Rename README->README.md

zyga pushed a commit that referenced this pull request Nov 25, 2016

Merge pull request #2 from mvo5/alex-store

b9413b9

small tweaks for the store branch

zyga closed this Dec 12, 2016

zyga pushed a commit that referenced this pull request Jul 2, 2018

Merge pull request #2 from stolowski/fix-subsystem-with-existingdevices

0ab5fc3

Thank you for your contribution, the implemetnation is valid and have no negative impact on the existing code, go merge ! Cheers Fixes #1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure gpio exported before apparmor setup #2

Make sure gpio exported before apparmor setup #2

jocave commented Oct 5, 2016

jdstrand left a comment

jdstrand Oct 5, 2016

jdstrand Oct 5, 2016

jocave Oct 6, 2016

jocave commented Oct 6, 2016

jdstrand commented Oct 6, 2016

zyga commented Dec 12, 2016

Make sure gpio exported before apparmor setup #2

Make sure gpio exported before apparmor setup #2

Conversation

jocave commented Oct 5, 2016

jdstrand left a comment

Choose a reason for hiding this comment

jdstrand Oct 5, 2016

Choose a reason for hiding this comment

jdstrand Oct 5, 2016

Choose a reason for hiding this comment

jocave Oct 6, 2016

Choose a reason for hiding this comment

jocave commented Oct 6, 2016

jdstrand commented Oct 6, 2016

zyga commented Dec 12, 2016