[8u/urgent] RFR: 8267689: [aarch64] Crash due to bad shift in indirect addressing mode

[Hohensee, Paul]

Thanks very much for the explanation. Looks good.

Paul

-----Original Message-----
From: Volker Simonis <volker.simonis at gmail.com>
Date: Thursday, May 27, 2021 at 1:04 AM
To: "Hohensee, Paul" <hohensee at amazon.com>
Cc: jdk8u-dev <jdk8u-dev at openjdk.java.net>, Roland Westrelin <rwestrel at redhat.com>
Subject: RE: [8u/urgent] RFR: 8267689: [aarch64] Crash due to bad shift in indirect addressing mode

"On Wed, May 26, 2021 at 4:16 PM Hohensee, Paul <hohensee at amazon.com> wrote:
>
> Lgtm, except:
>
> I'm not an expert on the details of the IR, but based on the description in addnode.hpp, instead of
>
>     n->in(AddPNode::Address)->in(AddPNode::Offset)
>
> could use
>
>     n->in(AddPNode::Offset)
>
> The former looks like it relies on n->in(AddPNode::Address) being itself an AddPNode.
>

Hi Paul,

thanks for looking at my change. The expression for selecting the
shift amount depends on the match expression. If we have a simple
match expression (with a single "AddP" node) like:

match(AddP reg (LShiftL lreg scale));

then we need a simple expression like:

n->in(AddPNode::Offset)->in(2)->get_int())

This  takes the second input argument (i.e. "in(AddPNode::Offset)") of
the AddP node (i.e. "n"). In this example that's an "LShiftL" node.
From this LShiftL node (whose first input argument is the register
"lreg") we have to take the second input argument (i.e. "in(2)") and
interpret it as an integer constant (i.e. "get_int()").

If we have a more complex match expression like:

match(AddP (AddP reg (LShiftL lreg scale)) off);

then we also need the more complex expression to get the shift amount:

n->in(AddPNode::Address)->in(AddPNode::Offset)->in(2)->get_int()));

Here, the first argument of the top-level "AddP" node (i.e.
"in(AddPNode::Address)") is not a simple register like in the previous
example but rather another "AddP" node. It's this nested "AddP" node
which has an "LShiftL" node as its second input argument (i.e.
"in(AddPNode::Address)->in(AddPNode::Offset)") that's why we need the
more complex expression to get a handle to it.

Hope this makes the change more understandable.

Best regards,
Volker

> Thanks,
> Paul
>
> -----Original Message-----
> From: jdk8u-dev <jdk8u-dev-retn at openjdk.java.net> on behalf of Volker Simonis <volker.simonis at gmail.com>
> Date: Tuesday, May 25, 2021 at 7:58 AM
> To: jdk8u-dev <jdk8u-dev at openjdk.java.net>, Roland Westrelin <rwestrel at redhat.com>
> Subject: [8u/urgent] RFR: 8267689: [aarch64] Crash due to bad shift in indirect addressing mode
>
> Hi,
>
> sorry for the the "urgent" tag but I think this is a fix which is
> worth while bringing into the next update release 8u302. This issue
> was initially reported against Corretto 8
> (https://github.com/corretto/corretto-8/issues/302), but it affects
> all version of OpenJDK8u on aarch64. It can cause arbitrary crashes
> due to incorrectly C2-generated code. We currently have two customer
> who are affected by this issue and one of them reported back that the
> proposed patch fixes the problem:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2021/8267689/
> https://bugs.openjdk.java.net/browse/JDK-8267689
>
> The JBS issue contains a detailed description on how this issue can be
> reproduced (thanks to @charliegracie).
>
> Notice that this issue only affects OpenJDK8u. In later releases it
> was fixed by JDK-8154826 which completely reworked complex addressing
> modes. Because that change doesn't apply cleanly (i.e. depends on
> JDK-8136820, JDK-8034812, etc) and also affects other platforms I
> suggest a point fix here which only fixes the current issue in jdk8u
> instead of a downport.
>
> The problem is that on aarch64, indirect loads and stores can accept
> and index and a shift, but the shift has to correspond to the size of
> the load/store operation (e.g. shift==2 for 32-bit operations and
> shift==3 for 64-bit operations). Usage of sun.misc.Unsafe can lead to
> address expressions which violate this requirement. They might for
> example request a shift of "1" for a 32-bit operation. In the release
> build, the generated code will nevertheless perform a 2-bit shift in
> such a situation which can lead to arbitrary crashes at a later time.
> In a debug build, the problem will be detected by the following
> assertion:
>
> assert(_ext.shift() == (int)size) failed: bad shift
>
> This change only fixes the current issue in jdk8 by checking the shift
> amount against the size of the memory operation in the Matcher and
> rejects address modes where they don't conform. It borrows some code
> like the `size_fits_all_mem_uses()` function from JDK-8154826 [1] but
> uses it in more places because 8u has much more match rules which are
> affected by the the bad pattern.
>
> @roland sorry for calling you out explicitly, but I think as the
> author of JDK-8154826u you would be the ideal Reviewer for this change
> :)
>
> Thank you and best regards,
> Volker
>
> [1] http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/064d18bc1889
>