Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1565074510-8480-1-git-send-email-pvanleeuwen@verimatrix.com>
 <5bf9d0be-3ba4-8903-f1b9-93aa32106274@gmail.com> <MN2PR20MB29734CFE2795639436C3CC91CAD50@MN2PR20MB2973.namprd20.prod.outlook.com>
 <MN2PR20MB2973A38A300804281CA6A109CAD40@MN2PR20MB2973.namprd20.prod.outlook.com>
 <a0e3ce44-3e47-b8d9-2152-3fd8ba99f09a@gmail.com> <MN2PR20MB297333F0024F94C647D71AA2CAD40@MN2PR20MB2973.namprd20.prod.outlook.com>
 <52a11506-0047-a7e7-4fa0-ba8d465b843c@gmail.com> <MN2PR20MB2973C4EAF89D158B779CDBDACAD40@MN2PR20MB2973.namprd20.prod.outlook.com>
 <46f76b06-004e-c08a-3ef3-4ba9fdc61d91@gmail.com>
In-Reply-To: <46f76b06-004e-c08a-3ef3-4ba9fdc61d91@gmail.com>
From:   =?UTF-8?B?T25kcmVqIE1vc27DocSNZWs=?= <omosnacek@gmail.com>
Date:   Wed, 7 Aug 2019 22:32:35 +0200
Message-ID: <CAAUqJDuMUHqd4J7TNRbMiEDNeb_GCJPhJUQJoOJo5zXKmL72nQ@mail.gmail.com>
Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing
To:     Milan Broz <gmazyland@gmail.com>
Cc:     Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>,
        Pascal van Leeuwen <pascalvanl@gmail.com>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "rsnel@cube.dyndns.org" <rsnel@cube.dyndns.org>,
        "herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
        "davem@davemloft.net" <davem@davemloft.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

st 7. 8. 2019 o 19:44 Milan Broz <gmazyland@gmail.com> nap=C3=ADsal(a):
> On 07/08/2019 17:13, Pascal Van Leeuwen wrote:
> >>>> Seems there is no mistake in your code, it is some bug in aesni_inte=
l implementation.
> >>>> If I disable this module, it works as expected (with aes generic and=
 aes_i586).
> >>>>
> >>> That's odd though, considering there is a dedicated xts-aes-ni implem=
entation,
> >>> i.e. I would not expect that to end up at the generic xts wrapper at =
all?
> >>
> >> Note it is 32bit system, AESNI XTS is under #ifdef CONFIG_X86_64 so it=
 is not used.
> >>
> > Ok, so I guess no one bothered to make an optimized XTS version for i38=
6.
> > I quickly browsed through the code - took me a while to realise the ass=
embly is
> > "backwards" compared to the original Intel definition :-) - but I did n=
ot spot
> > anything obvious :-(
> >
> >> I guess it only ECB part ...
>
> Mystery solved, the skcipher subreq must be te last member in the struct.
> (Some comments in Adiantum code mentions it too, so I do not think it
> just hides the corruption after the struct. Seems like another magic requ=
irement
> in crypto API :-)

Oh, yes, this makes sense! I would have noticed this immediately if I
had looked carefully at the struct definition :) The reason is that
the skcipher_request struct is followed by a variable-length request
context. So when you want to nest requests, you need to make the
subrequest the last member and declare your request context size as:
size of your request context struct + size of the sub-algorithm's
request context.

It is a bit confusing, but it is the only reasonable way to support
variably sized context and at the same time keep the whole request in
a single allocation.

>
> This chunk is enough to fix it for me:
>
> --- a/crypto/xts.c
> +++ b/crypto/xts.c
> @@ -33,8 +33,8 @@ struct xts_instance_ctx {
>
>  struct rctx {
>         le128 t, tcur;
> -       struct skcipher_request subreq;
>         int rem_bytes, is_encrypt;
> +       struct skcipher_request subreq;
>  };
>
> While at it, shouldn't be is_encrypt bool?
>
> Thanks,
> Milan