Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210512184439.8778-1-ardb@kernel.org> <20210512184439.8778-2-ardb@kernel.org>
 <YJw01Z3oxwY5Sfpa@gmail.com>
In-Reply-To: <YJw01Z3oxwY5Sfpa@gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Wed, 12 May 2021 23:24:09 +0200
Message-ID: <CAMj1kXHofDrzEs4qc8VNCLpyL-Hc4PSg-JXKTckJvfD6qoK78Q@mail.gmail.com>
Subject: Re: [PATCH v3 1/7] crypto: handle zero sized AEAD inputs correctly
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Will Deacon <will@kernel.org>,
        Android Kernel Team <kernel-team@android.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.