interfaces/seccomp/template: adding kcmp to allow Mesa usecases #12673

lissyx · 2023-03-24T22:29:32Z

For fixing https://bugs.launchpad.net/snapd/+bug/1998980 implement the suggested fix of allowing kcmp in the base template.

alexmurray

Can you please add a comment above this saying that kcmp is guarded in the kernel via ptrace with PTRACE_MODE_READ_REALCREDS such that the calling process must already be able to ptrace the target processes and so this is safe.

lissyx · 2023-03-27T07:10:54Z

Can you please add a comment above this saying that kcmp is guarded in the kernel via ptrace with PTRACE_MODE_READ_REALCREDS such that the calling process must already be able to ptrace the target processes and so this is safe.

Well, the suggested change is not working, that's more worrying: - Setup snap "core" (15061) security profiles (cannot compile /var/lib/snapd/seccomp/bpf/snap.core.hook.configure.src: error: cannot parse line: cannot parse token "KCMP_FILE" (line "kcmp - - KCMP_FILE")), I need to dig into seccomp ...

alexmurray · 2023-03-27T07:57:13Z

Ah okay, we need to teach snap-seccomp about KCMP_FILE - something like the following change is also needed:

diff --git a/cmd/snap-seccomp/main.go b/cmd/snap-seccomp/main.go
index 40d3dfbbdf..5356783eb6 100644
--- a/cmd/snap-seccomp/main.go
+++ b/cmd/snap-seccomp/main.go
@@ -28,6 +28,7 @@ package main
 //#include <errno.h>
 //#include <linux/can.h>
 //#include <linux/netlink.h>
+//#include <linux/kcmp.h>
 //#include <sched.h>
 //#include <search.h>
 //#include <stdbool.h>
@@ -182,6 +183,33 @@ package main
 // #ifndef PTRACE_SETFPXREGS
 // #define PTRACE_SETFPXREGS 19
 // #endif
+
+// /* Define missing kcmp constants */
+// #ifndef KCMP_FILE
+// #define KCMP_FILE 0
+// #endif
+// #ifndef KCMP_VM
+// #define KCMP_VM 1
+// #endif
+// #ifndef KCMP_FILES
+// #define KCMP_FILES 2
+// #endif
+// #ifndef KCMP_FS
+// #define KCMP_FS 3
+// #endif
+// #ifndef KCMP_SIGHAND
+// #define KCMP_SIGHAND 4
+// #endif
+// #ifndef KCMP_IO
+// #define KCMP_IO 5
+// #endif
+// #ifndef KCMP_SYSVSEM
+// #define KCMP_SYSVSEM 6
+// #endif
+// #ifndef KCMP_EPOLL_TFD
+// #define KCMP_EPOLL_TFD 7
+// #endif
+
 import "C"
 
 import (
@@ -439,6 +467,16 @@ var seccompResolver = map[string]uint64{
 	"PTRACE_PEEKUSR":  C.PTRACE_PEEKUSER,
 	"PTRACE_PEEKUSER": C.PTRACE_PEEKUSER,
 	"PTRACE_CONT":     C.PTRACE_CONT,
+
+	// man 2 kcmp
+	"KCMP_FILE":      C.KCMP_FILE,
+	"KCMP_VM":        C.KCMP_VM,
+	"KCMP_FILES":     C.KCMP_FILES,
+	"KCMP_FS":        C.KCMP_FS,
+	"KCMP_SIGHAND":   C.KCMP_SIGHAND,
+	"KCMP_IO":        C.KCMP_IO,
+	"KCMP_SYSVSEM":   C.KCMP_SYSVSEM,
+	"KCMP_EPOLL_TFD": C.KCMP_EPOLL_TFD,
 }
 
 // DpkgArchToScmpArch takes a dpkg architecture and converts it to

alexmurray

LGTM

lissyx · 2023-03-29T07:57:38Z

@alexmurray Can this be merged and when can we expect it ships to users ?

alexmurray · 2023-03-30T01:41:19Z

I'm not on the snapd team so I don't have permission to merge PRs nor decide what goes in each scheduled release - @mvo5 can you (or someone on your team) please take a look at this? Thanks

lissyx · 2023-04-06T15:36:09Z

Is there anything blocking now ? @alexmurray @mvo5

alexmurray · 2023-04-11T01:30:04Z

Nothing on my side - it is waiting on the snapd team to review and merge it.

kenvandine · 2023-05-25T19:38:30Z

@mvo5 can we please get someone to review this? I've been seeing the same sort of spamming of the journal from gnome-shell in core desktop as well, which I think this would fix.

jhenstridge

This seems reasonable. I have some concerns about how the fallback constants are being declared though.

jhenstridge · 2023-05-26T03:27:55Z

cmd/snap-seccomp/main.go

+// /* Define missing kcmp constants */
+// #ifndef KCMP_FILE
+// #define KCMP_FILE 0
+// #endif
+// #ifndef KCMP_VM
+// #define KCMP_VM 1
+// #endif
+// #ifndef KCMP_FILES
+// #define KCMP_FILES 2
+// #endif
+// #ifndef KCMP_FS
+// #define KCMP_FS 3
+// #endif
+// #ifndef KCMP_SIGHAND
+// #define KCMP_SIGHAND 4
+// #endif
+// #ifndef KCMP_IO
+// #define KCMP_IO 5
+// #endif
+// #ifndef KCMP_SYSVSEM
+// #define KCMP_SYSVSEM 6
+// #endif
+// #ifndef KCMP_EPOLL_TFD
+// #define KCMP_EPOLL_TFD 7
+// #endif


In the upstream kernel headers, these symbols come from the kcmp_type enum:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/kcmp.h

They are not preprocessor macros, so all of these #ifndef blocks will be included and we'll shadow the enumeration values with #defines to integers. So we're always ignoring the enumeration values from the kernel with this.

Maybe a better option would be to use the macros in <linux/version.h> to decide whether we've got a new enough kernel to rely on <linux/kcmp.h>, and then declare the enumeration (or macros) if not.

Ok, so <linux/kcmp.h> was added in 3.19

And KCMP_EPOLL_TFD was added in 4.13

Given we only care about the KCMP_FILE constant for now, we can probably ignore KCMP_EPOLL_TFD.

So the fallback could look something like:

#include <linux/version.h> #if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 19, 0) # include <linux/kcmp.h> #else # define KCMP_FILE 0 # define KCMP_VM 1 ... #endif

The man page says kcmp() was added in 3.5, so I'm not sure whether the version check is fully correct. It's far enough back that I'm not sure it really matters though.

Right, 3.19 made a move to uapi

jhenstridge · 2023-05-26T08:01:24Z

cmd/snap-seccomp/main.go

+// #if LINUX_VERSION_CODE < KERNEL_VERSION(4,13,0)
+// #define KCMP_EPOLL_TFD 7
+// #endif //  LINUX_VERSION_CODE < KERNEL_VERSION(4,13,0)
+//#endif //  LINUX_VERSION_CODE < KERNEL_VERSION(3,5,0)


I think you were right to pick 3.19 first time around: while the system call is in 3.5, the <linux/kcmp.h> header is not installed (at least it doesn't seem to be in trusty's linux-libc-dev package built from 3.13).

Also as a style note, I'd suggest putting the primary path of these if statements first with fallbacks after. Here we've got the main `#include <linux.kcmp.h> buried in the middle of two lots of fallbacks, so it isn't as obvious what the common case is going to be.

codecov-commenter · 2023-05-26T08:10:52Z

Codecov Report

Merging #12673 (04ea404) into master (4d64bb6) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master   #12673   +/-   ##
=======================================
  Coverage   78.60%   78.60%           
=======================================
  Files         991      991           
  Lines      122768   122768           
=======================================
+ Hits        96498    96504    +6     
+ Misses      20193    20189    -4     
+ Partials     6077     6075    -2

Flag	Coverage Δ
unittests	`78.60% <ø> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/snap-seccomp/main.go	`63.38% <ø> (ø)`

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mvo5

Thanks, this looks reasonable.

lissyx · 2023-05-26T08:36:37Z

# github.com/snapcore/snapd/cmd/snap-seccomp
../cmd/snap-seccomp/main.go:462:20: could not determine kind of name for C.KCMP_FILE
../cmd/snap-seccomp/main.go:464:20: could not determine kind of name for C.KCMP_FILES
../cmd/snap-seccomp/main.go:465:20: could not determine kind of name for C.KCMP_FS
../cmd/snap-seccomp/main.go:467:20: could not determine kind of name for C.KCMP_IO
../cmd/snap-seccomp/main.go:466:20: could not determine kind of name for C.KCMP_SIGHAND
../cmd/snap-seccomp/main.go:468:20: could not determine kind of name for C.KCMP_SYSVSEM
../cmd/snap-seccomp/main.go:463:20: could not determine kind of name for C.KCMP_VM

I know nothing about Go ... Maybe we still need somehow to #define those to make C.xxx happy?

mvo5 · 2023-05-26T08:39:11Z

@lissyx I merged "master" into your brand and pushed so that we benefit from the latest fixes in snapd for the integration tests (various external components have moved on and needed updates since this branch was created). Hope that is okay.

lissyx · 2023-05-26T08:40:10Z

@lissyx I merged "master" into your brand and pushed so that we benefit from the latest fixes in snapd for the integration tests (various external components have moved on and needed updates since this branch was created). Hope that is okay.

I rebased on master before my previous push already ?

mvo5 · 2023-05-26T08:44:28Z

@lissyx I merged "master" into your brand and pushed so that we benefit from the latest fixes in snapd for the integration tests (various external components have moved on and needed updates since this branch was created). Hope that is okay.

I rebased on master before my previous push already ?

Uh, sorry for that then, please just force push again, we must have had a mid-air collision then, I looked at the branch and thought it was based on a some weeks old "master" (apologies again!) [edit: or juts ignore as I will merge via a "rebase-merge" anyway so it should not matter :)]

lissyx · 2023-05-26T08:47:52Z

@lissyx I merged "master" into your brand and pushed so that we benefit from the latest fixes in snapd for the integration tests (various external components have moved on and needed updates since this branch was created). Hope that is okay.

I rebased on master before my previous push already ?

Uh, sorry for that then, please just force push again, we must have had a mid-air collision then, I looked at the branch and thought it was based on a some weeks old "master" (apologies again!) [edit: or juts ignore as I will merge via a "rebase-merge" anyway so it should not matter :)]

no problem, let's see if your push passes. if it does not, I'll need help because I have no idea what is wrong ...

lissyx · 2023-05-26T08:52:44Z

@mvo5 Still failing with the same error:

../cmd/snap-seccomp/main.go:462:20: could not determine kind of name for C.KCMP_FILE
../cmd/snap-seccomp/main.go:464:20: could not determine kind of name for C.KCMP_FILES
../cmd/snap-seccomp/main.go:465:20: could not determine kind of name for C.KCMP_FS
../cmd/snap-seccomp/main.go:467:20: could not determine kind of name for C.KCMP_IO
../cmd/snap-seccomp/main.go:466:20: could not determine kind of name for C.KCMP_SIGHAND
../cmd/snap-seccomp/main.go:468:20: could not determine kind of name for C.KCMP_SYSVSEM
../cmd/snap-seccomp/main.go:463:20: could not determine kind of name for C.KCMP_VM

mvo5 · 2023-05-26T08:54:01Z

@mvo5 Still failing with the same error:

../cmd/snap-seccomp/main.go:462:20: could not determine kind of name for C.KCMP_FILE
../cmd/snap-seccomp/main.go:464:20: could not determine kind of name for C.KCMP_FILES
../cmd/snap-seccomp/main.go:465:20: could not determine kind of name for C.KCMP_FS
../cmd/snap-seccomp/main.go:467:20: could not determine kind of name for C.KCMP_IO
../cmd/snap-seccomp/main.go:466:20: could not determine kind of name for C.KCMP_SIGHAND
../cmd/snap-seccomp/main.go:468:20: could not determine kind of name for C.KCMP_SYSVSEM
../cmd/snap-seccomp/main.go:463:20: could not determine kind of name for C.KCMP_VM

Thank you, it only happens in the "snap-build" test which is a bit special, let me see what is going on there.

cmd/snap-seccomp/main.go

mvo5 · 2023-05-26T15:50:14Z

Thanks! This looks great now and builds are passing

mvo5 · 2023-05-27T19:38:14Z

It looks like the 14.04 build is still not fully happy :-/

2023-05-27T19:29:43.6022695Z github.com/snapcore/snapd/cmd/snap-seccomp
2023-05-27T19:29:43.6022921Z # github.com/snapcore/snapd/cmd/snap-seccomp
2023-05-27T19:29:43.6023262Z ../cmd/snap-seccomp/main.go:477:20: could not determine kind of name for C.KCMP_EPOLL_TFD

cmd/snap-seccomp/main.go

For fixing https://bugs.launchpad.net/snapd/+bug/1998980 implement the suggested fix of allowing kcmp in the base template.

jhenstridge

Looks good!

alexmurray requested changes Mar 26, 2023

View reviewed changes

lissyx force-pushed the bug1800951_kcmp_zoom branch 2 times, most recently from a52325e to a7e5fe0 Compare March 27, 2023 14:26

pedronis changed the title ~~interfaces/seccomp/template: Adding kcmp to allow Mesa usecases~~ interfaces/seccomp/template: adding kcmp to allow Mesa usecases Mar 28, 2023

lissyx requested a review from alexmurray March 28, 2023 12:35

alexmurray approved these changes Mar 28, 2023

View reviewed changes

jhenstridge reviewed May 26, 2023

View reviewed changes

lissyx force-pushed the bug1800951_kcmp_zoom branch 2 times, most recently from 8b91d4b to ff7d138 Compare May 26, 2023 07:49

jhenstridge reviewed May 26, 2023

View reviewed changes

lissyx force-pushed the bug1800951_kcmp_zoom branch from ff7d138 to a500522 Compare May 26, 2023 08:17

mvo5 added this to the 2.59 milestone May 26, 2023

mvo5 approved these changes May 26, 2023

View reviewed changes

jhenstridge reviewed May 26, 2023

View reviewed changes

cmd/snap-seccomp/main.go Outdated Show resolved Hide resolved

lissyx force-pushed the bug1800951_kcmp_zoom branch from 297d4d4 to f376754 Compare May 26, 2023 12:04

jhenstridge reviewed May 28, 2023

View reviewed changes

cmd/snap-seccomp/main.go Outdated Show resolved Hide resolved

lissyx force-pushed the bug1800951_kcmp_zoom branch from 04ea404 to 936df6a Compare May 29, 2023 09:18

interfaces/seccomp/template: Adding kcmp to allow Mesa usecases

bfce6e9

For fixing https://bugs.launchpad.net/snapd/+bug/1998980 implement the suggested fix of allowing kcmp in the base template.

lissyx force-pushed the bug1800951_kcmp_zoom branch from 936df6a to bfce6e9 Compare May 29, 2023 09:22

jhenstridge approved these changes May 29, 2023

View reviewed changes

jhenstridge merged commit 91cff40 into snapcore:master May 30, 2023
48 of 50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interfaces/seccomp/template: adding kcmp to allow Mesa usecases #12673

interfaces/seccomp/template: adding kcmp to allow Mesa usecases #12673

lissyx commented Mar 24, 2023 •

edited by mvo5

alexmurray left a comment

lissyx commented Mar 27, 2023

alexmurray commented Mar 27, 2023

alexmurray left a comment

lissyx commented Mar 29, 2023

alexmurray commented Mar 30, 2023

lissyx commented Apr 6, 2023

alexmurray commented Apr 11, 2023

kenvandine commented May 25, 2023

jhenstridge left a comment

jhenstridge May 26, 2023

lissyx May 26, 2023 •

edited

lissyx May 26, 2023

jhenstridge May 26, 2023

lissyx May 26, 2023

jhenstridge May 26, 2023

codecov-commenter commented May 26, 2023 •

edited

mvo5 left a comment

lissyx commented May 26, 2023

mvo5 commented May 26, 2023

lissyx commented May 26, 2023

mvo5 commented May 26, 2023 •

edited

lissyx commented May 26, 2023

lissyx commented May 26, 2023

mvo5 commented May 26, 2023

mvo5 commented May 26, 2023

mvo5 commented May 27, 2023

jhenstridge left a comment

interfaces/seccomp/template: adding kcmp to allow Mesa usecases #12673

interfaces/seccomp/template: adding kcmp to allow Mesa usecases #12673

Conversation

lissyx commented Mar 24, 2023 • edited by mvo5

alexmurray left a comment

Choose a reason for hiding this comment

lissyx commented Mar 27, 2023

alexmurray commented Mar 27, 2023

alexmurray left a comment

Choose a reason for hiding this comment

lissyx commented Mar 29, 2023

alexmurray commented Mar 30, 2023

lissyx commented Apr 6, 2023

alexmurray commented Apr 11, 2023

kenvandine commented May 25, 2023

jhenstridge left a comment

Choose a reason for hiding this comment

jhenstridge May 26, 2023

Choose a reason for hiding this comment

lissyx May 26, 2023 • edited

Choose a reason for hiding this comment

lissyx May 26, 2023

Choose a reason for hiding this comment

jhenstridge May 26, 2023

Choose a reason for hiding this comment

lissyx May 26, 2023

Choose a reason for hiding this comment

jhenstridge May 26, 2023

Choose a reason for hiding this comment

codecov-commenter commented May 26, 2023 • edited

Codecov Report

mvo5 left a comment

Choose a reason for hiding this comment

lissyx commented May 26, 2023

mvo5 commented May 26, 2023

lissyx commented May 26, 2023

mvo5 commented May 26, 2023 • edited

lissyx commented May 26, 2023

lissyx commented May 26, 2023

mvo5 commented May 26, 2023

mvo5 commented May 26, 2023

mvo5 commented May 27, 2023

jhenstridge left a comment

Choose a reason for hiding this comment

lissyx commented Mar 24, 2023 •

edited by mvo5

lissyx May 26, 2023 •

edited

codecov-commenter commented May 26, 2023 •

edited

mvo5 commented May 26, 2023 •

edited