Skip to content

Allow to use system's tomsfastmath library instead of the bundled one.#803

Closed
sebastianas wants to merge 1 commit intoCisco-Talos:mainfrom
sebastianas:system_tfm
Closed

Allow to use system's tomsfastmath library instead of the bundled one.#803
sebastianas wants to merge 1 commit intoCisco-Talos:mainfrom
sebastianas:system_tfm

Conversation

@sebastianas
Copy link
Contributor

By default the bundled tomsfastmath library is used. It can be overriden to use the library from the system if available. Should the system library only be able to handle RSA keys up to 2048 bits then the selftest will fail.

Signed-off-by: Sebastian Andrzej Siewior sebastian@breakpoint.cc

By default the bundled tomsfastmath library is used. It can be overriden
to use the library from the system if available. Should the system
library only be able to handle RSA keys up to 2048 bits then the
selftest will fail.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
@val-ms
Copy link
Contributor

val-ms commented Jan 6, 2023

We had this feature but removed it, here: 5c3e866

A standard build of tomsfastmath does not support wide enough floating point numbers for our use case and will not work correctly. It is better (for us) to know that everyone is using tomsfastmath that is configured to work for clamav.

Edit: I saw your comment:

Should the system library only be able to handle RSA keys up to 2048 bits then the selftest will fail.

... but I do not see this feature in this PR.

Edit 2: I also see you have a patch for the debian tomsfastmath package https://sources.debian.org/patches/tomsfastmath/0.13.1-1/Increase-FP_MAX_SIZE-to-8192.patch/

I should also note that I'm really hesitant to revert 5c3e866 because the more build configurations we have, the more we should test -- and that is very time consuming to build and test each variation, which means we probably won't do it.
Further, Tomsfastmath's last release was in 2017. I realize a CVE could pop up any time. It is C after all. But... 🤞 it seems pretty stable.

@sebastianas
Copy link
Contributor Author

sebastianas commented Jan 22, 2023 via email

@val-ms
Copy link
Contributor

val-ms commented Jan 23, 2023

Could we have something that is not announced?

It is included in the major changes in the change log;

I kind of need to use the in-tree lib rather than the bundled copy.

I feel like "need" is an exaggeration. It is not your responsibility to de-bundle bundled sources. I see the appeal in the situation where a bunch of other packages are using tomsfastmath. If de-bundled, you would be able to push out 1 update to tomsfastmath if there is a security issue and have that cover them all. Though I don't know how many other packages depend on tomsfastmath for Debian. I don't know how to check what other packages depend on https://packages.debian.org/source/bullseye/tomsfastmath. Though that sort of ties into my next point.

You could as upstream to update lib and make a release ;)

I don't know if changing from the defaults will break other software packages that expect the default settings. If there are no other packages that depend on tomsfastmath, then it seems even less critical that this tomsfastmath is de-bundled from clamav.

This idea is not invalid, but it doesn't take into consideration all of the other distros that may use clamav and think "hey, external tomsfast seems better, let's do that!" We obviously can't find and "fix" tomsfastmath packages for every distro.

This is the only change right? Sebastian

We also build with these defined:

  • TFM_CHECK
  • TMP_NO_ASM

Set through the build system in: 375ecf6#diff-6d5ce841c8f84bb1d6383da4bf5fa35316926e2e0668ad71aa137d7bd5429c43R26-R34

@sebastianas
Copy link
Contributor Author

sebastianas commented Jan 27, 2023 via email

@sebastianas
Copy link
Contributor Author

This is obsoleted by #840

@val-ms
Copy link
Contributor

val-ms commented Mar 9, 2023

Closing because not desired out of compatibility concerns with default TFM build config for other distros that may use it, and because obsoleted by #840 as Sebastian said.

@val-ms val-ms closed this Mar 9, 2023
@sebastianas sebastianas deleted the system_tfm branch June 27, 2024 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants