2023-11-02 04:05:36

by Yuran Pereira

[permalink] [raw]
Subject: [PATCH 0/7] crypto: Proper Initialization of `struct skcipher_walk` in x86 Glue Files

In multiple `*_encrypt`, `*_crypt`, `*_decrypt` functions within the x86/crypto
glue files, the `skcipher_walk` structs being used are not properly initialized
prior their usage which can lead to undefined behaviour if the `flags` field of
this structure were to contain junk values at the time of its usage.

This patch series ensures that instances of `struct skcipher_walk` are correctly
initialized across different x86/crypto glue files.

Yuran Pereira (7):
crypto: Fixes uninitialized skcipher_walk use in sm4_aesni_avx_glue
crypto: Fixes uninitialized skcipher_walk use in des3_ede_glue
crypto: Fixes uninitialized skcipher_walk use in chacha_glue
crypto: Fixes uninitialized skcipher_walk use in aesni-intel_glue
crypto: Fixes uninitialized skcipher_walk use in aria_aesni_avx2_glue
crypto: Fixes uninitialized skcipher_walk use in aria_aesni_avx_glue
crypto: Fixes uninitialized skcipher_walk use in aria_gfni_avx512_glue

arch/x86/crypto/aesni-intel_glue.c | 12 ++++++++++++
arch/x86/crypto/aria_aesni_avx2_glue.c | 2 ++
arch/x86/crypto/aria_aesni_avx_glue.c | 2 ++
arch/x86/crypto/aria_gfni_avx512_glue.c | 2 ++
arch/x86/crypto/chacha_glue.c | 2 ++
arch/x86/crypto/des3_ede_glue.c | 4 ++++
arch/x86/crypto/sm4_aesni_avx_glue.c | 7 +++++++
7 files changed, 31 insertions(+)

--
2.25.1


2023-11-02 04:58:18

by Yuran Pereira

[permalink] [raw]
Subject: Re: [PATCH 0/7] crypto: Proper Initialization of `struct skcipher_walk` in x86 Glue Files

Hey Herbert,
On Thu, Nov 02, 2023 at 12:30:44PM +0800, Herbert Xu wrote:
> On Wed, Nov 01, 2023 at 09:20:43PM -0700, Eric Biggers wrote:
> >
> > Updating all callers of skcipher_walk_virt() seems like the wrong approach.
> > Shouldn't skcipher_walk_virt() be fixed to initialize the flags to 0 instead?
>
> The bits of the flags that are used are initialised in skcipher_walk_next.
>
I noticed that, but since skcipher_walk_first can return with failure, there seems
to be a chance that those bits are never initialized.
> > Also, does this fix affect any behavior, or is it just to fix a KMSAN warning?
> > It needs to be fixed either way, but it's helpful to understand the effect of
> > the fix so that people can decide whether it needs to be backported or not.
>
> Does this actually trigger a KMSAN warning? If so I'd like to see
> it. If it's just a static analyser then I'm not applying this.
No, there is no KMSAN warning. As I mentioned in the individual patches,
they're addressing "coverity" reports (so yes, static analyser).

Initially it did look like a false positive, but upon seeing that
skcipher_walk_first can return without ever calling skcipher_walk_next
I thought that there might be an off-chance that skcipher_walk_virt
returns without ever initializing those bits of the flag... hence this
patch set.

PS: I just saw Eric's reply,
> > Updating all callers of skcipher_walk_virt() seems like the wrong approach.

and realized that maybe my approach is in fact an overkill. Maybe simply initializing
the flags would indeed suffice.

Thanks,

2023-11-02 05:01:20

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH 0/7] crypto: Proper Initialization of `struct skcipher_walk` in x86 Glue Files

On Thu, Nov 02, 2023 at 10:27:41AM +0530, Yuran Pereira wrote:
>
> I noticed that, but since skcipher_walk_first can return with failure, there seems
> to be a chance that those bits are never initialized.

The API is such that if an error is returned by walk_first/next,
then you must not call into the skcipher_walk_* functions again.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt