aes_shift_rows_fwd and aes_shift_rows_inv do not match crypto spec.

[mlawson-tt]

An issue was discovered a few months ago in the vector crypto spec (riscv/riscv-crypto#268) that caused a fix for aes_shift_rows_fwd and aes_shift_rows_inv (riscv/riscv-crypto@a19ae20), but the changes have not yet been incorporated here.

This section in aes_shift_rows_fwd:

  let oc0 : bits(32) = ic0[31..24] @ ic1[23..16] @ ic2[15.. 8] @ ic3[ 7.. 0];
  let oc1 : bits(32) = ic1[31..24] @ ic2[23..16] @ ic3[15.. 8] @ ic0[ 7.. 0];
  let oc2 : bits(32) = ic2[31..24] @ ic3[23..16] @ ic0[15.. 8] @ ic1[ 7.. 0];
  let oc3 : bits(32) = ic3[31..24] @ ic0[23..16] @ ic1[15.. 8] @ ic2[ 7.. 0];

should be:

  let oc0 : bits(32) = ic3[31..24] @ ic2[23..16] @ ic1[15.. 8] @ ic0[ 7.. 0];
  let oc1 : bits(32) = ic0[31..24] @ ic3[23..16] @ ic2[15.. 8] @ ic1[ 7.. 0];
  let oc2 : bits(32) = ic1[31..24] @ ic0[23..16] @ ic3[15.. 8] @ ic2[ 7.. 0];
  let oc3 : bits(32) = ic2[31..24] @ ic1[23..16] @ ic0[15.. 8] @ ic3[ 7.. 0];

and this section in aes_shift_rows_inv:

  let oc0 : bits(32) = ic0[31..24] @ ic3[23..16] @ ic2[15.. 8] @ ic1[ 7.. 0];
  let oc1 : bits(32) = ic1[31..24] @ ic0[23..16] @ ic3[15.. 8] @ ic2[ 7.. 0];
  let oc2 : bits(32) = ic2[31..24] @ ic1[23..16] @ ic0[15.. 8] @ ic3[ 7.. 0];
  let oc3 : bits(32) = ic3[31..24] @ ic2[23..16] @ ic1[15.. 8] @ ic0[ 7.. 0];

should be:

  let oc0 : bits(32) = ic1[31..24] @ ic2[23..16] @ ic3[15.. 8] @ ic0[ 7.. 0];
  let oc1 : bits(32) = ic2[31..24] @ ic3[23..16] @ ic0[15.. 8] @ ic1[ 7.. 0];
  let oc2 : bits(32) = ic3[31..24] @ ic0[23..16] @ ic1[15.. 8] @ ic2[ 7.. 0];
  let oc3 : bits(32) = ic0[31..24] @ ic1[23..16] @ ic2[15.. 8] @ ic3[ 7.. 0];

I have these changes on a local branch and confirmed that Sail now matches Spike for these functions, so I can submit a PR if it makes sense to do so.

[jrtc27]

Is this observable for the scalar crypto instructions that use this (surely yes?), and if so, does the fact this made it into the Sail model mean there was a testing gap?

Either way, yes, filing a PR to fix a bug makes sense, it'll just need a clear commit message explaining how the problem manifests in the scalar crypto code, or, if it doesn't, that it's a non-functional change for the scalar crypto code that enables upcoming vector crypto code to use it.

[mlawson-tt]

Turns out these functions aren't used in any of the scalar instructions (and aren't even called anywhere yet), but they still show up in the scalar crypto spec I think because the spec links directly to the file containing these functions (riscv_insts_zvkned.sail) in Sail itself, so fixing the functions here should fix the scalar spec for the next release.

[ptomsich]

#234 uses these instructions for the vector spec.
I haven't reviewed whether this matches the byte-order used there, but am under the impression that this has been tested against the test-vectors from NIST.

@charmitro Please review (and provide information on the test status for vector-crypto).

[charmitro]

PR #281 fixes any remaining issues between SAIL & Spike, all Zvkned ACT tests from riscv-non-isa/riscv-arch-test#333 are now passing.

Given that the Zvkned extension did not change dramatically between v20230303(this is the implemented version in the Zvk* Pull Requests) and v20230620(freeze). PR #234 can be considered valid against the recently freezed version of Vector Crypto.

@mlawson-tt due to the fact that the Zvk* Pull Requests are created from vector-dev branch and your target branch is master I just did an "oldschool" copy of your changes to riscv_types_kext.sail.

[mlawson-tt]

@charmitro I think there still may be one more issue with vaeskf2.vi because we're matching with Sail but mismatching with Spike when running those tests you linked (but all of the rest match), and I think it's an issue with the spec. I filed riscv/riscv-crypto#336, and it looks like the pseudocode is going to change slightly (via riscv/riscv-crypto#337), so just a heads up on that change just in case you're seeing it too.

[charmitro]

On Mon Jul 3, 2023 at 6:56 PM EEST, Matt Lawson wrote: @charmitro I think there still may be one more issue with `vaeskf2.vi` because we're matching with Sail but mismatching with Spike when running those tests you linked (but all of the rest match), and I think it's an issue with the spec. I filed riscv/riscv-crypto#336, and it looks like the pseudocode is going to change slightly (via riscv/riscv-crypto#337), so just a heads up on that change just in case you're seeing it too.

Yes, saw it when I was testing and already changed it to match Spike.