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

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

[Yuran Pereira]

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

[PATCH 1/7] crypto: Fixes uninitialized skcipher_walk use in sm4_aesni_avx_glue

[Yuran Pereira]

In the following functions:
 - `sm4_avx_ctr_crypt`
 - `sm4_avx_cfb_decrypt`
 - `sm4_cfb_encrypt`
 - `sm4_cbc_encrypt`
 - `sm4_avx_cbc_decrypt`
 - `ecb_do_crypt`

`struct skcipher_walk *walk` is not fully initialized before its use.

Although the call to `skcipher_walk_virt()` and subsequent functions
that this function calls seem to initialize some fields of this struct,

there is a chance that `skcipher_walk_virt()` returns
without fully clearing or properly initializing the `->flags` field
which means that the following flags:
`SKCIPHER_WALK_DIFF`, `SKCIPHER_WALK_COPY`, `SKCIPHER_WALK_SLOW`
could be storing junk values by the time `skcipher_walk_done()` is called.

This could lead to buggy or undefined behaviour since these flags
are checked in `skcipher_walk_done()`:

```C
int skcipher_walk_done(struct skcipher_walk *walk, int err)
{
	...
    if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
        SKCIPHER_WALK_SLOW |
        SKCIPHER_WALK_COPY |
        SKCIPHER_WALK_DIFF)))) {
    ...
}
```

To prevent this, this patch ensures that instances of `struct skcipher_walk`
are correctly initialized prior to their use.

Addresses-Coverity-IDs: 1491520, 1491533, 1491610, 1491651, 1491715,
                        1491774 ("Unintialized scalar variable")
Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
---
 arch/x86/crypto/sm4_aesni_avx_glue.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/crypto/sm4_aesni_avx_glue.c b/arch/x86/crypto/sm4_aesni_avx_glue.c
index 7800f77d68ad..4117c6f787e2 100644
--- a/arch/x86/crypto/sm4_aesni_avx_glue.c
+++ b/arch/x86/crypto/sm4_aesni_avx_glue.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/crypto.h>
 #include <linux/kernel.h>
+#include <linux/string.h>
 #include <asm/simd.h>
 #include <crypto/internal/simd.h>
 #include <crypto/internal/skcipher.h>
@@ -44,6 +45,7 @@ static int ecb_do_crypt(struct skcipher_request *req, const u32 *rkey)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
@@ -98,6 +100,7 @@ int sm4_cbc_encrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
@@ -132,6 +135,7 @@ int sm4_avx_cbc_decrypt(struct skcipher_request *req,
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
@@ -196,6 +200,7 @@ int sm4_cfb_encrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
@@ -238,6 +243,7 @@ int sm4_avx_cfb_decrypt(struct skcipher_request *req,
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
@@ -307,6 +313,7 @@ int sm4_avx_ctr_crypt(struct skcipher_request *req,
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
-- 
2.25.1

[PATCH 2/7] crypto: Fixes uninitialized skcipher_walk use in des3_ede_glue

[Yuran Pereira]

In the following functions:
 - `ecb_crypt`
 - `cbc_encrypt`
 - `cbc_decrypt`

`struct skcipher_walk *walk` is not fully initialized before its use.

Although the call to `skcipher_walk_virt()` and subsequent functions
that this function calls seem to initialize some fields of this struct,

there is a chance that `skcipher_walk_virt()` returns
without fully clearing or properly initializing the `->flags` field
which means that the following flags:
`SKCIPHER_WALK_DIFF`, `SKCIPHER_WALK_COPY`, `SKCIPHER_WALK_SLOW`
could be storing junk values by the time `skcipher_walk_done()` is called.

This could lead to buggy or undefined behaviour since these flags
are checked in `skcipher_walk_done()`:

```C
int skcipher_walk_done(struct skcipher_walk *walk, int err)
{
	...
    if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
        SKCIPHER_WALK_SLOW |
        SKCIPHER_WALK_COPY |
        SKCIPHER_WALK_DIFF)))) {
    ...
}
```

To prevent this, this patch ensures that instances of
`struct skcipher_walk` are correctly initialized prior to their use.

Addresses-Coverity-IDs: 1434673 ("Unintialized scalar variable")
Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
---
 arch/x86/crypto/des3_ede_glue.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/crypto/des3_ede_glue.c b/arch/x86/crypto/des3_ede_glue.c
index abb8b1fe123b..6f16b09b52fe 100644
--- a/arch/x86/crypto/des3_ede_glue.c
+++ b/arch/x86/crypto/des3_ede_glue.c
@@ -15,6 +15,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/string.h>
 
 struct des3_ede_x86_ctx {
 	struct des3_ede_ctx enc;
@@ -70,6 +71,7 @@ static int ecb_crypt(struct skcipher_request *req, const u32 *expkey)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes)) {
@@ -154,6 +156,7 @@ static int cbc_encrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while (walk.nbytes) {
@@ -233,6 +236,7 @@ static int cbc_decrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while (walk.nbytes) {
-- 
2.25.1

[PATCH 3/7] crypto: Fixes uninitialized skcipher_walk use in chacha_glue

[Yuran Pereira]

In `chacha_simd_stream_xor()`, `struct skcipher_walk *walk` is not
fully initialized before its use.

Although the call to `skcipher_walk_virt()` and subsequent functions
that this function calls seem to initialize some fields of this struct,

there is a chance that `skcipher_walk_virt()` returns
without fully clearing or properly initializing the `->flags` field
which means that the following flags:
`SKCIPHER_WALK_DIFF`, `SKCIPHER_WALK_COPY`, `SKCIPHER_WALK_SLOW`
could be storing junk values by the time `skcipher_walk_done()` is called.

This could lead to buggy or undefined behaviour since these flags
are checked in `skcipher_walk_done()`:

```C
int skcipher_walk_done(struct skcipher_walk *walk, int err)
{
	...
    if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
        SKCIPHER_WALK_SLOW |
        SKCIPHER_WALK_COPY |
        SKCIPHER_WALK_DIFF)))) {
    ...
}
```

To prevent this, this patch ensures that instances of
`struct skcipher_walk` are correctly initialized prior to their use.

Addresses-Coverity-IDs: 1456799 ("Unintialized scalar variable")
Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
---
 arch/x86/crypto/chacha_glue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/crypto/chacha_glue.c b/arch/x86/crypto/chacha_glue.c
index 7b3a1cf0984b..be8dc756d205 100644
--- a/arch/x86/crypto/chacha_glue.c
+++ b/arch/x86/crypto/chacha_glue.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/sizes.h>
+#include <linux/string.h>
 #include <asm/simd.h>
 
 asmlinkage void chacha_block_xor_ssse3(u32 *state, u8 *dst, const u8 *src,
@@ -167,6 +168,7 @@ static int chacha_simd_stream_xor(struct skcipher_request *req,
 	struct skcipher_walk walk;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	chacha_init_generic(state, ctx->key, iv);
-- 
2.25.1

[PATCH 4/7] crypto: Fixes uninitialized skcipher_walk use in aesni-intel_glue

[Yuran Pereira]

In the following functions:
 - `sm4_avx_ctr_crypt`
 - `ecb_encrypt`
 - `ecb_decrypt`
 - `cbc_encrypt`
 - `cbc_decrypt`
 - `cts_cbc_encrypt`
 - `cts_cbc_decrypt`
 - `ctr_crypt`
 - `xctr_crypt`
 - `xts_crypt`

`struct skcipher_walk *walk` is not fully initialized before its use.

Although the call to `skcipher_walk_virt()` and subsequent functions
that this function calls seem to initialize some fields of this struct,

there is a chance that `skcipher_walk_virt()` returns
without fully clearing or properly initializing the `->flags` field
which means that the following flags:
`SKCIPHER_WALK_DIFF`, `SKCIPHER_WALK_COPY`, `SKCIPHER_WALK_SLOW`
could be storing junk values by the time `skcipher_walk_done()` is called.

This could lead to buggy or undefined behaviour since these flags
are checked in `skcipher_walk_done()`:

```C
int skcipher_walk_done(struct skcipher_walk *walk, int err)
{
	...
    if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
        SKCIPHER_WALK_SLOW |
        SKCIPHER_WALK_COPY |
        SKCIPHER_WALK_DIFF)))) {
    ...
}
```

To prevent this, this patch ensures that instances of
`struct skcipher_walk` are correctly initialized prior to their use.

Addresses-Coverity-IDs: 139545, 1508669 ("Unintialized scalar variable")
Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
---
 arch/x86/crypto/aesni-intel_glue.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 39d6a62ac627..d2c5f00a33ff 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -35,6 +35,7 @@
 #include <linux/workqueue.h>
 #include <linux/spinlock.h>
 #include <linux/static_call.h>
+#include <linux/string.h>
 
 
 #define AESNI_ALIGN	16
@@ -296,6 +297,7 @@ static int ecb_encrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes)) {
@@ -318,6 +320,7 @@ static int ecb_decrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes)) {
@@ -340,6 +343,7 @@ static int cbc_encrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes)) {
@@ -362,6 +366,7 @@ static int cbc_decrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes)) {
@@ -387,6 +392,8 @@ static int cts_cbc_encrypt(struct skcipher_request *req)
 	struct skcipher_walk walk;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
+
 	skcipher_request_set_tfm(&subreq, tfm);
 	skcipher_request_set_callback(&subreq, skcipher_request_flags(req),
 				      NULL, NULL);
@@ -443,6 +450,8 @@ static int cts_cbc_decrypt(struct skcipher_request *req)
 	struct skcipher_walk walk;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
+
 	skcipher_request_set_tfm(&subreq, tfm);
 	skcipher_request_set_callback(&subreq, skcipher_request_flags(req),
 				      NULL, NULL);
@@ -515,6 +524,7 @@ static int ctr_crypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
@@ -566,6 +576,7 @@ static int xctr_crypt(struct skcipher_request *req)
 	int err;
 	__le32 block[AES_BLOCK_SIZE / sizeof(__le32)];
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
@@ -912,6 +923,7 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt)
 	if (req->cryptlen < AES_BLOCK_SIZE)
 		return -EINVAL;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 	if (!walk.nbytes)
 		return err;
-- 
2.25.1

[PATCH 5/7] crypto: Fixes uninitialized skcipher_walk use in aria_aesni_avx2_glue

[Yuran Pereira]

In `aria_avx2_ctr_encrypt()`, `struct skcipher_walk *walk` is not fully
initialized before its use.

Although the call to `skcipher_walk_virt()` and subsequent functions
that this function calls seem to initialize some fields of this struct,

there is a chance that `skcipher_walk_virt()` returns
without fully clearing or properly initializing the `->flags` field
which means that the following flags:
`SKCIPHER_WALK_DIFF`, `SKCIPHER_WALK_COPY`, `SKCIPHER_WALK_SLOW`
could be storing junk values by the time `skcipher_walk_done()` is called.

This could lead to buggy or undefined behaviour since these flags
are checked in `skcipher_walk_done()`:

```C
int skcipher_walk_done(struct skcipher_walk *walk, int err)
{
	...
    if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
        SKCIPHER_WALK_SLOW |
        SKCIPHER_WALK_COPY |
        SKCIPHER_WALK_DIFF)))) {
    ...
}
```

To prevent this, this patch ensures that instances of
`struct skcipher_walk` are correctly initialized prior to their use.

Addresses-Coverity-IDs: 1521842 ("Unintialized scalar variable")
Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
---
 arch/x86/crypto/aria_aesni_avx2_glue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/crypto/aria_aesni_avx2_glue.c b/arch/x86/crypto/aria_aesni_avx2_glue.c
index 87a11804fc77..d5a8077f9f96 100644
--- a/arch/x86/crypto/aria_aesni_avx2_glue.c
+++ b/arch/x86/crypto/aria_aesni_avx2_glue.c
@@ -12,6 +12,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/string.h>
 
 #include "ecb_cbc_helpers.h"
 #include "aria-avx.h"
@@ -94,6 +95,7 @@ static int aria_avx2_ctr_encrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
-- 
2.25.1

[PATCH 6/7] crypto: Fixes uninitialized skcipher_walk use in aria_aesni_avx_glue

[Yuran Pereira]

In `aria_avx_ctr_encrypt`, `struct skcipher_walk *walk` is not fully
initialized before its use.

Although the call to `skcipher_walk_virt()` and subsequent functions
that this function calls seem to initialize some fields of this struct,

there is a chance that `skcipher_walk_virt()` returns
without fully clearing or properly initializing the `->flags` field
which means that the following flags:
`SKCIPHER_WALK_DIFF`, `SKCIPHER_WALK_COPY`, `SKCIPHER_WALK_SLOW`
could be storing junk values by the time `skcipher_walk_done()` is called.

This could lead to buggy or undefined behaviour since these flags
are checked in `skcipher_walk_done()`:

```C
int skcipher_walk_done(struct skcipher_walk *walk, int err)
{
	...
    if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
        SKCIPHER_WALK_SLOW |
        SKCIPHER_WALK_COPY |
        SKCIPHER_WALK_DIFF)))) {
    ...
}
```

To prevent this, this patch ensures that instances of
`struct skcipher_walk` are correctly initialized prior to their use.

Addresses-Coverity-IDs: 1516269 ("Unintialized scalar variable")
Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
---
 arch/x86/crypto/aria_aesni_avx_glue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/crypto/aria_aesni_avx_glue.c b/arch/x86/crypto/aria_aesni_avx_glue.c
index 4e1516b76669..71589d58d0d3 100644
--- a/arch/x86/crypto/aria_aesni_avx_glue.c
+++ b/arch/x86/crypto/aria_aesni_avx_glue.c
@@ -12,6 +12,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/string.h>
 
 #include "ecb_cbc_helpers.h"
 #include "aria-avx.h"
@@ -92,6 +93,7 @@ static int aria_avx_ctr_encrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
-- 
2.25.1

[PATCH 7/7] crypto: Fixes uninitialized skcipher_walk use in aria_gfni_avx512_glue

[Yuran Pereira]

In `aria_avx512_ctr_encrypt`, `struct skcipher_walk *walk` is not fully
initialized before its use.

Although the call to `skcipher_walk_virt()` and subsequent functions
that this function calls seem to initialize some fields of this struct,

there is a chance that `skcipher_walk_virt()` returns
without fully clearing or properly initializing the `->flags` field
which means that the following flags:
`SKCIPHER_WALK_DIFF`, `SKCIPHER_WALK_COPY`, `SKCIPHER_WALK_SLOW`
could be storing junk values by the time `skcipher_walk_done()` is called.

This could lead to buggy or undefined behaviour since these flags
are checked in `skcipher_walk_done()`:

```C
int skcipher_walk_done(struct skcipher_walk *walk, int err)
{
	...
    if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
        SKCIPHER_WALK_SLOW |
        SKCIPHER_WALK_COPY |
        SKCIPHER_WALK_DIFF)))) {
    ...
}
```

To prevent this, this patch ensures that instances of
`struct skcipher_walk` are correctly initialized prior to their use.

Addresses-Coverity-IDs: 15916159 ("Unintialized scalar variable")
Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
---
 arch/x86/crypto/aria_gfni_avx512_glue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/crypto/aria_gfni_avx512_glue.c b/arch/x86/crypto/aria_gfni_avx512_glue.c
index f4a2208d2638..cf689cc880df 100644
--- a/arch/x86/crypto/aria_gfni_avx512_glue.c
+++ b/arch/x86/crypto/aria_gfni_avx512_glue.c
@@ -12,6 +12,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/string.h>
 
 #include "ecb_cbc_helpers.h"
 #include "aria-avx.h"
@@ -81,6 +82,7 @@ static int aria_avx512_ctr_encrypt(struct skcipher_request *req)
 	unsigned int nbytes;
 	int err;
 
+	memset(&walk, 0, sizeof(walk));
 	err = skcipher_walk_virt(&walk, req, false);
 
 	while ((nbytes = walk.nbytes) > 0) {
-- 
2.25.1

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

[Eric Biggers]

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

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

[Herbert Xu]

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

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

[Yuran Pereira]

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,

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

[Herbert Xu]

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