Message ID | 20211021183028.837112-1-daniel.m.jordan@oracle.com (mailing list archive) |
---|---|
State | Accepted |
Delegated to: | Herbert Xu |
Headers | show |
Series | crypto: pcrypt - Delay write to padata->info | expand |
On Thu, Oct 21, 2021 at 02:30:28PM -0400, Daniel Jordan wrote: > These three events can race when pcrypt is used multiple times in a > template ("pcrypt(pcrypt(...))"): > > 1. [taskA] The caller makes the crypto request via crypto_aead_encrypt() > 2. [kworkerB] padata serializes the inner pcrypt request > 3. [kworkerC] padata serializes the outer pcrypt request > > 3 might finish before the call to crypto_aead_encrypt() returns in 1, > resulting in two possible issues. > > First, a use-after-free of the crypto request's memory when, for > example, taskA writes to the outer pcrypt request's padata->info in > pcrypt_aead_enc() after kworkerC completes the request. > > Second, the outer pcrypt request overwrites the inner pcrypt request's > return code with -EINPROGRESS, making a successful request appear to > fail. For instance, kworkerB writes the outer pcrypt request's > padata->info in pcrypt_aead_done() and then taskA overwrites it > in pcrypt_aead_enc(). > > Avoid both situations by delaying the write of padata->info until after > the inner crypto request's return code is checked. This prevents the > use-after-free by not touching the crypto request's memory after the > next-inner crypto request is made, and stops padata->info from being > overwritten. > > Fixes: 5068c7a883d16 ("crypto: pcrypt - Add pcrypt crypto parallelization wrapper") > Reported-by: syzbot+b187b77c8474f9648fae@syzkaller.appspotmail.com > Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com> > --- > > So, pcrypt(pcrypt(...)) doesn't make all that much sense, but apparently > there's not an easy way to prevent it [1], so I'm going with the minimal > fix. > > [1] https://lkml.kernel.org/20171230083744.vuclnbs677tj7pi2@gauss3.secunet.de/ > > crypto/pcrypt.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) Patch applied. Thanks.
diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c index d569c7ed6c80..9d10b846ccf7 100644 --- a/crypto/pcrypt.c +++ b/crypto/pcrypt.c @@ -78,12 +78,14 @@ static void pcrypt_aead_enc(struct padata_priv *padata) { struct pcrypt_request *preq = pcrypt_padata_request(padata); struct aead_request *req = pcrypt_request_ctx(preq); + int ret; - padata->info = crypto_aead_encrypt(req); + ret = crypto_aead_encrypt(req); - if (padata->info == -EINPROGRESS) + if (ret == -EINPROGRESS) return; + padata->info = ret; padata_do_serial(padata); } @@ -123,12 +125,14 @@ static void pcrypt_aead_dec(struct padata_priv *padata) { struct pcrypt_request *preq = pcrypt_padata_request(padata); struct aead_request *req = pcrypt_request_ctx(preq); + int ret; - padata->info = crypto_aead_decrypt(req); + ret = crypto_aead_decrypt(req); - if (padata->info == -EINPROGRESS) + if (ret == -EINPROGRESS) return; + padata->info = ret; padata_do_serial(padata); }
These three events can race when pcrypt is used multiple times in a template ("pcrypt(pcrypt(...))"): 1. [taskA] The caller makes the crypto request via crypto_aead_encrypt() 2. [kworkerB] padata serializes the inner pcrypt request 3. [kworkerC] padata serializes the outer pcrypt request 3 might finish before the call to crypto_aead_encrypt() returns in 1, resulting in two possible issues. First, a use-after-free of the crypto request's memory when, for example, taskA writes to the outer pcrypt request's padata->info in pcrypt_aead_enc() after kworkerC completes the request. Second, the outer pcrypt request overwrites the inner pcrypt request's return code with -EINPROGRESS, making a successful request appear to fail. For instance, kworkerB writes the outer pcrypt request's padata->info in pcrypt_aead_done() and then taskA overwrites it in pcrypt_aead_enc(). Avoid both situations by delaying the write of padata->info until after the inner crypto request's return code is checked. This prevents the use-after-free by not touching the crypto request's memory after the next-inner crypto request is made, and stops padata->info from being overwritten. Fixes: 5068c7a883d16 ("crypto: pcrypt - Add pcrypt crypto parallelization wrapper") Reported-by: syzbot+b187b77c8474f9648fae@syzkaller.appspotmail.com Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com> --- So, pcrypt(pcrypt(...)) doesn't make all that much sense, but apparently there's not an easy way to prevent it [1], so I'm going with the minimal fix. [1] https://lkml.kernel.org/20171230083744.vuclnbs677tj7pi2@gauss3.secunet.de/ crypto/pcrypt.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)