8374755: ML-KEM's 12-bit decompression can be simplified on aarch64 by ferakocz · Pull Request #29141 · openjdk/jdk

ferakocz · 2026-01-09T14:41:07Z

The preconditions for the aarch64 and the AVX-512 intrinsic implementations of the implKyber12To16() method of com.sun.crypto.provider.ML_KEM are different and the AVX-512 one has stricter preconditions on the input, which was not recorded in the assert() before calling the function (although they were satisfied by all calling code). Now the assert() is corrected, and with these preconditions, the aarch64 implementation is simplified.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8374755: ML-KEM's 12-bit decompression can be simplified on aarch64 (Bug - P4)

Reviewers

Andrew Dinn (@adinn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/29141/head:pull/29141
$ git checkout pull/29141

Update a local copy of the PR:
$ git checkout pull/29141
$ git pull https://git.openjdk.org/jdk.git pull/29141/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 29141

View PR using the GUI difftool:
$ git pr show -t 29141

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/29141.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-01-09T14:43:32Z

👋 Welcome back ferakocz! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-01-09T14:44:36Z

@ferakocz This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8374755: ML-KEM's 12-bit decompression can be simplified on aarch64

Reviewed-by: adinn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 169 new commits pushed to the master branch:

983ae96: 8375498: [VectorAPI] Dump primary vector IR details with -XX:+TraceNewVectors
5c7c2f0: 8375717: Outdated link in jdk.jfr.internal.JVM javadoc
b134030: 8238686: G1 may waste lots of space or fail to uncommit when observing MinHeapFreeRatio during sizing after full gc
... and 166 more: https://git.openjdk.org/jdk/compare/904ba5f5ed7d3ac1a3606ff7532ba3c206a2d9b9...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@adinn) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2026-01-09T14:45:44Z

@ferakocz The following labels will be automatically applied to this pull request:

hotspot-compiler
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2026-01-09T14:50:13Z

Webrevs

adinn · 2026-01-12T10:37:31Z

Changes look good. What testing have you run?

smemery · 2026-01-14T06:42:54Z

Changes look good. What testing have you run?

Thank you for your review. I've ran the ACVP tests for ML-KEM, which have all passed (default and intrinsics (aarch64)).

smemery · 2026-01-14T06:51:00Z

+        assert ((parsed.length >= n * 128) &&
+                (condensed.length >= index + n * 192));


Given the comments, can this be simplified to just:

- int n = (parsedLength + 127) / 128; - assert ((parsed.length >= n * 128) && - (condensed.length >= index + n * 192)); + assert((parsed.length % 128) == 0) && (condensed.length % 192 == 0));

If the length is smaller than the constant then the remainder will be non-zero.

These are the exact conditions that the most demanding intrinsic (the AVX-512 one) requires. If we would rely on that the callers satisfy these, we wouldn't need the assert :-) . The loop in the intrinsic will read n * 192 bytes and write n * 128 shorts, your suggestion would not ensure that the arrays have at least that much space.

Ah, I see that now. Maybe an update to the comments would alleviate my confusion?:
NEW:
// An intrinsic implementation assumes that the input and output buffers
// are such that condensed can be read in n-multiples of 192 bytes and
// parsed can be written in n-multiples of 128 shorts, so callers should allocate
// the condensed and parsed arrays to at least these amounts, see the assert()

smemery · 2026-01-14T07:45:26Z

    __ sub(parsedLength, parsedLength, 64);
-    __ cmp(parsedLength, (u1)64);
+    __ cmp(parsedLength, (u1)0);
    __ br(Assembler::GE, L_loop);


Should this be GT now?

Yes, I believe it should. That makes me wonder why the test did not fail. I would have expected it to loop back to the top and try to consume an extra 96 bytes of non-existent input and write it to 64 bytes of of non-existent output buffer? Did this erroneous computation not happen? or was the error simply not manifest?

It is a buffer overflow, so if the memory after the arrays is there, it would be read/written, if you are lucky, it doesn't overwrite anything that is used later, so it might be able to pass a test program (which definitely had happened here).

Yes, it should. Fixed.

theRealAph · 2026-01-15T10:54:08Z

  //         byte[] condensed, int index, short[] parsed, int parsedLength) {}
  //
-  // (parsedLength or (parsedLength - 48) must be divisible by 64.)
+  // it is assumed that parsed and condensed are allocated such that for


By whom? :-)

Suggested change

// it is assumed that parsed and condensed are allocated such that for

// we assume that parsed and condensed are allocated such that for

theRealAph · 2026-01-15T10:55:50Z

-    vs_st2_post(vs_front(va), __ T8H, parsed);
-    vs_st2_post(vs_front(vs_front(vb)), __ T8H, parsed);
-
-    __ BIND(L_end);


This is a substantial change, not a mere matter of "incorrect assertions". Perhaps this PR needs a more appropriate title.

Also, the description in the JIRA and the opening comment in this PR should mention that the intrinsic can be simplified in response to the stricter preconditions maintained by the Java client.

adinn

This looks good now, thank you.

I'm a little unhappy that the initial test did not detect the reads and writes that overflowed the end of, respectively, the input and output arrays. That may indeed be fixed now but it would have been nicer it the test had been able to catch the error. However, I understand that it is hard to achieve that when driving the VM from Java. So, let's hope we don't need any more changes or, if we do, we do our best to ensure (by eyeball) that we don't overshoot the end of the arrays.

smemery · 2026-01-21T06:50:01Z

/approve yes

openjdk · 2026-01-21T06:50:29Z

@smemery Only integrators for this repository are allowed to issue the /approve command.

ferakocz · 2026-01-21T12:14:35Z

Thanks! @adinn would you be so kind as to /sponsor the integration?

ferakocz · 2026-01-21T12:15:40Z

/integrate

openjdk · 2026-01-21T12:17:49Z

@ferakocz
Your change (at version da86c0b) is now ready to be sponsored by a Committer.

adinn · 2026-01-21T13:36:32Z

I believe this needs a second sign-off.

@theRealAph can you do the honours?

wangweij · 2026-01-29T12:49:46Z

/sponsor

openjdk · 2026-01-29T12:52:29Z

Going to push as commit 9911959.
Since your change was applied there have been 293 commits pushed to the master branch:

e85d5d7: 8375010: C2 VectorAPI: assert(vbox->is_CheckCastPP()) failed: should be expanded
4884674: 8374343: Fix SIGSEGV when lib/modules is unreadable
f96974d: 8373898: RepeatCompilation does not repeat compilation after bailout
... and 290 more: https://git.openjdk.org/jdk/compare/904ba5f5ed7d3ac1a3606ff7532ba3c206a2d9b9...master

Your commit was automatically rebased without conflicts.

openjdk · 2026-01-29T12:52:48Z

@wangweij @ferakocz Pushed as commit 9911959.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

adinn · 2026-01-29T14:30:03Z

@wangweij Thanks for sponsoring. While that probably counts implicitly as a review I think you are supposed to actually click the 'reviewed' button to acknowledge that you have reviewed the code.

n.b. Although the PR says only one review is required a Hotspot change actually needs two reviews with at least one (big R) 'Reviewer'.

wangweij · 2026-01-29T15:00:49Z

Didn't notice the reviewer count. Maybe the Skara bot hasn't enforced it. Otherwise, it should not add the ready and sponsor labels.

I typed /sponsor mainly because I trust Ferenc' code and reviews from you and Shawn. I don't think a sponsor has to review the code change themselves.

8374755: ML-KEM's 12-bit decompression uses incorrect assertions

f2437a6

openjdk Bot added security security-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Jan 9, 2026

openjdk Bot added the rfr Pull request is ready for review label Jan 9, 2026

smemery reviewed Jan 14, 2026

View reviewed changes

Fix off-by-one error discovered by Shawn

2fca58b

theRealAph reviewed Jan 15, 2026

View reviewed changes

improve comment

da86c0b

ferakocz changed the title ~~8374755: ML-KEM's 12-bit decompression uses incorrect assertions~~ 8374755: ML-KEM's 12-bit decompression can be simplified on aarch64 Jan 19, 2026

ferakocz changed the title ~~8374755: ML-KEM's 12-bit decompression can be simplified on aarch64~~ 8374755: ML-KEM's 12-bit decompression can be simplified on aarch64 Jan 19, 2026

adinn approved these changes Jan 20, 2026

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label Jan 20, 2026

openjdk Bot added the sponsor Pull request is ready to be sponsored label Jan 21, 2026

openjdk Bot added the integrated Pull request has been integrated label Jan 29, 2026

openjdk Bot closed this Jan 29, 2026

openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 29, 2026

		assert ((parsed.length >= n * 128) &&
		(condensed.length >= index + n * 192));

	// it is assumed that parsed and condensed are allocated such that for
	// we assume that parsed and condensed are allocated such that for

Conversation

ferakocz commented Jan 9, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Jan 9, 2026

Uh oh!

openjdk Bot commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk Bot commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

adinn commented Jan 12, 2026

Uh oh!

smemery commented Jan 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinn left a comment

Choose a reason for hiding this comment

Uh oh!

smemery commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk Bot commented Jan 21, 2026

Uh oh!

ferakocz commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ferakocz commented Jan 21, 2026

Uh oh!

openjdk Bot commented Jan 21, 2026

Uh oh!

adinn commented Jan 21, 2026

Uh oh!

wangweij commented Jan 29, 2026

Uh oh!

openjdk Bot commented Jan 29, 2026

Uh oh!

openjdk Bot commented Jan 29, 2026

Uh oh!

adinn commented Jan 29, 2026

Uh oh!

wangweij commented Jan 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

ferakocz commented Jan 9, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Jan 9, 2026 •

edited

Loading

openjdk Bot commented Jan 9, 2026 •

edited

Loading

mlbridge Bot commented Jan 9, 2026 •

edited

Loading

smemery commented Jan 21, 2026 •

edited

Loading

ferakocz commented Jan 21, 2026 •

edited

Loading