As it is xts only handles the special return value of EINPROGERSS,
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 <[email protected]>
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;
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)
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, Jan 19, 2023 at 10:50:26AM +0100, Ard Biesheuvel wrote:
> On Thu, 19 Jan 2023 at 10:08, Herbert Xu <[email protected]> wrote:
> >
> > As it is xts only handles the special return value of EINPROGERSS,
>
> EINPROGRESS
Thanks, I will fix this.
> > - 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.
This change is just in case we introduce any more flags in
future that we may not wish to pass along (as this code knows
nothing about it).
> So how is this related? Why are we clearing
> CRYPTO_TFM_REQ_FORBID_WEAK_KEYS here for instance?
WEAK_KEYS is only defined for setkey. Only MAY_SLEEP and MAY_BACKLOG
are currently meaningful for encryption and decryption.
> 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?
EBUSY signals to the caller that it must back off and not issue
any more requests. The caller should wait for a completion call
with EINPROGRESS to indicate that it may issue new requests.
For xts we essentially ignore EBUSY at this point, and assume that
if our own caller issued any more requests it would directly get
an EBUSY which should be sufficient to avoid total collapse.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, 19 Jan 2023 at 10:58, Herbert Xu <[email protected]> wrote:
>
> On Thu, Jan 19, 2023 at 10:50:26AM +0100, Ard Biesheuvel wrote:
> > On Thu, 19 Jan 2023 at 10:08, Herbert Xu <[email protected]> wrote:
> > >
> > > As it is xts only handles the special return value of EINPROGERSS,
> >
> > EINPROGRESS
>
> Thanks, I will fix this.
>
> > > - 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.
>
> This change is just in case we introduce any more flags in
> future that we may not wish to pass along (as this code knows
> nothing about it).
>
> > So how is this related? Why are we clearing
> > CRYPTO_TFM_REQ_FORBID_WEAK_KEYS here for instance?
>
> WEAK_KEYS is only defined for setkey. Only MAY_SLEEP and MAY_BACKLOG
> are currently meaningful for encryption and decryption.
>
Fair enough.
> > 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?
>
> EBUSY signals to the caller that it must back off and not issue
> any more requests. The caller should wait for a completion call
> with EINPROGRESS to indicate that it may issue new requests.
>
> For xts we essentially ignore EBUSY at this point, and assume that
> if our own caller issued any more requests it would directly get
> an EBUSY which should be sufficient to avoid total collapse.
>
Ah right - the request is only split across two calls into the
underlying skcipher if CTS is needed, and otherwise, we just forward
whatever non-zero return value we received.
Thanks for the explanation.
Acked-by: Ard Biesheuvel <[email protected]>
On Thu, 19 Jan 2023 at 10:08, Herbert Xu <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
v2 fixes a typo in the commit message.
---8<---
As it is xts only handles the special return value of 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 <[email protected]>
Acked-by: Ard Biesheuvel <[email protected]>
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;
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)
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt