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(+)
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
Hi Yuran, On Thu, Nov 02, 2023 at 09:34:08AM +0530, Yuran Pereira wrote: > 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 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? 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. - Eric
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. > 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. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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,
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 <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
© 2016 - 2025 Red Hat, Inc.