diff mbox series

RPCSEC GSS krb5 KUnit test fails on arm64 with h/w accelerated ciphers enabled

Message ID ZEBi8ReG9LKLcmW3@aion.usersys.redhat.com (mailing list archive)
State New, archived
Headers show
Series RPCSEC GSS krb5 KUnit test fails on arm64 with h/w accelerated ciphers enabled | expand

Commit Message

Scott Mayhew April 19, 2023, 9:53 p.m. UTC
Chuck's recently-added RPCSEC GSS krb5 KUnit test
(net/sunrpc/auth_gss/gss_krb5_test.c) is failing on arm64, specifically
the RFC 3962 test cases (I'm just pasting the output of 1 case, but all
6 cases fail):

---8<---
[  237.255197]         # Subtest: RFC 3962 encryption
[  237.255588]     # RFC 3962 encryption: EXPECTATION FAILED at net/sunrpc/auth_gss/gss_krb5_test.c:772
                   Expected memcmp(param->next_iv->data, iv, param->next_iv->len) == 0, but
                       memcmp(param->next_iv->data, iv, param->next_iv->len) == 1 (0x1)
               
               IV mismatch
---8<---

If I disable the hardware accelerated ciphers
(CONFIG_CRYPTO_AES_ARM64_CE_BLK and CONFIG_CRYPTO_AES_ARM64_NEON_BLK),
then the test works.

Likewise, if I modify Chuck's test to explicitly request
"cts(cbc(aes-generic))", then the test works.

The problem is that the asm helper aes_cbc_cts_encrypt in
arch/arm64/crypto/aes-modes.S doesn't return the next IV.

If I make the following change, then the test works:

But I don't know if that change is at all correct! (I've never even
looked at arm64 asm before).  If someone who's knowledgeable about this
code could chime in, I'd appreciate it.

-Scott

Comments

Herbert Xu April 28, 2023, 9:44 a.m. UTC | #1
Scott Mayhew <smayhew@redhat.com> wrote:
>
> diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
> index 0e834a2c062c..477605fad76b 100644
> --- a/arch/arm64/crypto/aes-modes.S
> +++ b/arch/arm64/crypto/aes-modes.S
> @@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt)
>        add             x4, x0, x4
>        st1             {v0.16b}, [x4]                  /* overlapping stores */
>        st1             {v1.16b}, [x0]
> +       st1             {v1.16b}, [x5]
>        ret
> AES_FUNC_END(aes_cbc_cts_encrypt)
> 
> But I don't know if that change is at all correct! (I've never even
> looked at arm64 asm before).  If someone who's knowledgeable about this
> code could chime in, I'd appreciate it.

Ard, could you please take a look at this?

Thanks,
Ard Biesheuvel April 28, 2023, 9:57 a.m. UTC | #2
On Fri, 28 Apr 2023 at 10:44, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> Scott Mayhew <smayhew@redhat.com> wrote:
> >
> > diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
> > index 0e834a2c062c..477605fad76b 100644
> > --- a/arch/arm64/crypto/aes-modes.S
> > +++ b/arch/arm64/crypto/aes-modes.S
> > @@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt)
> >        add             x4, x0, x4
> >        st1             {v0.16b}, [x4]                  /* overlapping stores */
> >        st1             {v1.16b}, [x0]
> > +       st1             {v1.16b}, [x5]
> >        ret
> > AES_FUNC_END(aes_cbc_cts_encrypt)
> >
> > But I don't know if that change is at all correct! (I've never even
> > looked at arm64 asm before).  If someone who's knowledgeable about this
> > code could chime in, I'd appreciate it.
>
> Ard, could you please take a look at this?
>

The issue seems to be that the caller expects iv_out to have been
populated even when doing ciphertext stealing.

This is meaningless, because the output IV can only be used to chain
additional encrypted data if the split is at a block boundary. The
ciphertext stealing fundamentally terminates the encryption, and
produces a block of ciphertext that is shorter than the block size, so
what the output IV should be is actually unspecified.

IOW, test cases having plain/ciphertext vectors whose size is not a
multiple of the cipher block size should not specify an expected value
for the output IV.
Chuck Lever April 28, 2023, 12:59 p.m. UTC | #3
> On Apr 28, 2023, at 5:57 AM, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Fri, 28 Apr 2023 at 10:44, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>> 
>> Scott Mayhew <smayhew@redhat.com> wrote:
>>> 
>>> diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
>>> index 0e834a2c062c..477605fad76b 100644
>>> --- a/arch/arm64/crypto/aes-modes.S
>>> +++ b/arch/arm64/crypto/aes-modes.S
>>> @@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt)
>>>       add             x4, x0, x4
>>>       st1             {v0.16b}, [x4]                  /* overlapping stores */
>>>       st1             {v1.16b}, [x0]
>>> +       st1             {v1.16b}, [x5]
>>>       ret
>>> AES_FUNC_END(aes_cbc_cts_encrypt)
>>> 
>>> But I don't know if that change is at all correct! (I've never even
>>> looked at arm64 asm before).  If someone who's knowledgeable about this
>>> code could chime in, I'd appreciate it.
>> 
>> Ard, could you please take a look at this?
>> 
> 
> The issue seems to be that the caller expects iv_out to have been
> populated even when doing ciphertext stealing.
> 
> This is meaningless, because the output IV can only be used to chain
> additional encrypted data if the split is at a block boundary. The
> ciphertext stealing fundamentally terminates the encryption, and
> produces a block of ciphertext that is shorter than the block size, so
> what the output IV should be is actually unspecified.
> 
> IOW, test cases having plain/ciphertext vectors whose size is not a
> multiple of the cipher block size should not specify an expected value
> for the output IV.

The test cases are extracted from RFC 3962 Appendix B. The
standard clearly expects there to be a non-zero next IV for
plaintext sizes that are not block-aligned.

Also, these test cases pass on other hardware platforms.


--
Chuck Lever
Ard Biesheuvel April 28, 2023, 4:09 p.m. UTC | #4
On Fri, 28 Apr 2023 at 13:59, Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Apr 28, 2023, at 5:57 AM, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Fri, 28 Apr 2023 at 10:44, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >>
> >> Scott Mayhew <smayhew@redhat.com> wrote:
> >>>
> >>> diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
> >>> index 0e834a2c062c..477605fad76b 100644
> >>> --- a/arch/arm64/crypto/aes-modes.S
> >>> +++ b/arch/arm64/crypto/aes-modes.S
> >>> @@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt)
> >>>       add             x4, x0, x4
> >>>       st1             {v0.16b}, [x4]                  /* overlapping stores */
> >>>       st1             {v1.16b}, [x0]
> >>> +       st1             {v1.16b}, [x5]
> >>>       ret
> >>> AES_FUNC_END(aes_cbc_cts_encrypt)
> >>>
> >>> But I don't know if that change is at all correct! (I've never even
> >>> looked at arm64 asm before).  If someone who's knowledgeable about this
> >>> code could chime in, I'd appreciate it.
> >>
> >> Ard, could you please take a look at this?
> >>
> >
> > The issue seems to be that the caller expects iv_out to have been
> > populated even when doing ciphertext stealing.
> >
> > This is meaningless, because the output IV can only be used to chain
> > additional encrypted data if the split is at a block boundary. The
> > ciphertext stealing fundamentally terminates the encryption, and
> > produces a block of ciphertext that is shorter than the block size, so
> > what the output IV should be is actually unspecified.
> >
> > IOW, test cases having plain/ciphertext vectors whose size is not a
> > multiple of the cipher block size should not specify an expected value
> > for the output IV.
>
> The test cases are extracted from RFC 3962 Appendix B. The
> standard clearly expects there to be a non-zero next IV for
> plaintext sizes that are not block-aligned.
>

OK, so this is the Kerberos V specific spec on how to use AES in CBC
mode, which appears to describe how to chain multiple CBC encryptions
together.

CBC-CTS itself does not define this: the IV is an input only, and the
reason we happen to return the IV is because a single CBC operation
may be broken up into several ones, which can only be done on block
boundaries. This is why CBC-CTS itself passes all its tests: a single
CBC-CTS encryption only performs ciphertext stealing at the very end,
and the next IV is never used in that case. (This is why the CTS mode
tests in crypto/testmgr.h don't have iv_out vectors)

Note that RFC3962 defines that the penultimate block of CBC-CTS
ciphertext is used as the next IV. CBC returns the last ciphertext
block as the output IV. It is a happy coincidence that the generic CTS
template encapsulates CBC in a way where its output IV ends up in the
right place.

> Also, these test cases pass on other hardware platforms.
>

Fair enough.

I am not opposed to fixing this, but given that it is the RFC3962 spec
that defines that the next IV is the penultimate full block of the
previous CBC-CTS ciphertext, we might consider doing the memcpy() in
the Kerberos code not in the CBC-CTS implementations. (The 32-bit ARM
code will be broken in the same way, and potentially other
implementations as well)
Chuck Lever April 28, 2023, 4:18 p.m. UTC | #5
> On Apr 28, 2023, at 12:09 PM, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Fri, 28 Apr 2023 at 13:59, Chuck Lever III <chuck.lever@oracle.com> wrote:
>> 
>> 
>> 
>>> On Apr 28, 2023, at 5:57 AM, Ard Biesheuvel <ardb@kernel.org> wrote:
>>> 
>>> On Fri, 28 Apr 2023 at 10:44, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>>>> 
>>>> Scott Mayhew <smayhew@redhat.com> wrote:
>>>>> 
>>>>> diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
>>>>> index 0e834a2c062c..477605fad76b 100644
>>>>> --- a/arch/arm64/crypto/aes-modes.S
>>>>> +++ b/arch/arm64/crypto/aes-modes.S
>>>>> @@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt)
>>>>>      add             x4, x0, x4
>>>>>      st1             {v0.16b}, [x4]                  /* overlapping stores */
>>>>>      st1             {v1.16b}, [x0]
>>>>> +       st1             {v1.16b}, [x5]
>>>>>      ret
>>>>> AES_FUNC_END(aes_cbc_cts_encrypt)
>>>>> 
>>>>> But I don't know if that change is at all correct! (I've never even
>>>>> looked at arm64 asm before).  If someone who's knowledgeable about this
>>>>> code could chime in, I'd appreciate it.
>>>> 
>>>> Ard, could you please take a look at this?
>>>> 
>>> 
>>> The issue seems to be that the caller expects iv_out to have been
>>> populated even when doing ciphertext stealing.
>>> 
>>> This is meaningless, because the output IV can only be used to chain
>>> additional encrypted data if the split is at a block boundary. The
>>> ciphertext stealing fundamentally terminates the encryption, and
>>> produces a block of ciphertext that is shorter than the block size, so
>>> what the output IV should be is actually unspecified.
>>> 
>>> IOW, test cases having plain/ciphertext vectors whose size is not a
>>> multiple of the cipher block size should not specify an expected value
>>> for the output IV.
>> 
>> The test cases are extracted from RFC 3962 Appendix B. The
>> standard clearly expects there to be a non-zero next IV for
>> plaintext sizes that are not block-aligned.
>> 
> 
> OK, so this is the Kerberos V specific spec on how to use AES in CBC
> mode, which appears to describe how to chain multiple CBC encryptions
> together.
> 
> CBC-CTS itself does not define this: the IV is an input only, and the
> reason we happen to return the IV is because a single CBC operation
> may be broken up into several ones, which can only be done on block
> boundaries. This is why CBC-CTS itself passes all its tests: a single
> CBC-CTS encryption only performs ciphertext stealing at the very end,
> and the next IV is never used in that case. (This is why the CTS mode
> tests in crypto/testmgr.h don't have iv_out vectors)
> 
> Note that RFC3962 defines that the penultimate block of CBC-CTS
> ciphertext is used as the next IV. CBC returns the last ciphertext
> block as the output IV. It is a happy coincidence that the generic CTS
> template encapsulates CBC in a way where its output IV ends up in the
> right place.
> 
>> Also, these test cases pass on other hardware platforms.
>> 
> 
> Fair enough.
> 
> I am not opposed to fixing this, but given that it is the RFC3962 spec
> that defines that the next IV is the penultimate full block of the
> previous CBC-CTS ciphertext, we might consider doing the memcpy() in
> the Kerberos code not in the CBC-CTS implementations.

Interesting thought. I'm all about proper layering, so I think this
is worth considering. Can you send an RFC patch?

David, any opinion about this for crypto/krb5?


> (The 32-bit ARM
> code will be broken in the same way, and potentially other
> implementations as well)


--
Chuck Lever
Ard Biesheuvel April 28, 2023, 4:48 p.m. UTC | #6
On Fri, 28 Apr 2023 at 17:18, Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Apr 28, 2023, at 12:09 PM, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Fri, 28 Apr 2023 at 13:59, Chuck Lever III <chuck.lever@oracle.com> wrote:
> >>
> >>
> >>
> >>> On Apr 28, 2023, at 5:57 AM, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>
> >>> On Fri, 28 Apr 2023 at 10:44, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >>>>
> >>>> Scott Mayhew <smayhew@redhat.com> wrote:
> >>>>>
> >>>>> diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
> >>>>> index 0e834a2c062c..477605fad76b 100644
> >>>>> --- a/arch/arm64/crypto/aes-modes.S
> >>>>> +++ b/arch/arm64/crypto/aes-modes.S
> >>>>> @@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt)
> >>>>>      add             x4, x0, x4
> >>>>>      st1             {v0.16b}, [x4]                  /* overlapping stores */
> >>>>>      st1             {v1.16b}, [x0]
> >>>>> +       st1             {v1.16b}, [x5]
> >>>>>      ret
> >>>>> AES_FUNC_END(aes_cbc_cts_encrypt)
> >>>>>
> >>>>> But I don't know if that change is at all correct! (I've never even
> >>>>> looked at arm64 asm before).  If someone who's knowledgeable about this
> >>>>> code could chime in, I'd appreciate it.
> >>>>
> >>>> Ard, could you please take a look at this?
> >>>>
> >>>
> >>> The issue seems to be that the caller expects iv_out to have been
> >>> populated even when doing ciphertext stealing.
> >>>
> >>> This is meaningless, because the output IV can only be used to chain
> >>> additional encrypted data if the split is at a block boundary. The
> >>> ciphertext stealing fundamentally terminates the encryption, and
> >>> produces a block of ciphertext that is shorter than the block size, so
> >>> what the output IV should be is actually unspecified.
> >>>
> >>> IOW, test cases having plain/ciphertext vectors whose size is not a
> >>> multiple of the cipher block size should not specify an expected value
> >>> for the output IV.
> >>
> >> The test cases are extracted from RFC 3962 Appendix B. The
> >> standard clearly expects there to be a non-zero next IV for
> >> plaintext sizes that are not block-aligned.
> >>
> >
> > OK, so this is the Kerberos V specific spec on how to use AES in CBC
> > mode, which appears to describe how to chain multiple CBC encryptions
> > together.
> >
> > CBC-CTS itself does not define this: the IV is an input only, and the
> > reason we happen to return the IV is because a single CBC operation
> > may be broken up into several ones, which can only be done on block
> > boundaries. This is why CBC-CTS itself passes all its tests: a single
> > CBC-CTS encryption only performs ciphertext stealing at the very end,
> > and the next IV is never used in that case. (This is why the CTS mode
> > tests in crypto/testmgr.h don't have iv_out vectors)
> >
> > Note that RFC3962 defines that the penultimate block of CBC-CTS
> > ciphertext is used as the next IV. CBC returns the last ciphertext
> > block as the output IV. It is a happy coincidence that the generic CTS
> > template encapsulates CBC in a way where its output IV ends up in the
> > right place.
> >
> >> Also, these test cases pass on other hardware platforms.
> >>
> >
> > Fair enough.
> >
> > I am not opposed to fixing this, but given that it is the RFC3962 spec
> > that defines that the next IV is the penultimate full block of the
> > previous CBC-CTS ciphertext, we might consider doing the memcpy() in
> > the Kerberos code not in the CBC-CTS implementations.
>
> Interesting thought. I'm all about proper layering, so I think this
> is worth considering. Can you send an RFC patch?
>

I'm not that familiar with kunit so this is just an off hand
suggestion, but I imagine something like the below would suffice

--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -639,6 +639,13 @@ gss_krb5_cts_crypt(struct crypto_sync_skcipher
*cipher, struct xdr_buf *buf,

        ret = write_bytes_to_xdr_buf(buf, offset, data, len);

+       /*
+        * CBC-CTS does not define an output IV but RFC 3962 defines it as the
+        * penultimate block of ciphertext, so copy that into the IV buffer
+        * before returning.
+        */
+       if (encrypt)
+               memcpy(iv, data, crypto_sync_skcipher_ivsize(cipher));
 out:
        kfree(data);
        return ret;
Eric Biggers April 28, 2023, 11:46 p.m. UTC | #7
On Fri, Apr 28, 2023 at 05:48:30PM +0100, Ard Biesheuvel wrote:
> On Fri, 28 Apr 2023 at 17:18, Chuck Lever III <chuck.lever@oracle.com> wrote:
> >
> >
> >
> > > On Apr 28, 2023, at 12:09 PM, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Fri, 28 Apr 2023 at 13:59, Chuck Lever III <chuck.lever@oracle.com> wrote:
> > >>
> > >>
> > >>
> > >>> On Apr 28, 2023, at 5:57 AM, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >>>
> > >>> On Fri, 28 Apr 2023 at 10:44, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > >>>>
> > >>>> Scott Mayhew <smayhew@redhat.com> wrote:
> > >>>>>
> > >>>>> diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
> > >>>>> index 0e834a2c062c..477605fad76b 100644
> > >>>>> --- a/arch/arm64/crypto/aes-modes.S
> > >>>>> +++ b/arch/arm64/crypto/aes-modes.S
> > >>>>> @@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt)
> > >>>>>      add             x4, x0, x4
> > >>>>>      st1             {v0.16b}, [x4]                  /* overlapping stores */
> > >>>>>      st1             {v1.16b}, [x0]
> > >>>>> +       st1             {v1.16b}, [x5]
> > >>>>>      ret
> > >>>>> AES_FUNC_END(aes_cbc_cts_encrypt)
> > >>>>>
> > >>>>> But I don't know if that change is at all correct! (I've never even
> > >>>>> looked at arm64 asm before).  If someone who's knowledgeable about this
> > >>>>> code could chime in, I'd appreciate it.
> > >>>>
> > >>>> Ard, could you please take a look at this?
> > >>>>
> > >>>
> > >>> The issue seems to be that the caller expects iv_out to have been
> > >>> populated even when doing ciphertext stealing.
> > >>>
> > >>> This is meaningless, because the output IV can only be used to chain
> > >>> additional encrypted data if the split is at a block boundary. The
> > >>> ciphertext stealing fundamentally terminates the encryption, and
> > >>> produces a block of ciphertext that is shorter than the block size, so
> > >>> what the output IV should be is actually unspecified.
> > >>>
> > >>> IOW, test cases having plain/ciphertext vectors whose size is not a
> > >>> multiple of the cipher block size should not specify an expected value
> > >>> for the output IV.
> > >>
> > >> The test cases are extracted from RFC 3962 Appendix B. The
> > >> standard clearly expects there to be a non-zero next IV for
> > >> plaintext sizes that are not block-aligned.
> > >>
> > >
> > > OK, so this is the Kerberos V specific spec on how to use AES in CBC
> > > mode, which appears to describe how to chain multiple CBC encryptions
> > > together.
> > >
> > > CBC-CTS itself does not define this: the IV is an input only, and the
> > > reason we happen to return the IV is because a single CBC operation
> > > may be broken up into several ones, which can only be done on block
> > > boundaries. This is why CBC-CTS itself passes all its tests: a single
> > > CBC-CTS encryption only performs ciphertext stealing at the very end,
> > > and the next IV is never used in that case. (This is why the CTS mode
> > > tests in crypto/testmgr.h don't have iv_out vectors)
> > >
> > > Note that RFC3962 defines that the penultimate block of CBC-CTS
> > > ciphertext is used as the next IV. CBC returns the last ciphertext
> > > block as the output IV. It is a happy coincidence that the generic CTS
> > > template encapsulates CBC in a way where its output IV ends up in the
> > > right place.
> > >
> > >> Also, these test cases pass on other hardware platforms.
> > >>
> > >
> > > Fair enough.
> > >
> > > I am not opposed to fixing this, but given that it is the RFC3962 spec
> > > that defines that the next IV is the penultimate full block of the
> > > previous CBC-CTS ciphertext, we might consider doing the memcpy() in
> > > the Kerberos code not in the CBC-CTS implementations.
> >
> > Interesting thought. I'm all about proper layering, so I think this
> > is worth considering. Can you send an RFC patch?
> >
> 
> I'm not that familiar with kunit so this is just an off hand
> suggestion, but I imagine something like the below would suffice
> 
> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> @@ -639,6 +639,13 @@ gss_krb5_cts_crypt(struct crypto_sync_skcipher
> *cipher, struct xdr_buf *buf,
> 
>         ret = write_bytes_to_xdr_buf(buf, offset, data, len);
> 
> +       /*
> +        * CBC-CTS does not define an output IV but RFC 3962 defines it as the
> +        * penultimate block of ciphertext, so copy that into the IV buffer
> +        * before returning.
> +        */
> +       if (encrypt)
> +               memcpy(iv, data, crypto_sync_skcipher_ivsize(cipher));

The whole "update the IV" thing that the crypto API does for some algorithms is
very weird and nonstandard, and most algorithms don't implement it.  So I'd
definitely support handling this in the caller here.

- Eric
Scott Mayhew May 1, 2023, 1:02 p.m. UTC | #8
On Fri, 28 Apr 2023, Ard Biesheuvel wrote:

> On Fri, 28 Apr 2023 at 17:18, Chuck Lever III <chuck.lever@oracle.com> wrote:
> >
> >
> >
> > > On Apr 28, 2023, at 12:09 PM, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Fri, 28 Apr 2023 at 13:59, Chuck Lever III <chuck.lever@oracle.com> wrote:
> > >>
> > >>
> > >>
> > >>> On Apr 28, 2023, at 5:57 AM, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >>>
> > >>> On Fri, 28 Apr 2023 at 10:44, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > >>>>
> > >>>> Scott Mayhew <smayhew@redhat.com> wrote:
> > >>>>>
> > >>>>> diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
> > >>>>> index 0e834a2c062c..477605fad76b 100644
> > >>>>> --- a/arch/arm64/crypto/aes-modes.S
> > >>>>> +++ b/arch/arm64/crypto/aes-modes.S
> > >>>>> @@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt)
> > >>>>>      add             x4, x0, x4
> > >>>>>      st1             {v0.16b}, [x4]                  /* overlapping stores */
> > >>>>>      st1             {v1.16b}, [x0]
> > >>>>> +       st1             {v1.16b}, [x5]
> > >>>>>      ret
> > >>>>> AES_FUNC_END(aes_cbc_cts_encrypt)
> > >>>>>
> > >>>>> But I don't know if that change is at all correct! (I've never even
> > >>>>> looked at arm64 asm before).  If someone who's knowledgeable about this
> > >>>>> code could chime in, I'd appreciate it.
> > >>>>
> > >>>> Ard, could you please take a look at this?
> > >>>>
> > >>>
> > >>> The issue seems to be that the caller expects iv_out to have been
> > >>> populated even when doing ciphertext stealing.
> > >>>
> > >>> This is meaningless, because the output IV can only be used to chain
> > >>> additional encrypted data if the split is at a block boundary. The
> > >>> ciphertext stealing fundamentally terminates the encryption, and
> > >>> produces a block of ciphertext that is shorter than the block size, so
> > >>> what the output IV should be is actually unspecified.
> > >>>
> > >>> IOW, test cases having plain/ciphertext vectors whose size is not a
> > >>> multiple of the cipher block size should not specify an expected value
> > >>> for the output IV.
> > >>
> > >> The test cases are extracted from RFC 3962 Appendix B. The
> > >> standard clearly expects there to be a non-zero next IV for
> > >> plaintext sizes that are not block-aligned.
> > >>
> > >
> > > OK, so this is the Kerberos V specific spec on how to use AES in CBC
> > > mode, which appears to describe how to chain multiple CBC encryptions
> > > together.
> > >
> > > CBC-CTS itself does not define this: the IV is an input only, and the
> > > reason we happen to return the IV is because a single CBC operation
> > > may be broken up into several ones, which can only be done on block
> > > boundaries. This is why CBC-CTS itself passes all its tests: a single
> > > CBC-CTS encryption only performs ciphertext stealing at the very end,
> > > and the next IV is never used in that case. (This is why the CTS mode
> > > tests in crypto/testmgr.h don't have iv_out vectors)
> > >
> > > Note that RFC3962 defines that the penultimate block of CBC-CTS
> > > ciphertext is used as the next IV. CBC returns the last ciphertext
> > > block as the output IV. It is a happy coincidence that the generic CTS
> > > template encapsulates CBC in a way where its output IV ends up in the
> > > right place.
> > >
> > >> Also, these test cases pass on other hardware platforms.
> > >>
> > >
> > > Fair enough.
> > >
> > > I am not opposed to fixing this, but given that it is the RFC3962 spec
> > > that defines that the next IV is the penultimate full block of the
> > > previous CBC-CTS ciphertext, we might consider doing the memcpy() in
> > > the Kerberos code not in the CBC-CTS implementations.
> >
> > Interesting thought. I'm all about proper layering, so I think this
> > is worth considering. Can you send an RFC patch?
> >
> 
> I'm not that familiar with kunit so this is just an off hand
> suggestion, but I imagine something like the below would suffice
> 
> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> @@ -639,6 +639,13 @@ gss_krb5_cts_crypt(struct crypto_sync_skcipher
> *cipher, struct xdr_buf *buf,
> 
>         ret = write_bytes_to_xdr_buf(buf, offset, data, len);
> 
> +       /*
> +        * CBC-CTS does not define an output IV but RFC 3962 defines it as the
> +        * penultimate block of ciphertext, so copy that into the IV buffer
> +        * before returning.
> +        */
> +       if (encrypt)
> +               memcpy(iv, data, crypto_sync_skcipher_ivsize(cipher));
>  out:
>         kfree(data);
>         return ret;
> 
Thanks, Ard.  That fixes it on aarch64 (also ran it on x86_64, ppc64le,
s390x, and aarch64 w/ 64k pages).  Could you send it as an official
patch and add

Tested-by: Scott Mayhew <smayhew@redhat.com>

-Scott
diff mbox series

Patch

diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
index 0e834a2c062c..477605fad76b 100644
--- a/arch/arm64/crypto/aes-modes.S
+++ b/arch/arm64/crypto/aes-modes.S
@@ -268,6 +268,7 @@  AES_FUNC_START(aes_cbc_cts_encrypt)
 	add		x4, x0, x4
 	st1		{v0.16b}, [x4]			/* overlapping stores */
 	st1		{v1.16b}, [x0]
+	st1		{v1.16b}, [x5]
 	ret
 AES_FUNC_END(aes_cbc_cts_encrypt)