| Message ID | 20210512184439.8778-2-ardb@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | running kernel mode SIMD with softirqs disabled | expand |
On Wed, May 12, 2021 at 08:44:33PM +0200, Ard Biesheuvel wrote: > There are corner cases where skcipher_walk_aead_[en|de]crypt() may be > invoked with a zero sized input, which is not rejected by the walker > code, but results in the skcipher_walk structure to not be fully > initialized. This will leave stale values in its page and buffer > members, which will be subsequently passed to kfree() or free_page() by > skcipher_walk_done(), resulting in a crash if those routines fail to > identify them as in valid inputs. > > Fix this by setting page and buffer to NULL even if the size of the > input is zero. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Is this fixing an existing bug, or only a bug that got exposed by this patchset? It would be helpful to make that clear (and if it fixes an existing bug, include a Fixes tag). Also, skcipher_walk_virt() doesn't set page and buffer to NULL, as it is currently expected that skcipher_walk_done() is only called when walk.nbytes != 0. Is something different for skcipher_walk_aead_[en|de]crypt()? - Eric
On Wed, 12 May 2021 at 22:04, Eric Biggers <ebiggers@kernel.org> wrote: > > On Wed, May 12, 2021 at 08:44:33PM +0200, Ard Biesheuvel wrote: > > There are corner cases where skcipher_walk_aead_[en|de]crypt() may be > > invoked with a zero sized input, which is not rejected by the walker > > code, but results in the skcipher_walk structure to not be fully > > initialized. This will leave stale values in its page and buffer > > members, which will be subsequently passed to kfree() or free_page() by > > skcipher_walk_done(), resulting in a crash if those routines fail to > > identify them as in valid inputs. > > > > Fix this by setting page and buffer to NULL even if the size of the > > input is zero. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Is this fixing an existing bug, or only a bug that got exposed by this patchset? > It would be helpful to make that clear (and if it fixes an existing bug, include > a Fixes tag). > The CCM change in the last patch uncovers this issue, and I don't think it is likely we would ever hit it anywhere else. > Also, skcipher_walk_virt() doesn't set page and buffer to NULL, as it is > currently expected that skcipher_walk_done() is only called when > walk.nbytes != 0. Is something different for skcipher_walk_aead_[en|de]crypt()? > The difference is that zero sized inputs never make sense for skciphers, but for AEADs, they could occur, even if they are uncommon (the AEAD could have associated data only, and no plain/ciphertext) But in the general case, I would assume that skcipher_walk_done() can be called on a walk that was successfully started with skcipher_walk_virt() without crashing, even if the scatterlist has size zero, so perhaps we should fix that one as well.
On Wed, May 12, 2021 at 11:24:09PM +0200, Ard Biesheuvel wrote: > > The difference is that zero sized inputs never make sense for > skciphers, but for AEADs, they could occur, even if they are uncommon > (the AEAD could have associated data only, and no plain/ciphertext) I don't see what a zero-sized input has to do with this though. When the walk->nbytes is zero, that means that you must never call the done function, because the walk state could be in error in which case everything would have been freed already and calling the done function may potentially cause a double-free. I don't understand why in the case of AEAD you cannot structure your code such that the done function is not called when nbytes is zero. Cheers,
On Fri, 21 May 2021 at 09:55, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Wed, May 12, 2021 at 11:24:09PM +0200, Ard Biesheuvel wrote: > > > > The difference is that zero sized inputs never make sense for > > skciphers, but for AEADs, they could occur, even if they are uncommon > > (the AEAD could have associated data only, and no plain/ciphertext) > > I don't see what a zero-sized input has to do with this though. > When the walk->nbytes is zero, that means that you must never > call the done function, because the walk state could be in error > in which case everything would have been freed already and calling > the done function may potentially cause a double-free. > > I don't understand why in the case of AEAD you cannot structure > your code such that the done function is not called when nbytes > is zero. > OK.
diff --git a/crypto/skcipher.c b/crypto/skcipher.c index a15376245416..93fdacf49697 100644 --- a/crypto/skcipher.c +++ b/crypto/skcipher.c @@ -511,6 +511,8 @@ static int skcipher_walk_aead_common(struct skcipher_walk *walk, walk->nbytes = 0; walk->iv = req->iv; walk->oiv = req->iv; + walk->buffer = NULL; + walk->page = NULL; if (unlikely(!walk->total)) return 0;
There are corner cases where skcipher_walk_aead_[en|de]crypt() may be invoked with a zero sized input, which is not rejected by the walker code, but results in the skcipher_walk structure to not be fully initialized. This will leave stale values in its page and buffer members, which will be subsequently passed to kfree() or free_page() by skcipher_walk_done(), resulting in a crash if those routines fail to identify them as in valid inputs. Fix this by setting page and buffer to NULL even if the size of the input is zero. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- crypto/skcipher.c | 2 ++ 1 file changed, 2 insertions(+)