diff mbox series

[v3,1/7] crypto: handle zero sized AEAD inputs correctly

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

Commit Message

Ard Biesheuvel May 12, 2021, 6:44 p.m. UTC
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(+)

Comments

Eric Biggers May 12, 2021, 8:04 p.m. UTC | #1
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
Ard Biesheuvel May 12, 2021, 9:24 p.m. UTC | #2
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.
Herbert Xu May 21, 2021, 7:55 a.m. UTC | #3
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,
Ard Biesheuvel May 21, 2021, 9:28 a.m. UTC | #4
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 mbox series

Patch

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;