void-packages icon indicating copy to clipboard operation
void-packages copied to clipboard

[ci skip] Enable AEGIS cipher in kernel, clean up and make kernel crypto config more uniform across platforms

Open PoroCYon opened this issue 3 months ago • 10 comments

This is the outcome of things I talked about on IRC a week or two ago. Basically, I wanted to enable AEGIS support in the kernel (which is simply just a line in the config file). But, as I went along, I noticed that some parts of the kernel config files didn't really match those for other architectures (at least, speaking about the cryptography parts. I didn't look at any other part of the kernel config files).

For example, for some reason, the i686 config files still had SERPENT (an obsolete cipher) support built into the kernel (not as a module). Other config files disagreed on whether CONFIG_CRYPTO_SM4 or CONFIG_CRYPTO_842 should be enabled (as a module) or entirely disabled, which seems like a bad situation. So (after asking for permisson on IRC) I started to rectify this situation, by:

  • Enabling any config option that is enabled on at least one architecture
  • If the option is enabled as in-kernel on one architecture, enable it on all architectures
    • Unless it is ancient cryptography, like SERPENT or MD4 (yes, 4, not 5), then make it a module everywhere
    • GCM for example is now in-kernel

I split this PR into 3 commits (as this is how I have done it at first), but this could change if needed. Things I am currently still wondering about:

  • Do the commits need to be split up into ones for different packages?
  • Should the kernel release number be increased at all? I've seen commits changing the kernel config files without doing this, but to me this seems vaguely problematic wrt. "one version+release == one config/binary/behavior" being a desirable property.

Testing the changes

  • I tested the changes in this PR: YES
    • Tested on:
      • x86_64-glibc (HP Probook 650)
      • aarch64-musl (RPi 3B+)
      • i686-glibc (Qemu VM)

I've been using this configuration for weeks now on the Probook and I haven't encountered any issue. Testing times on the others weren't as long but I still couldn't notice a single problem.

Local build testing

  • I built this PR locally for my native architecture, x86_64-glibc, aarch64-musl
  • I built this PR locally for these architectures (if supported. mark crossbuilds):
    • aarch64-musl (crossbuild)
      • Also for rpi-kernel, rpi5-kernel, pinephone-kernel and linux-asahi
        • Yes I still tested rpi-kernel on an actual machine but I had to crossbuild it because the RPi 3B+ OOMed and ran out of disk space while trying to build the kernel.
    • i686-glibc (crossbuild)
    • ppc64-musl (crossbuild)
    • ppc64le-musl (crossbuild)
    • armv7hf-musl (crossbuild)
    • armv6l-musl (crossbuild)
      • Also for rpi-kernel

PoroCYon avatar Aug 29 '25 05:08 PoroCYon

Ok apparently the lint is failing because the commit message is "a bit long". I can change that once I have an answer to the "should the commits be split up into ones for separate packages?" question.

PoroCYon avatar Aug 29 '25 06:08 PoroCYon

one commit per package please

classabbyamp avatar Aug 31 '25 03:08 classabbyamp

Is this OK, or should I change more things? Should the CI build now be ran? (I didn't enable this initially as I had anticipated some requests for changes, and didn't want to burden the buildservers with not-very-useful work.)

PoroCYon avatar Aug 31 '25 18:08 PoroCYon

I don't understand the 'm' to 'y' changes, that looks backwards. Why would you make the kernel bigger for everyone even for those that won't use the code.

If they were 'y' only for some arches, maybe that's what needs to change.

Then based on the answer to that... I'd not object to enabling additionnal algorithms as long as that is an 'm', so it is only download bandwidth and disk storage.

Removing obsoleted ones is in between, you might break someone's usage, but then if it's not considered secure any more, but then if the usage was not about security... So, no real objection to that.

Just IMHO, though.

vincele avatar Sep 01 '25 19:09 vincele

@vincele have you looked at the changes I'm proposing here?

I suppose AEGIS128 could be built as a module instead (which is an n->y change, not m->y which is what you seem to be objecting to), but, as for the rest:

Removing obsoleted ones is in between, you might break someone's usage, but then if it's not considered secure any more, but then if the usage was not about security... So, no real objection to that.

I didn't even change a single outdated module to n, no matter how silly it is to still include it (e.g. MD4). A few weeks ago on IRC I asked about what should be done about these, and the response was very much that these should not be fully removed (even though this would've been my personal preference, for non-critical stuff one could also just use a userspace implementation).

If they were 'y' only for some arches, maybe that's what needs to change.

That's the thing here, the n/m/y stuff was very inconsistent between architectures. And while having e.g. SERPENT as y also feels slightly ridiculous (so I suppose we both agree on that it should be m), I thought for many other (modern, deemed secure) ciphers there had to be a reason for this, and thus I had set them to y across all architectures. Config options that had m across all architectures I left basically as-is.

PoroCYon avatar Sep 01 '25 19:09 PoroCYon

(Also, we're talking about overheads of at most a few kilobytes, see the implementation. At those sizes, the ELF file format overhead is probably larger than the code itself, so compiling it as a module might introduce more overhead compared to including it into the kernel binary)

PoroCYon avatar Sep 01 '25 20:09 PoroCYon

(Also, we're talking about overheads of at most a few kilobytes, see the implementation. At those sizes, the ELF file format overhead is probably larger than the code itself, so compiling it as a module might introduce more overhead compared to including it into the kernel binary)

Did a quick measurement: I compiled aegis-aesni.ko and measured the total "code and runtime data" bytes in that file (strip aegis128-aesni.ko && readelf -WS aegis128-aesni.ko | awk '$3 ~ "PROGBITS" {print $6}'|xargs python -c 'import sys;print(sum(int(x,16) for x in sys.argv[1:]))'). The result was 3294 bytes (kernel 6.12.39 which is what this laptop is currently running), while the total size of the kernel module file is.... 12152 bytes. Rougly 75% of the kernel module file is overhead. Furthermore, this module is not signed as it was built out-of-tree, while ones built in-tree and distributed through XBPS are. These signatures add another 720 bytes.

PoroCYon avatar Sep 01 '25 20:09 PoroCYon

No I obviously did not read the whole thing, sorry about that.

I am objecting to the m->y, and I saw that hapening in the thing I read, that's why I chimed in.

I don't think I carried my thoughts adequately about the size. I don't really care about the net BW or disk size. But getting code in RAM, just for nothing is wasteful no matter the size. Making this modular instead of builtin has almost no overhead, so why not moving everything to 'm' is really the core of my objection and questionning.

vincele avatar Sep 01 '25 21:09 vincele

Ah yeah, I see now, sorry for earlier then.

In the current changeset, I more or less did what is the status-quo. That doesn't mean it shouldn't be changed, but, that falls slightly outside of the scope of the current PR. It might be a good idea to ask around & see what the consensus is wrt. keeping various ciphers in the kernel or leaving them out, but that would delay this PR even more, hence why I think a separate PR for that would make more sense.

EDIT: which is to say: if the "maybe it should all be modules" comment would've been raised on IRC a few weeks ago and people there seemed to be in general agreement about it, I would've done it, but now it feels a bit late to wait another week or so with this PR to look for more consensus.

PoroCYon avatar Sep 01 '25 22:09 PoroCYon

Pull Requests become stale 90 days after last activity and are closed 14 days after that. If this pull request is still relevant bump it or assign it.

github-actions[bot] avatar Dec 09 '25 02:12 github-actions[bot]