RFE: add new API to enable addfd support for seccomp notifier

[realsdx]

This PR adds a new API to enable addfd support for seccomp notifiers. It introduces changes to the seccomp_notif and seccomp_notif_resp structures to handle the addfd operation, allowing file descriptors to be passed through a seccomp notifier.

It also adds supporting man page entry and unit regression tests for both C and Python APIs.

[pcmoore]

Hi @realsdx, thanks for the PR :)

Unfortunately, it looks like the the CI test are failing because of a missing seccomp_notif_addfd struct declaration due to an older userspace/headers on the build/CI system. Look at how we handle seccomp_notif and seccomp_notif_resp in the "include/seccomp.h.in" file for an example on how to fix this:

https://github.com/seccomp/libseccomp/blob/7db46d72f13c172b290818f624c2966bd0db5677/include/seccomp.h.in#L391-L408

[pcmoore]

FWIW, if I made the following changes on top of your PR I was able to get everything building:

diff --git a/include/seccomp.h.in b/include/seccomp.h.in
index 024aca537f64..568eabaad949 100644
--- a/include/seccomp.h.in
+++ b/include/seccomp.h.in
@@ -406,6 +406,11 @@ struct seccomp_notif_resp {
        __u32 flags;
 };
 
+#endif
+
+/* seccomp_notif_addfd and ADDFD_FLAG_SETFD was added in kernel v5.10 */
+#ifndef SECCOMP_ADDFD_FLAG_SETFD
+#define SECCOMP_ADDFD_FLAG_SETFD       (1UL << 0)
 struct seccomp_notif_addfd {
        __u64 id;
        __u32 flags;
diff --git a/src/system.h b/src/system.h
index 495050fb2e75..87bf8a6e832d 100644
--- a/src/system.h
+++ b/src/system.h
@@ -180,14 +180,6 @@ struct seccomp_notif_resp {
        __u32 flags;
 };
 
-struct seccomp_notif_addfd {
-       __u64 id;
-       __u32 flags;
-       __u32 srcfd;
-       __u32 newfd;
-       __u32 newfd_flags;
-};
-
 #define SECCOMP_IOC_MAGIC               '!'
 #define SECCOMP_IO(nr)                  _IO(SECCOMP_IOC_MAGIC, nr)
 #define SECCOMP_IOR(nr, type)           _IOR(SECCOMP_IOC_MAGIC, nr, type)
@@ -201,6 +193,24 @@ struct seccomp_notif_addfd {
 #define SECCOMP_IOCTL_NOTIF_ID_VALID    SECCOMP_IOW(2, __u64)
 #endif /* SECCOMP_RET_USER_NOTIF */
 
+/* seccomp_notif_addfd and ADDFD_FLAG_SETFD was added in kernel v5.10 */
+#ifndef SECCOMP_ADDFD_FLAG_SETFD
+#define SECCOMP_ADDFD_FLAG_SETFD       (1UL << 0)
+struct seccomp_notif_addfd {
+       __u64 id;
+       __u32 flags;
+       __u32 srcfd;
+       __u32 newfd;
+       __u32 newfd_flags;
+};
+#endif
+
+/* SECCOMP_IOCTL_NOTIF_ADDFD was added in kernel v5.10 */
+#ifndef SECCOMP_IOCTL_NOTIF_ADDFD
+#define SECCOMP_IOCTL_NOTIF_ADDFD      SECCOMP_IOW(3, \
+                                                   struct seccomp_notif_addfd)
+#endif
+
 /* non-public ioctl number for backwards compat (see system.c) */
 #define SECCOMP_IOCTL_NOTIF_ID_VALID_WRONG_DIR SECCOMP_IOR(2, __u64)

[pcmoore]

Another quick comment, we'll likely want to add a new API level to seccomp_api_get() to indicate ADDFD support.

[realsdx]

Another quick comment, we'll likely want to add a new API level to seccomp_api_get() to indicate ADDFD support.

The current API levels are based on different Seccomp actions and filter flags. If we add a new level for addfd support, it would be based on IOCTL support.

What would be the best way to determine the addfd support at runtime? One issue with probing the IOCTL with notifyfd is that we need a valid notify fd first. Since we cannot uninstall the seccomp filter once it's loaded, is it a good idea to probe for ADDFD IOCTL?

[coveralls]

coverage: 88.845% (-0.2%) from 89.046% when pulling 805517e704b35310d64961f7ef2a637277eefced on realsdx:dev/addfd into 9b9ea8e7a173b96e59fb21e8d461365110e7b4ef on seccomp:main.

[pcmoore]

What would be the best way to determine the addfd support at runtime? One issue with probing the IOCTL with notifyfd is that we need a valid notify fd first. Since we cannot uninstall the seccomp filter once it's loaded, is it a good idea to probe for ADDFD IOCTL?

My apologies, I suspect I was stuck in the thinking that we were doing this through the seccomp() syscall somehow ... but yes, I see what you mean about the ioctl issue. I'm looking at the associated kernel code to see if there something in there for us to use to query for the functionality without actually doing anything, and nothing is jumping out at me. The closest I can think of would be to pass a bogus file descriptor and see if -EBADF is returned; if the kernel support is missing we should see -EINVAL. However, I'm not sure we want to risk doing that in a library, that seems like a Bad Thing.

@drakenclimber do you have any thoughts on this?

[pcmoore]

Okay, I'm not seeing any good ideas around the runtime detection so let's just go ahead and skip that. Let me take a closer look at the rest of the PR ...

[pcmoore]

@realsdx mentioned SECCOMP_ADDFD_FLAG_SEND in a separate chat, and I think it might be a nice thing to add both SECCOMP_ADDFD_FLAG_SETFD and SECCOMP_ADDFD_FLAG_SEND to this PR, thoughts?

For reference, SECCOMP_ADDFD_FLAG_SEND injects a file descriptor into the target's file descriptor table while also atomically clearing the target's errno and setting the syscall return value equal to the value of the newly injected file descriptor. More information can be found in the manpage.

[pcmoore]

This is looking good @realsdx, did you want to do another push to this PR branch with all of the changes discussed and remove the draft flag on this PR?

[pcmoore]

@drakenclimber aside from the two things mentioned above, what do you think of this?

[drakenclimber]

@drakenclimber aside from the two things mentioned above, what do you think of this?

I'll look into it this week. Thanks for reminding me :)

[drakenclimber]

What would be the best way to determine the addfd support at runtime? One issue with probing the IOCTL with notifyfd is that we need a valid notify fd first. Since we cannot uninstall the seccomp filter once it's loaded, is it a good idea to probe for ADDFD IOCTL?

My apologies, I suspect I was stuck in the thinking that we were doing this through the seccomp() syscall somehow ... but yes, I see what you mean about the ioctl issue. I'm looking at the associated kernel code to see if there something in there for us to use to query for the functionality without actually doing anything, and nothing is jumping out at me. The closest I can think of would be to pass a bogus file descriptor and see if -EBADF is returned; if the kernel support is missing we should see -EINVAL. However, I'm not sure we want to risk doing that in a library, that seems like a Bad Thing.

@drakenclimber do you have any thoughts on this?

I don't have any better ideas here either. Bummer.

[realsdx]

First of all, apologies for the delay. I was on a long vacation, and then some work piled up. I should’ve replied much sooner! Thanks a lot for the thorough review. I definitely made a few silly mistakes and some poor choices, really appreciate you pointing those out.

I’ve made several updates to the C test code (refactored the parent and child functions) and cleaned up some of the debug code. A couple of things are still pending:

• Refactoring the changes into 3 commits, as suggested by @drakenclimber
• Fixing the duplicate macros in the system.h file

I still have a few doubts regarding the duplicate macros @pcmoore

[pcmoore]

I still have a few doubts regarding the duplicate macros @pcmoore

I think I just answered those in the code review/comment section, but please let me know if that still doesn't make sense.

[realsdx]

Thank you, @pcmoore, for the explanation. It's much clearer to me now. I'll update the code with the remaining changes.