Capsicum-related fixes for tcpdump #687
Conversation
|
It would not harm to have some comments in the commit messages or the source code to explain these changes to the folks in future. |
|
Are you referring to both commits? |
|
Well, yes. |
|
Better now? |
|
Looks much more comprehensible to me now, thank you. |
|
So there are no backends that 1) do have a file descriptor but 2) don't support Capsicum? For example, does the Netmap support in Or is it a case of "support Capsicum" really meaning that, to support Capsicum with a given device, you have to enable the particular ioctls used by the device, in which case, to fix this, there will have to be support in libpcap (as that's the only place that knows which ioctls have to be enabled)? |
|
For example libibverbs have a bunch of file descriptors associated with the data transport object. None of these are registered by libpcap, so at the moment we enter sandboxing, these transport objects stop working. From what I understand, the file handle in question is only used for BPF, Berkeley Packet Filter, compatible file handles. If it is -1 it means BPF is not in use. I'll have to dig a bit deeper in the code to verify this. Basically these bits been tested under FreeBSD and verified. And there is also an identical review pending with some more information: |
That's one particular example; you need to look at all the backends that will run on OSes that support Capsicum (is it FreeBSD-only, or do any other OSes that support libpcap support it?) to determine whether this change is sufficient.
Are there libibverbs call to fetch the descriptors from the various handles
Not true - perhaps, in my previous comment, I should have explicitly indicated that the file handle is set by the Netmap backend, and therefore that
"The return value of "BPF is not in use", however, does not imply "the return value of |
This is out of the scope of my patch. Do you believe there is any reason my patch will break anything? |
If you understand the code better than I do, maybe you can help correct my patch. |
This is not possible currently, so capsicum + libibverbs is a no-go. |
|
Ping - can these patches be pushed? |
|
Please allow some more time for a more thorough review. |
Do you have a rough guess of the timeline? We're approaching code freeze for FreeBSD 12 and would prefer to include a change identical to upstream, but if it will be a while yet we'll incorporate a local patch and revert to upstream later. |
|
To answer one of the Guy's questions: Capsicum project page says it is supported on FreeBSD, Linux and DragonFlyBSD, if that helps. |
|
Ping - any updates? |
|
FYI: Submitted to FreeBSD: |
|
Sorry this is taking so long. |
Done. |
Not all libpcap backends use the BPF compatible set of IOCTLs. For example the mlx5 backend uses libibverbs which is currently not capsicum compatible. Disable sandboxing for such backends. Completes commit 3e26499 Signed-off-by: Hans Petter Selasky <hps@selasky.org>
|
Any reason to NOT merge this patch? |
|
Perhaps the test should be explained... |
|
@hselasky, a few months ago I made a few Capsicum-related bugfixes in tcpdump (commits 780f86b, ac23514 and 51f9c3b), it would be great if you could confirm these look good to you. Also it would be very useful to add a detailed comment into the proposed change, as it is often difficult to reconstruct the context. |
|
I'll have a look. Thanks for the heads up. |
|
Is there anything remaining to do here? |
|
@emaste : I have not observed any problems in FreeBSD-14-main as of recently. Are these patches imported into our code-base? If yes, then this issue can be closed. |
|
It looks like the one change under "Commits" is in FreeBSD, but is not in upstream tcpdump. |
|
I had another round at comprehending this change and managed to understand it this time. Excuse us for the delay, there has been a lot of backlog. |
No description provided.