Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221004044912.24770-1-ap420073@gmail.com> <Yzu8Kd2botr3eegj@gondor.apana.org.au>
 <f7c52ed1-8061-8147-f676-86190118cc56@gmail.com> <MW5PR84MB18420D6E1A31D9C765EF6ED4AB5A9@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
In-Reply-To: <MW5PR84MB18420D6E1A31D9C765EF6ED4AB5A9@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Fri, 7 Oct 2022 23:54:49 +0200
Message-ID: <CAMj1kXGmKunh-OCvGFf8T6KJJXSHRYzacjSojBD3__u0o-3D1w@mail.gmail.com>
Subject: Re: [PATCH] crypto: x86: Do not acquire fpu context for too long
To:     "Elliott, Robert (Servers)" <elliott@hpe.com>
Cc:     Taehee Yoo <ap420073@gmail.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "x86@kernel.org" <x86@kernel.org>, "hpa@zytor.com" <hpa@zytor.com>,
        "ebiggers@google.com" <ebiggers@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 4 Oct 2022 at 17:17, Elliott, Robert (Servers) <elliott@hpe.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Taehee Yoo <ap420073@gmail.com>
> > Sent: Tuesday, October 4, 2022 1:02 AM
> > To: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: linux-crypto@vger.kernel.org; davem@davemloft.net; tglx@linutronix.de;
> > mingo@redhat.com; bp@alien8.de; dave.hansen@linux.intel.com; x86@kernel.org;
> > hpa@zytor.com; ardb@kernel.org; ebiggers@google.com
> > Subject: Re: [PATCH] crypto: x86: Do not acquire fpu context for too long
> >
> > Hi Herbert,
> > Thanks a lot for your review!
> >
> > On 10/4/22 13:52, Herbert Xu wrote:
> >  > On Tue, Oct 04, 2022 at 04:49:12AM +0000, Taehee Yoo wrote:
> >  >>
> >  >>   #define ECB_WALK_START(req, bsize, fpu_blocks) do {                     \
> >  >>           void *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req));   \
> >  >> + unsigned int walked_bytes = 0;                                  \
> >  >>           const int __bsize = (bsize);                                    \
> >  >>           struct skcipher_walk walk;                                      \
> >  >> - int err = skcipher_walk_virt(&walk, (req), false);              \
> >  >> + int err;                                                        \
> >  >> +                                                                 \
> >  >> + err = skcipher_walk_virt(&walk, (req), false);                  \
> >  >>           while (walk.nbytes > 0) {                                       \
> >  >> -         unsigned int nbytes = walk.nbytes;                      \
> >  >> -         bool do_fpu = (fpu_blocks) != -1 &&                     \
> >  >> -                       nbytes >= (fpu_blocks) * __bsize;         \
> >  >>                   const u8 *src = walk.src.virt.addr;                     \
> >  >> -         u8 *dst = walk.dst.virt.addr;                           \
> >  >>                   u8 __maybe_unused buf[(bsize)];                         \
> >  >> -         if (do_fpu) kernel_fpu_begin()
> >  >> +         u8 *dst = walk.dst.virt.addr;                           \
> >  >> +         unsigned int nbytes;                                    \
> >  >> +         bool do_fpu;                                            \
> >  >> +                                                                 \
> >  >> +         if (walk.nbytes - walked_bytes > ECB_CBC_WALK_MAX) {    \
> >  >> +                 nbytes = ECB_CBC_WALK_MAX;                      \
> >  >> +                 walked_bytes += ECB_CBC_WALK_MAX;               \
> >  >> +         } else {                                                \
> >  >> +                 nbytes = walk.nbytes - walked_bytes;            \
> >  >> +                 walked_bytes = walk.nbytes;                     \
> >  >> +         }                                                       \
> >  >> +                                                                 \
> >  >> +         do_fpu = (fpu_blocks) != -1 &&                          \
> >  >> +                  nbytes >= (fpu_blocks) * __bsize;              \
> >  >> +         if (do_fpu)                                             \
> >  >> +                 kernel_fpu_begin()
> >  >>
> >  >>   #define CBC_WALK_START(req, bsize, fpu_blocks)                          \
> >  >>           ECB_WALK_START(req, bsize, fpu_blocks)
> >  >> @@ -65,8 +81,12 @@
> >  >>   } while (0)
> >  >>
> >  >>   #define ECB_WALK_END()                                                  \
> >  >> -         if (do_fpu) kernel_fpu_end();                           \
> >  >> +         if (do_fpu)                                             \
> >  >> +                 kernel_fpu_end();                               \
> >  >> +         if (walked_bytes < walk.nbytes)                         \
> >  >> +                 continue;                                       \
> >  >>                   err = skcipher_walk_done(&walk, nbytes);                \
> >  >> +         walked_bytes = 0;                                       \
> >  >>           }                                                               \
> >  >>           return err;                                                     \
> >  >>   } while (0)
> >  >
> >  > skcipher_walk_* is supposed to return at most a page.  Why is this
> >  > necessary?
> >  >
> >  > Cheers,
> >
> > I referred to below link.
> > https://lore.kernel.org/all/MW5PR84MB18426EBBA3303770A8BC0BDFAB759@MW5PR84MB18
> > 42.NAMPRD84.PROD.OUTLOOK.COM/
> >
> > Sorry for that I didn't check that skcipher_walk_* returns only under 4K
> > sizes.
> > So, I thought fpu context would be too long.
> > But, I just checked the skcipher_walk_*, and it's right, it returns
> > under 4K sizes.
> > So, there are no problems as you mentioned.
> >
> > Thank you so much!
> > Taehee Yoo
>
> I think functions using the ECB and CBC macros (and
> those helper functions) are safe  - notice the third
> argument is called fpu_blocks. So, Aria's ECB mode is
> probably safe. There are no CTR macros, so that needs
> to be checked more carefully.
>
> We need to check all the functions that don't use the
> macros and functions. The SHA functions (I've started
> working on a patch for those).
>
> Running modprobe tcrypt mode=0, I encountered RCU stalls
> on these:
>         tcrypt: testing encryption speed of sync skcipher cts(cbc(aes)) using cts(cbc(aes-aesni))
>         tcrypt: testing encryption speed of sync skcipher cfb(aes) using cfb(aes-aesni)
>
> aesni-intel_glue.c registers "__cts(cbs(aes))", not "cts(cbc(aes-aesni)",
> and doesn't register any cfb algorithms, so those tests are using the
> generic templates, which must not be breaking up the loops as needed.
>

These all use the aes-aesni cipher wrapped in various layers of
generic code. The core cipher puts kernel_fpu_begin/end around every
AES block {16 bytes) it processes, so I doubt that the crypto code is
at fault here, unless the issues is in tcrypt itself.