RFE: add support for comparisons against 32-bit arguments

[jhenstridge]

This is intended to fix #383.

The basic implementation strategy was:

1. 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.
2. 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.
3. merge the two _db_rule_gen_* functions now that they are basically the same except for which helper they invoke in the loop.
4. have _db_rule_gen unconditionally generate 32-bit comparison tree nodes if the argument comparison had the SCMP_CMP_32BIT flag set.

[coveralls]

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

[jhenstridge]

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.

[jhenstridge]

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.

[pcmoore]

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!

[cyphar]

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]

@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]

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]

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]

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]

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