Message ID | Y8kInrsuWybCTgK0@gondor.apana.org.au (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Herbert Xu |
Headers | show |
Series | crypto: xts - Handle EBUSY correctly | expand |
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
On Thu, Jan 19, 2023 at 10:50:26AM +0100, Ard Biesheuvel wrote: > 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 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,
On Thu, 19 Jan 2023 at 10:58, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Thu, Jan 19, 2023 at 10:50:26AM +0100, Ard Biesheuvel wrote: > > 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 > > 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 <ardb@kernel.org>
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; } }
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 <herbert@gondor.apana.org.au>