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

RFE: add transaction support to the libseccomp API #415

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 134 additions & 0 deletions doc/man/man3/seccomp_transaction_start.3
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
.TH "seccomp_transaction_start" 3 "21 September 2023" "[email protected]" "libseccomp Documentation"
.\" //////////////////////////////////////////////////////////////////////////
.SH NAME
.\" //////////////////////////////////////////////////////////////////////////
seccomp_transaction_start, seccomp_transaction_commit, seccomp_transaction_reject \- Manage seccomp filter transactions
.\" //////////////////////////////////////////////////////////////////////////
.SH SYNOPSIS
.\" //////////////////////////////////////////////////////////////////////////
.nf
.B #include <seccomp.h>
.sp
.B typedef void * scmp_filter_ctx;
.sp
.BI "int seccomp_transaction_start(scmp_filter_ctx " ctx ");
.BI "int seccomp_transaction_commit(scmp_filter_ctx " ctx ");
.BI "void seccomp_transaction_reject(scmp_filter_ctx " ctx ");
.sp
Link with \fI\-lseccomp\fP.
.fi
.\" //////////////////////////////////////////////////////////////////////////
.SH DESCRIPTION
.\" //////////////////////////////////////////////////////////////////////////
.P
The
.BR seccomp_transaction_start ()
function starts a new seccomp filter
transaction that the caller can use to perform any number of filter
modifications which can then be committed to the filter using
.BR seccomp_transaction_commit ()
or rejected using
.BR seccomp_transaction_reject ().
It is important to note that transactions only affect the seccomp filter state
while it is being managed by libseccomp; seccomp filters which have been loaded
into the kernel can not be modified, only new seccomp filters can be added on
top of the existing loaded filter stack.
.P
Finishing, or committing, a transaction is optional, although it is encouraged.
Copy link
Member

Choose a reason for hiding this comment

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

I like this code block as it clearly explains why a user would be interested in this feature

Copy link
Member

Choose a reason for hiding this comment

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

As you have outlined in issue #181, this should greatly help in testing transactions 👍. Do you have a use case in mind for libseccomp's users (like systemd, etc.)?

Copy link
Member Author

Choose a reason for hiding this comment

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

No one particular use case in mind, I'll leave that up to more clever people than I :)

At any point in time, regardless of the transaction state, the seccomp filter
is determined by all of the libseccomp operations performed on the filter up to
that point. Committing a transaction simply flushes the transaction rollback
marker of the current transaction making the filter changes permanent;
rejecting a transaction rolls the filter state back to immediately before the
transaction was started.
.P
Transactions can be nested arbitrarily deep with the
.BR seccomp_transaction_commit ()
and
.BR seccomp_transaction_reject ()
functions always operating on the deepest, or more recently started transaction.
A nested set of filter modifications, even if committed, is still subject to
rejection by shallower, or older transactions that have yet to be committed or
rejected.
.\" //////////////////////////////////////////////////////////////////////////
.SH RETURN VALUE
.\" //////////////////////////////////////////////////////////////////////////
The
.BR seccomp_transaction_start ()
and
.BR seccomp_transaction_commit ()
functions return zero on success or one of the following error codes on
failure:
.TP
.B -ENOMEM
The library was unable to allocate enough memory.
.\" //////////////////////////////////////////////////////////////////////////
.SH EXAMPLES
.\" //////////////////////////////////////////////////////////////////////////
.nf
#include <seccomp.h>

int libseccomp_generate(scmp_filter_ctx *ctx)
{
int rc;

rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(close), 0);
if (rc)
return rc;
rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(exit_group), 0);
if (rc)
return rc;
rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(exit), 0);
if (rc)
return rc;

return 0;
}

int main(int argc, char *argv[])
{
int rc = \-1;
scmp_filter_ctx ctx;

ctx = seccomp_init(SCMP_ACT_KILL);
if (ctx == NULL)
goto out;

rc = seccomp_transaction_start(ctx)
if (rc)
goto out;
rc = libseccomp_generate(ctx);
if (rc == 0) {
rc = seccomp_transaction_commit(ctx);
if (rc)
goto out;
} else
seccomp_transaction_reject(ctx);

/* ... */

out:
seccomp_release(ctx);
return \-rc;
}
.fi
.\" //////////////////////////////////////////////////////////////////////////
.SH NOTES
.\" //////////////////////////////////////////////////////////////////////////
.P
While the seccomp filter can be generated independent of the kernel, kernel
support is required to load and enforce the seccomp filter generated by
libseccomp.
.P
The libseccomp project site, with more information and the source code
repository, can be found at https://github.com/seccomp/libseccomp. This tool,
as well as the libseccomp library, is currently under development, please
report any bugs at the project site or directly to the author.
.\" //////////////////////////////////////////////////////////////////////////
.SH AUTHOR
.\" //////////////////////////////////////////////////////////////////////////
Paul Moore <[email protected]>
.\" //////////////////////////////////////////////////////////////////////////
.SH SEE ALSO
.\" //////////////////////////////////////////////////////////////////////////
.BR seccomp_init (3),
29 changes: 29 additions & 0 deletions include/seccomp.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,35 @@ int seccomp_export_bpf(const scmp_filter_ctx ctx, int fd);
*/
int seccomp_export_bpf_mem(const scmp_filter_ctx ctx, void *buf, size_t *len);

/**
* Start a filter transaction
* @param ctx the filter context
*
* This function starts a filter transaction for modifying the seccomp filter.
* Returns zero on success, negative values on failure.
*
*/
int seccomp_transaction_start(const scmp_filter_ctx ctx);

/**
* Reject the current filter transaction
* @param ctx the filter context
*
* This function rejects the current seccomp filter transaction.
*
*/
void seccomp_transaction_reject(const scmp_filter_ctx ctx);
Copy link
Member

Choose a reason for hiding this comment

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

Would we ever want a return value here? I can't think of one, but as you mentioned in the cover letter, the API is set in stone once we release this :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm struggling to think of one too, what do you think, should we return an int?


/**
* Commit the current filter transaction
* @param ctx the filter context
*
* This function commits the current seccomp filter transaction. Returns zero
* on success, negative values on failure.
*
*/
int seccomp_transaction_commit(const scmp_filter_ctx ctx);

/**
* Precompute the seccomp filter for future use
* @param ctx the filter context
Expand Down
39 changes: 39 additions & 0 deletions src/api.c
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,45 @@ API int seccomp_export_bpf_mem(const scmp_filter_ctx ctx, void *buf,
return rc;
}

/* NOTE - function header comment in include/seccomp.h */
API int seccomp_transaction_start(const scmp_filter_ctx ctx)
{
int rc;
struct db_filter_col *col;

if (_ctx_valid(ctx))
return _rc_filter(-EINVAL);
col = (struct db_filter_col *)ctx;

rc = db_col_transaction_start(col, true);
return _rc_filter(rc);
}

/* NOTE - function header comment in include/seccomp.h */
API void seccomp_transaction_reject(const scmp_filter_ctx ctx)
{
struct db_filter_col *col;

if (_ctx_valid(ctx))
return;
col = (struct db_filter_col *)ctx;

db_col_transaction_abort(col, true);
}

/* NOTE - function header comment in include/seccomp.h */
API int seccomp_transaction_commit(const scmp_filter_ctx ctx)
{
struct db_filter_col *col;

if (_ctx_valid(ctx))
return _rc_filter(-EINVAL);
col = (struct db_filter_col *)ctx;

db_col_transaction_commit(col, true);
return _rc_filter(0);
}

/* NOTE - function header comment in include/seccomp.h */
API int seccomp_precompute(const scmp_filter_ctx ctx)
{
Expand Down
47 changes: 34 additions & 13 deletions src/db.c
Original file line number Diff line number Diff line change
Expand Up @@ -2369,7 +2369,7 @@ int db_col_rule_add(struct db_filter_col *col,
}

/* create a checkpoint */
rc = db_col_transaction_start(col);
rc = db_col_transaction_start(col, false);
if (rc != 0)
goto add_return;

Expand All @@ -2396,9 +2396,9 @@ int db_col_rule_add(struct db_filter_col *col,

/* commit the transaction or abort */
if (rc == 0)
db_col_transaction_commit(col);
db_col_transaction_commit(col, false);
else
db_col_transaction_abort(col);
db_col_transaction_abort(col, false);

add_return:
/* update the misc state */
Expand All @@ -2415,12 +2415,13 @@ int db_col_rule_add(struct db_filter_col *col,
/**
* Start a new seccomp filter transaction
* @param col the filter collection
* @param user true if initiated by a user
*
* This function starts a new seccomp filter transaction for the given filter
* collection. Returns zero on success, negative values on failure.
*
*/
int db_col_transaction_start(struct db_filter_col *col)
int db_col_transaction_start(struct db_filter_col *col, bool user)
{
int rc;
unsigned int iter;
Expand All @@ -2439,6 +2440,7 @@ int db_col_transaction_start(struct db_filter_col *col)
* transaction is current/correct */

col->snapshots->shadow = false;
col->snapshots->user = user;
return 0;
}

Expand Down Expand Up @@ -2486,6 +2488,8 @@ int db_col_transaction_start(struct db_filter_col *col)
} while (rule_o != filter_o->rules);
}

snap->user = user;

/* add the snapshot to the list */
snap->next = col->snapshots;
col->snapshots = snap;
Expand All @@ -2502,23 +2506,35 @@ int db_col_transaction_start(struct db_filter_col *col)
/**
* Abort the top most seccomp filter transaction
* @param col the filter collection
* @param user true if initiated by a user
*
* This function aborts the most recent seccomp filter transaction.
*
*/
void db_col_transaction_abort(struct db_filter_col *col)
void db_col_transaction_abort(struct db_filter_col *col, bool user)
{
int iter;
unsigned int filter_cnt;
struct db_filter **filters;
struct db_filter_snap *snap;

if (col->snapshots == NULL)
snap = col->snapshots;
if (snap == NULL)
return;

/* replace the current filter with the last snapshot, skipping shadow
* snapshots are they are duplicates of the current snapshot */
if (snap->shadow) {
struct db_filter_snap *tmp = snap;
snap = snap->next;
_db_snap_release(tmp);
}

if (snap->user != user)
return;

/* replace the current filter with the last snapshot */
snap = col->snapshots;
col->snapshots = snap->next;

filter_cnt = col->filter_cnt;
filters = col->filters;
col->filter_cnt = snap->filter_cnt;
Expand All @@ -2537,13 +2553,14 @@ void db_col_transaction_abort(struct db_filter_col *col)
/**
* Commit the top most seccomp filter transaction
* @param col the filter collection
* @param user true if initiated by a user
*
* This function commits the most recent seccomp filter transaction and
* attempts to create a shadow transaction that is a duplicate of the current
* filter to speed up future transactions.
*
*/
void db_col_transaction_commit(struct db_filter_col *col)
void db_col_transaction_commit(struct db_filter_col *col, bool user)
{
int rc;
unsigned int iter;
Expand All @@ -2559,12 +2576,16 @@ void db_col_transaction_commit(struct db_filter_col *col)
if (snap->shadow) {
/* leave the shadow intact, but drop the next snapshot */
Copy link
Member

Choose a reason for hiding this comment

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

Is this block of code accessible? I hacked around for just a bit and was unable to hit it.

snap->shadow is effectively always set to false at the start of db_col_transaction_start(). snap->shadow is only set true at the end of db_col_transaction_commit() - approximately 100 lines after this. Is there a code path where snap->shadow is true at this point?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, it looks like I never responded back when I "fixed" this, so I'm sorta going by memory/comments/self-code-review and I believe the answer that yes, you can hit this code when you start nesting transactions.

if (snap->next) {
snap->next = snap->next->next;
_db_snap_release(snap->next);
struct db_filter_snap *tmp = snap->next;
snap->next = tmp->next;
_db_snap_release(tmp);
}
return;
}
Copy link
Member

@drakenclimber drakenclimber Sep 27, 2023

Choose a reason for hiding this comment

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

(This comment is for a couple lines down, but Github won't let me comment there. Specifically this line: https://github.com/pcmoore/misc-libseccomp/blob/working-transactions/src/db.c#L2589)

Since we had issues handling aborts, I wrote a quick test for this large block of code where the col's filter count didn't match the snap's filter count.

Here's the test I threw together:
drakenclimber@3814774

And here's the code coverage (prior to this test these lines weren't being hit):
https://coveralls.io/builds/62918086/source?filename=src%2Fdb.c#L2589

Interestingly this leads to a valgrind failure, but I haven't looked into it yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the test! I've included it in my dev branch (which I'm going to refresh here in a few minutes) and used it to fix a number of bugs.


if (snap->user != user)
return;

/* adjust the number of filters if needed */
if (col->filter_cnt > snap->filter_cnt) {
unsigned int tmp_i;
Expand Down Expand Up @@ -2593,8 +2614,8 @@ void db_col_transaction_commit(struct db_filter_col *col)
* at the cost of a not reaping all the memory possible */

do {
_db_release(snap->filters[snap->filter_cnt--]);
} while (snap->filter_cnt > col->filter_cnt);
_db_release(snap->filters[--snap->filter_cnt]);
} while (col->filter_cnt < snap->filter_cnt);
}

/* loop through each filter and update the rules on the snapshot */
Expand Down
7 changes: 4 additions & 3 deletions src/db.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ struct db_filter_snap {
struct db_filter **filters;
unsigned int filter_cnt;
bool shadow;
bool user;
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking a bool should meet our needs initially, but I'm wondering if long-term we'll want to switch to a transaction number (with a special bit for user-initiated transactions). I'm afraid the bool check of if snap->user != user may not be powerful enough someday.

The good thing is none of this exposed by the API, so we could change it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, let's leave it as a bool now, mostly for simplicity; we can change it later if needed.


struct db_filter_snap *next;
};
Expand Down Expand Up @@ -215,9 +216,9 @@ int db_col_rule_add(struct db_filter_col *col,
int db_col_syscall_priority(struct db_filter_col *col,
int syscall, uint8_t priority);

int db_col_transaction_start(struct db_filter_col *col);
void db_col_transaction_abort(struct db_filter_col *col);
void db_col_transaction_commit(struct db_filter_col *col);
int db_col_transaction_start(struct db_filter_col *col, bool user);
void db_col_transaction_abort(struct db_filter_col *col, bool user);
void db_col_transaction_commit(struct db_filter_col *col, bool user);

int db_col_precompute(struct db_filter_col *col);
void db_col_precompute_reset(struct db_filter_col *col);
Expand Down
4 changes: 4 additions & 0 deletions src/python/libseccomp.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ cdef extern from "seccomp.h":
int seccomp_export_bpf_mem(const scmp_filter_ctx ctx, void *buf,
size_t *len)

int seccomp_transaction_start(const scmp_filter_ctx ctx)
void seccomp_transaction_reject(const scmp_filter_ctx ctx)
int seccomp_transaction_commit(const scmp_filter_ctx ctx)

int seccomp_precompute(const scmp_filter_ctx ctx)

# kate: syntax python;
Expand Down
Loading
Loading