RFE: add support for comparisons against 32-bit arguments #384

jhenstridge · 2022-05-11T08:51:34Z

This is intended to fix #383.

The basic implementation strategy was:

add an SCMP_CMP_32BIT flag that can be bitwise ORed with the existing comparison operators, indicating that a 32-bit comparison is requested even on 64-bit platforms. This should be ABI backward compatible, and should also cleanly return an error should an application link against an old libseccomp.
extract the "per argument comparison" logic from _db_rule_gen_64 and _db_rule_gen_32 into _db_rule_gen_arg_64 and _db_rule_gen_arg_32 helper functions that share the same signature.
merge the two _db_rule_gen_* functions now that they are basically the same except for which helper they invoke in the loop.
have _db_rule_gen unconditionally generate 32-bit comparison tree nodes if the argument comparison had the SCMP_CMP_32BIT flag set.

coveralls · 2022-05-11T16:42:13Z

Coverage decreased (-0.1%) to 89.454% when pulling 518493f on jhenstridge:32-bit-comparisons into 72198d9 on seccomp:main.

jhenstridge · 2022-05-12T08:59:18Z

I think this is ready for review now. I've added tests to demonstrate that the 32-bit comparisons work and ignore data in the high bits, and also made sure rule_add() will return -EINVAL if unknown flags are set in the scmp_compare operation for better forward compatibility.

One thing I haven't done is change the SCMP_A?_32() macros to automatically set SCMP_CMP_32BIT in the comparison operation. I think it is probably what most people using those macros would actually want, but is a change in behaviour.

… comparison operations Signed-off-by: James Henstridge <[email protected]>

…nction Signed-off-by: James Henstridge <[email protected]>

Signed-off-by: James Henstridge <[email protected]>

…2BIT is set Signed-off-by: James Henstridge <[email protected]>

jhenstridge · 2022-05-18T10:08:36Z

I've rebased against main, and added a Python version of the new test. It all looked okay locally, but I hadn't built with --enable-python.

Signed-off-by: James Henstridge <[email protected]>

…n op Signed-off-by: James Henstridge <[email protected]>

Signed-off-by: James Henstridge <[email protected]>

pcmoore · 2022-05-19T22:28:46Z

I want to apologize @jhenstridge, you've put a lot of work in on this and we haven't had a chance to look at it yet. I just want to let you know that this PR hasn't gone unnoticed or been forgotten, we just happen to have a lot on our plates and this is not only tricky code but an API addition so it takes some time to review.

Thank you for your help!

drakenclimber

Thanks, @jhenstridge!

I looked through the changes pretty closely, and it looks solid to me. I took a couple notes along the way as I worked through the patches, and I'll leave them in there for now in case they help others, but they don't affect the overall outcome of the review. Thanks for the refactor of the _db_rule_gen_* logic into a single function.

As a small nitpick, I am used to more descriptive commit comments, but I honestly can't fault what you've added. They explained each commit.

It feels like too significant of a change to be part of v2.5.5, and thus I would lean toward it going in v2.6.0. I'm open to discussion to hear others' thoughts here though.

Thanks!

Acked-by: Tom Hromatka <[email protected]>

drakenclimber · 2022-05-20T16:53:45Z

src/db.c

@@ -1944,6 +1944,72 @@ static struct db_sys_list *_db_rule_gen_64(const struct arch_def *arch,
 return NULL;
 }

+static int _db_rule_gen_arg_32(struct db_sys_list *s_new,


Note to self as much as anything - I don't see this function having any code coverage even though it's getting called 500,000+ times.

https://coveralls.io/builds/49252361/source?filename=src%2Fdb.c#L1918

Hmm, I think something is broken for me and coveralls.io, I'm not able to see the marked-up source anymore ... :/

Nevermind, it looks like I just needed to re-auth to coveralls.io.

Back to the original comment ... yes, something is odd here, both the 64-bit and 32-bit variants of this function appear to have zero code coverage, which is not right ...

drakenclimber · 2022-05-20T17:04:27Z

src/db.c

@@ -1959,8 +2025,8 @@ static struct db_sys_list *_db_rule_gen_32(const struct arch_def *arch,
 unsigned int iter;
 struct db_sys_list *s_new;
 const struct db_api_arg *chain = rule->args;
- struct db_arg_chain_tree *c_iter = NULL, *c_prev = NULL;
- bool tf_flag;
+ struct db_arg_chain_tree *prev_nodes[4] = { NULL, };


Another note to self - the prev_nodes[] change will become important in later commits. This commit is (slightly) more than just a refactoring of code to a separate function.

The fixed 4 is very "magic" too, which makes me uneasy. At the very least that needs a comment (why 4?), but a much better approach would be a macro definition and and comment with the macro.

Okay, I now understand where the 4 comes from, and to be fair we have similar constants in the current _db_rule_gen_64() function, but the important difference is that in the current implementation the magic numbers are contained as local variables within the function, here they are part of the function's signature and are thus much more important.

I'm hoping that as I get farther into this review we can revisit how this code was extracted, because I'm not really liking this right now.

cyphar · 2022-05-23T02:15:16Z

I wonder if it would be possible (or even preferable) to merge this PR with the requirements of #310 and just have a set of MASKED_* comparators so that you can do any masked comparison you like? It would result in the same BPF output for 32-bit but would be more general?

jhenstridge · 2022-05-24T08:03:09Z

@cyphar: I think handling 32-bit arguments is important enough to have on its own, given how many syscalls have 32-bit arguments. And if libseccomp ever leans how to perform signed comparisons, the argument width will be important due to different positions for the sign bit. It also has the benefit of generating smaller BPF for 32-bit argument checks compared to generalised masked 64-bit comparisons.

If the part of my proposal to add flag bits to scmp_compare is accepted though, it might provide a better basis for generalised mask comparisons. Rather than adding new scmp_compare values for the other comparisons like was done with SCMP_CMP_MASKED_EQ, you could have a flag that can be ORed to indicate that the argument should be masked before performing the comparison.

pcmoore · 2022-05-27T21:29:01Z

include/seccomp.h.in

@@ -94,6 +94,9 @@ enum scmp_compare {
 SCMP_CMP_GT = 6, /**< greater than */
 SCMP_CMP_MASKED_EQ = 7, /**< masked equality */
 _SCMP_CMP_MAX,
+
+ SCMP_CMP_OPMASK = 0xFFFF,
+ SCMP_CMP_32BIT = 1 << 16, /**< operation is 32-bit */


I'm still working my way through this PR, but I think we want to move SCMP_CMP_OPMASK and SCMP_CMP_32BIT out of the enum and have them be standalone preprocessor macros/constants.

I guess if we do that we need to add a "filler" value to cause the enum to be a certain size, which feels a little icky but I think it's okay if we make sure to represent all of the flag space, e.g.

enum scmp_compare { _SCMP_CMP_MIN = 0, SCMP_CMP_NE = 1, SCMP_CMP_LT = 2, SCMP_CMP_LE = 3, SCMP_CMP_EQ = 4, SCMP_CMP_GE = 5, SCMP_CMP_GT = 6, SCMP_CMP_MASKED_EQ = 7, _SCMP_CMP_MAX, _SCMP_CMP_OPMASK = 0x0000FFFF, _SCMP_CMP_FLAGMASK = 0xFFFF0000, };

... which somewhat questions the value of moving the flag definitions out of the enum and into their own macros/constants, but that still seems like a goodish idea to me at this point.

pcmoore · 2022-05-27T21:49:37Z

src/db.h

@@ -38,6 +38,7 @@ struct db_api_arg {
 scmp_datum_t datum;

 bool valid;
+ bool is_32bit;


I think I'd prefer to drop the is_32bit flag and just preserve the SCMP_CMP_XXX flags in the db_api_arg::op field; there aren't that many places that check the comparison op so masking them shouldn't be too terrible. If necessary we can always wrap it in a preprocessor macro, e.g.

#define CMP_OP(x) (x & __SCMP_CMP_OPMASK) #define CMP_FLAGS(x) (x & __SCMP_CMP_FLAGMASK)

pcmoore · 2022-05-27T21:51:01Z

My apologies @jhenstridge, I just got pinged that I've run out of time today, if I don't get a chance to finish this over the weekend I'll pick it up next week.

pcmoore · 2022-07-11T02:03:25Z

src/db.c

+ if (*tf_flag)
+ prev_nodes[iter]->nxt_t = _db_node_get(c_iter[0]);
+ else
+ prev_nodes[iter]->nxt_f = _db_node_get(c_iter[0]);


I know braces ("{ ... }") aren't strictly required for this for-loop body, but please add them to help readability and prevent silly mistakes in the future.

pcmoore · 2022-07-11T02:26:20Z

A quick comment independent of any one particular commit - all of the commits in this PR appear pretty substantial, yet there are no commit descriptions in the first few commits I've reviewed. Please add some meaningful commit descriptions to each patch explaining what the patch does, and why this change is important.

We often let trivial patches go into the tree without commit descriptions, but that should be seen as a special case and not something that should be taken as a general rule.

pcmoore · 2022-07-11T02:29:07Z

src/db.c

@@ -1944,6 +1944,72 @@ static struct db_sys_list *_db_rule_gen_64(const struct arch_def *arch,
 return NULL;
 }

+static int _db_rule_gen_arg_32(struct db_sys_list *s_new,


This function also needs a comment block at the top like all the other functions in libseccomp.

Same as with the 64-bit variant, it looks like the comment block is added in 0936a72 ("db: merge _db_rule_gen_64 and _db_rule_gen_32"); it would be best to add the comment block in the first appearance of the function.

pcmoore · 2022-07-11T02:32:06Z

src/db.c

+static int _db_rule_gen_arg_32(struct db_sys_list *s_new,
+ const struct arch_def *arch,
+ const struct db_api_arg *api_arg,
+ struct db_arg_chain_tree *prev_nodes[4],


I'm thinking out-loud here, but I wonder if the readability would be better if the prev_nodes[] array was expanded into individual variables with more descriptive names?

Also, this function only appears to ever use one prev_nodes[] array slot, right?

pcmoore · 2022-07-11T02:35:42Z

src/db.c

- */
-static struct db_sys_list *_db_rule_gen_64(const struct arch_def *arch,
- const struct db_api_rule_list *rule)
+static int _db_rule_gen_arg_64(struct db_sys_list *s_new,


Needs a comment block describing the function, see the others for an example.

Okay, so it looks like it is added in 0936a72 ("db: merge _db_rule_gen_64 and _db_rule_gen_32"); it would be best to add the comment block in the first appearance of the function.

pcmoore · 2022-07-11T02:36:02Z

src/db.c

+static int _db_rule_gen_arg_64(struct db_sys_list *s_new,
+ const struct arch_def *arch,
+ const struct db_api_arg *api_arg,
+ struct db_arg_chain_tree *prev_nodes[4],


The other comments about prev_nodes[] apply here.

pcmoore · 2022-07-11T02:36:50Z

src/db.c

+ if (*tf_flag)
+ (*node)->nxt_t = _db_node_get(c_iter);
+ else
+ (*node)->nxt_f = _db_node_get(c_iter);


I know braces ("{ ... }") aren't strictly required for this for-loop body, but please add them to help readability and prevent silly mistakes in the future.

pcmoore · 2022-07-11T02:37:34Z

src/db.c

+ prev_nodes[iter]->nxt_t = _db_node_get(c_iter[0]);
+ else
+ prev_nodes[iter]->nxt_f = _db_node_get(c_iter[0]);
+ prev_nodes[0] = prev_nodes[1] = prev_nodes[2] = prev_nodes[3] = NULL;


Personal nit-pick: please don't chain assignments together like this, put them each on their own line.

pcmoore · 2022-07-12T20:49:41Z

src/db.c

 {
 unsigned int iter;
 struct db_sys_list *s_new;
 const struct db_api_arg *chain = rule->args;
 struct db_arg_chain_tree *prev_nodes[4] = { NULL, };
+ bool is_32bit = arch->size == ARCH_SIZE_32;


Since is_32bit is only used once below, why not just drop the dedicated local variable and just do the arch->size comparison where needed?

pcmoore · 2022-07-12T20:52:31Z

tests/Makefile.am

@@ -94,7 +94,8 @@ check_PROGRAMS = \
 56-basic-iterate_syscalls \
 57-basic-rawsysrc \
 58-live-tsync_notify \
- 59-basic-empty_binary_tree
+ 59-basic-empty_binary_tree \
+ 60-sim-32b_args_on_64b


Thank you for the new tests :)

pcmoore · 2022-07-12T20:55:12Z

src/db.c

@@ -2331,6 +2331,11 @@ int db_col_rule_add(struct db_filter_col *col,
 rc = -EINVAL;
 goto add_return;
 }
+ /* Check that no unknown flags are specified in the op */


It might be a good idea to check this up near the top of the if-block once the once the chain entry is considered valid (the fail early idea).

pcmoore · 2022-07-12T20:56:21Z

src/db.c

@@ -2331,6 +2331,11 @@ int db_col_rule_add(struct db_filter_col *col,
 rc = -EINVAL;
 goto add_return;
 }
+ /* Check that no unknown flags are specified in the op */
+ if ((arg_data.op & ~(SCMP_CMP_OPMASK | SCMP_CMP_32BIT)) != 0) {


This may argue for a SCMP_CMP_FLAGMASK or similar, see the comments on the first patch in the PR.

pcmoore · 2022-07-12T20:57:03Z

tests/Makefile.am

@@ -155,7 +155,8 @@ EXTRA_DIST_TESTPYTHON = \
 56-basic-iterate_syscalls.py \
 57-basic-rawsysrc.py \
 58-live-tsync_notify.py \
- 59-basic-empty_binary_tree.py
+ 59-basic-empty_binary_tree.py \
+ 60-sim-32b_args_on_64b.tests.py


Double bonus points for the Python tests :)

pcmoore · 2022-07-12T21:01:21Z

First off, I'm really sorry @jhenstridge that it took me so long to give this a proper review, that was pretty crappy and you deserved better considering the time and effort that no doubt went into this PR. That said, I'm finally done going through the PR, but please don't feel that my comments are "you MUST do this!", I think most of my comments were intended more as starting points for some discussion between you, @drakenclimber, myself, and anyone else who is interested in this functionality. Put another way, please don't rush out to change a lot of your code just yet, let's make sure we are all in agreement before you spend a bunch more time revising this patchset :)

pcmoore · 2022-10-14T19:11:19Z

Hi @jhenstridge, it's been a while so I just wanted to check in on this to see where things were at?

jhenstridge force-pushed the 32-bit-comparisons branch from 516fb01 to f5fef33 Compare May 12, 2022 08:46

jhenstridge marked this pull request as ready for review May 12, 2022 08:59

pcmoore added enhancement priority/medium pending/review labels May 12, 2022

jhenstridge added 5 commits May 18, 2022 17:57

api: Add a SCMP_CMP_32BIT flag bit that can be combined with existing…

ad65b76

… comparison operations Signed-off-by: James Henstridge <[email protected]>

db: extract per db_api_arg portion of _db_rule_gen_32 into its own fu…

b6b2e21

…nction Signed-off-by: James Henstridge <[email protected]>

db: extract 64-bit comparison generation into _db_rule_gen_arg_64

9090c42

Signed-off-by: James Henstridge <[email protected]>

db: merge _db_rule_gen_64 and _db_rule_gen_32

0936a72

Signed-off-by: James Henstridge <[email protected]>

db: generate 32-bit comparisons on 64-bit architectures if SCMP_CMP_3…

b86348b

…2BIT is set Signed-off-by: James Henstridge <[email protected]>

jhenstridge force-pushed the 32-bit-comparisons branch from f5fef33 to 7415076 Compare May 18, 2022 10:00

jhenstridge added 3 commits May 18, 2022 18:10

tests: add a test covering forced 32-bit comparisons

3a13808

Signed-off-by: James Henstridge <[email protected]>

db: return EINVAL if there are any unknown flags set in the compariso…

d472727

…n op Signed-off-by: James Henstridge <[email protected]>

tests: add Python version of 32-bit comparison test program

518493f

Signed-off-by: James Henstridge <[email protected]>

jhenstridge force-pushed the 32-bit-comparisons branch from 7415076 to 518493f Compare May 18, 2022 10:11

drakenclimber approved these changes May 20, 2022

View reviewed changes

drakenclimber requested a review from pcmoore May 20, 2022 17:33

pcmoore reviewed May 27, 2022

View reviewed changes

pcmoore added this to the v2.6.0 milestone Jul 11, 2022

pcmoore reviewed Jul 11, 2022

View reviewed changes

pcmoore reviewed Jul 12, 2022

View reviewed changes

pcmoore removed the pending/review label Jul 12, 2022

pcmoore added the pending/revision label Sep 20, 2022

pcmoore modified the milestones: v2.6.0, v2.7.0 May 30, 2024

RFE: add support for comparisons against 32-bit arguments #384

Are you sure you want to change the base?

RFE: add support for comparisons against 32-bit arguments #384

Conversation

jhenstridge commented May 11, 2022 • edited Loading

coveralls commented May 11, 2022 • edited Loading

jhenstridge commented May 12, 2022

jhenstridge commented May 18, 2022

pcmoore commented May 19, 2022

drakenclimber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoore Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoore Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

cyphar commented May 23, 2022

jhenstridge commented May 24, 2022

Choose a reason for hiding this comment

pcmoore May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoore commented May 27, 2022

Choose a reason for hiding this comment

pcmoore commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoore commented Jul 12, 2022

pcmoore commented Oct 14, 2022

jhenstridge commented May 11, 2022 •

edited

Loading

coveralls commented May 11, 2022 •

edited

Loading

pcmoore Jul 11, 2022 •

edited

Loading

pcmoore Jul 11, 2022 •

edited

Loading

pcmoore May 27, 2022 •

edited

Loading