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: <Y8kInrsuWybCTgK0@gondor.apana.org.au>
In-Reply-To: <Y8kInrsuWybCTgK0@gondor.apana.org.au>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Thu, 19 Jan 2023 10:50:26 +0100
Message-ID: <CAMj1kXGPqHsHSkj+hV_AcwPZxmWMi_=sVBHQWckUPomh6D7uGg@mail.gmail.com>
Subject: Re: [PATCH] crypto: xts - Handle EBUSY correctly
To:     Herbert Xu <herbert@gondor.apana.org.au>
Cc:     Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 19 Jan 2023 at 10:08, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> As it is xts only handles the special return value of EINPROGERSS,

EINPROGRESS

> which means that in all other cases it will free data related to the
> request.
>
> However, as the caller of xts may specify MAY_BACKLOG, we also need
> to expect EBUSY and treat it in the same way.  Otherwise backlogged
> requests will trigger a use-after-free.
>
> Fixes: 8083b1bf8163 ("crypto: xts - add support for ciphertext stealing")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/crypto/xts.c b/crypto/xts.c
> index 63c85b9e64e0..de6cbcf69bbd 100644
> --- a/crypto/xts.c
> +++ b/crypto/xts.c
> @@ -203,12 +203,12 @@ static void xts_encrypt_done(struct crypto_async_request *areq, int err)
>         if (!err) {
>                 struct xts_request_ctx *rctx = skcipher_request_ctx(req);
>
> -               rctx->subreq.base.flags &= ~CRYPTO_TFM_REQ_MAY_SLEEP;
> +               rctx->subreq.base.flags &= CRYPTO_TFM_REQ_MAY_BACKLOG;

I don't get this bit. We used to preserve CRYPTO_TFM_REQ_MAY_BACKLOG
before (along with all other flags except MAY_SLEEP), but now, we
*only* preserve CRYPTO_TFM_REQ_MAY_BACKLOG.

So how is this related? Why are we clearing
CRYPTO_TFM_REQ_FORBID_WEAK_KEYS here for instance?


>                 err = xts_xor_tweak_post(req, true);
>
>                 if (!err && unlikely(req->cryptlen % XTS_BLOCK_SIZE)) {
>                         err = xts_cts_final(req, crypto_skcipher_encrypt);
> -                       if (err == -EINPROGRESS)
> +                       if (err == -EINPROGRESS || err == -EBUSY)

Apologies for the noob questions about this aspect of the crypto API,
but I suppose this means that, if CRYPTO_TFM_REQ_MAY_BACKLOG was
specified, it is up to the skcipher implementation to queue up the
request and return -EBUSY, as opposed to starting the request
asynchronously and returning -EINPROGRESS?

So why the distinction? If the skcipher signals that the request is
accepted and will complete asynchronously, couldn't it use EINPROGRESS
for both cases? Or is the EBUSY interpreted differently by the caller,
for providing back pressure to the source?

>                                 return;
>                 }
>         }
> @@ -223,12 +223,12 @@ static void xts_decrypt_done(struct crypto_async_request *areq, int err)
>         if (!err) {
>                 struct xts_request_ctx *rctx = skcipher_request_ctx(req);
>
> -               rctx->subreq.base.flags &= ~CRYPTO_TFM_REQ_MAY_SLEEP;
> +               rctx->subreq.base.flags &= CRYPTO_TFM_REQ_MAY_BACKLOG;
>                 err = xts_xor_tweak_post(req, false);
>
>                 if (!err && unlikely(req->cryptlen % XTS_BLOCK_SIZE)) {
>                         err = xts_cts_final(req, crypto_skcipher_decrypt);
> -                       if (err == -EINPROGRESS)
> +                       if (err == -EINPROGRESS || err == -EBUSY)
>                                 return;
>                 }
>         }
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt