[PATCH 1/2] crypto: drivers - avoid memcpy size warning

[PATCH 1/2] crypto: drivers - avoid memcpy size warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Some configurations with gcc-12 or gcc-13 produce a warning for the source
and destination of a memcpy() in atmel_sha_hmac_compute_ipad_hash() potentially
overlapping:

In file included from include/linux/string.h:254,
                 from drivers/crypto/atmel-sha.c:15:
drivers/crypto/atmel-sha.c: In function 'atmel_sha_hmac_compute_ipad_hash':
include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 408 and 280 overlaps 1 or more bytes at offset 408 [-Werror=restrict]
   57 | #define __underlying_memcpy     __builtin_memcpy
      |                                 ^
include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy'
  648 |         __underlying_##op(p, q, __fortify_size);                        \
      |         ^~~~~~~~~~~~~
include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk'
  693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
      |                          ^~~~~~~~~~~~~~~~~~~~
drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro 'memcpy'
 1773 |         memcpy(hmac->opad, hmac->ipad, bs);
      |         ^~~~~~

The same thing happens in two more drivers that have the same logic:

drivers/crypto/chelsio/chcr_algo.c: In function 'chcr_ahash_setkey':
include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 260 and 132 overlaps 1 or more bytes at offset 260 [-Werror=restrict]
drivers/crypto/bcm/cipher.c: In function 'ahash_hmac_setkey':
include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing between 129 and 4294967295 bytes at offsets 840 and 712 overlaps between 1 and 4294967167 bytes at offset 840 [-Werror=restrict]

I don't think it can actually happen because the size is strictly bounded
to the available block sizes, at most 128 bytes, though inlining decisions
could lead gcc to not see that.

Add an explicit size check to make sure gcc also sees this function is safe
regardless of inlining.

Note that the -Wrestrict warning is currently disabled by default, but it
would be nice to finally enable it, and these are the only false
postives that I see at the moment. There are 9 other crypto drivers that
also use an identical memcpy() but don't show up in randconfig build
warnings for me, presumably because of different inlining decisions.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/crypto/atmel-sha.c         | 3 +++
 drivers/crypto/bcm/cipher.c        | 3 +++
 drivers/crypto/chelsio/chcr_algo.c | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
index f2031f934be95..52a3c81b3a05a 100644
--- a/drivers/crypto/atmel-sha.c
+++ b/drivers/crypto/atmel-sha.c
@@ -1770,6 +1770,9 @@ static int atmel_sha_hmac_compute_ipad_hash(struct atmel_sha_dev *dd)
 	size_t bs = ctx->block_size;
 	size_t i, num_words = bs / sizeof(u32);
 
+	if (bs > sizeof(hmac->opad))
+		return -EINVAL;
+
 	memcpy(hmac->opad, hmac->ipad, bs);
 	for (i = 0; i < num_words; ++i) {
 		hmac->ipad[i] ^= 0x36363636;
diff --git a/drivers/crypto/bcm/cipher.c b/drivers/crypto/bcm/cipher.c
index 70b911baab26d..8633ca0286a10 100644
--- a/drivers/crypto/bcm/cipher.c
+++ b/drivers/crypto/bcm/cipher.c
@@ -2327,6 +2327,9 @@ static int ahash_hmac_setkey(struct crypto_ahash *ahash, const u8 *key,
 		 __func__, ahash, key, keylen, blocksize, digestsize);
 	flow_dump("  key: ", key, keylen);
 
+	if (blocksize > sizeof(ctx->opad))
+		return -EINVAL;
+
 	if (keylen > blocksize) {
 		switch (ctx->auth.alg) {
 		case HASH_ALG_MD5:
diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
index 0eade4fa6695b..5c8e10ee010ff 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -2201,6 +2201,9 @@ static int chcr_ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
 
 	SHASH_DESC_ON_STACK(shash, hmacctx->base_hash);
 
+	if (bs > sizeof(hmacctx->opad))
+		return -EINVAL;
+
 	/* use the key to calculate the ipad and opad. ipad will sent with the
 	 * first request's data. opad will be sent with the final hash result
 	 * ipad in hmacctx->ipad and opad in hmacctx->opad location
-- 
2.39.2

[PATCH 2/2] [v2] kallsyms: rework symbol lookup return codes

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Building with W=1 in some configurations produces a false positive
warning for kallsyms:

kernel/kallsyms.c: In function '__sprint_symbol.isra':
kernel/kallsyms.c:503:17: error: 'strcpy' source argument is the same as destination [-Werror=restrict]
  503 |                 strcpy(buffer, name);
      |                 ^~~~~~~~~~~~~~~~~~~~

This originally showed up while building with -O3, but later started
happening in other configurations as well, depending on inlining
decisions. The underlying issue is that the local 'name' variable is
always initialized to the be the same as 'buffer' in the called functions
that fill the buffer, which gcc notices while inlining, though it could
see that the address check always skips the copy.

The calling conventions here are rather unusual, as all of the internal
lookup functions (bpf_address_lookup, ftrace_mod_address_lookup,
ftrace_func_address_lookup, module_address_lookup and
kallsyms_lookup_buildid) already use the provided buffer and either return
the address of that buffer to indicate success, or NULL for failure,
but the callers are written to also expect an arbitrary other buffer
to be returned.

Rework the calling conventions to return the length of the filled buffer
instead of its address, which is simpler and easier to follow as well
as avoiding the warning. Leave only the kallsyms_lookup() calling conventions
unchanged, since that is called from 16 different functions and
adapting this would be a much bigger change.

Link: https://lore.kernel.org/all/20200107214042.855757-1-arnd@arndb.de/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: complete rewrite after the first patch was rejected (in 2020). This
    is now one of only two warnings that are in the way of enabling
    -Wextra/-Wrestrict by default.
---
 include/linux/filter.h   | 14 +++++++-------
 include/linux/ftrace.h   |  6 +++---
 include/linux/module.h   | 14 +++++++-------
 kernel/bpf/core.c        |  7 +++----
 kernel/kallsyms.c        | 23 ++++++++++++-----------
 kernel/module/kallsyms.c | 26 +++++++++++++-------------
 kernel/trace/ftrace.c    | 13 +++++--------
 7 files changed, 50 insertions(+), 53 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index f69114083ec71..10f2b1acb138b 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1130,17 +1130,17 @@ static inline bool bpf_jit_kallsyms_enabled(void)
 	return false;
 }
 
-const char *__bpf_address_lookup(unsigned long addr, unsigned long *size,
+int __bpf_address_lookup(unsigned long addr, unsigned long *size,
 				 unsigned long *off, char *sym);
 bool is_bpf_text_address(unsigned long addr);
 int bpf_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 		    char *sym);
 
-static inline const char *
+static inline int
 bpf_address_lookup(unsigned long addr, unsigned long *size,
 		   unsigned long *off, char **modname, char *sym)
 {
-	const char *ret = __bpf_address_lookup(addr, size, off, sym);
+	int ret = __bpf_address_lookup(addr, size, off, sym);
 
 	if (ret && modname)
 		*modname = NULL;
@@ -1184,11 +1184,11 @@ static inline bool bpf_jit_kallsyms_enabled(void)
 	return false;
 }
 
-static inline const char *
+static inline int
 __bpf_address_lookup(unsigned long addr, unsigned long *size,
 		     unsigned long *off, char *sym)
 {
-	return NULL;
+	return 0;
 }
 
 static inline bool is_bpf_text_address(unsigned long addr)
@@ -1202,11 +1202,11 @@ static inline int bpf_get_kallsym(unsigned int symnum, unsigned long *value,
 	return -ERANGE;
 }
 
-static inline const char *
+static inline int
 bpf_address_lookup(unsigned long addr, unsigned long *size,
 		   unsigned long *off, char **modname, char *sym)
 {
-	return NULL;
+	return 0;
 }
 
 static inline void bpf_prog_kallsyms_add(struct bpf_prog *fp)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index ce156c7704ee5..50c7ca7125caa 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -87,15 +87,15 @@ struct ftrace_direct_func;
 
 #if defined(CONFIG_FUNCTION_TRACER) && defined(CONFIG_MODULES) && \
 	defined(CONFIG_DYNAMIC_FTRACE)
-const char *
+int
 ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
 		   unsigned long *off, char **modname, char *sym);
 #else
-static inline const char *
+static inline int
 ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
 		   unsigned long *off, char **modname, char *sym)
 {
-	return NULL;
+	return 0;
 }
 #endif
 
diff --git a/include/linux/module.h b/include/linux/module.h
index a98e188cf37b8..76e6104d41ba5 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -920,11 +920,11 @@ int module_kallsyms_on_each_symbol(const char *modname,
  * least KSYM_NAME_LEN long: a pointer to namebuf is returned if
  * found, otherwise NULL.
  */
-const char *module_address_lookup(unsigned long addr,
-				  unsigned long *symbolsize,
-				  unsigned long *offset,
-				  char **modname, const unsigned char **modbuildid,
-				  char *namebuf);
+int module_address_lookup(unsigned long addr,
+			  unsigned long *symbolsize,
+			  unsigned long *offset,
+			  char **modname, const unsigned char **modbuildid,
+			  char *namebuf);
 int lookup_module_symbol_name(unsigned long addr, char *symname);
 int lookup_module_symbol_attrs(unsigned long addr,
 			       unsigned long *size,
@@ -953,14 +953,14 @@ static inline int module_kallsyms_on_each_symbol(const char *modname,
 }
 
 /* For kallsyms to ask for address resolution.  NULL means not found. */
-static inline const char *module_address_lookup(unsigned long addr,
+static inline int module_address_lookup(unsigned long addr,
 						unsigned long *symbolsize,
 						unsigned long *offset,
 						char **modname,
 						const unsigned char **modbuildid,
 						char *namebuf)
 {
-	return NULL;
+	return 0;
 }
 
 static inline int lookup_module_symbol_name(unsigned long addr, char *symname)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ea371f790f47c..da0590e5c87e1 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -688,11 +688,11 @@ static struct bpf_ksym *bpf_ksym_find(unsigned long addr)
 	return n ? container_of(n, struct bpf_ksym, tnode) : NULL;
 }
 
-const char *__bpf_address_lookup(unsigned long addr, unsigned long *size,
+int __bpf_address_lookup(unsigned long addr, unsigned long *size,
 				 unsigned long *off, char *sym)
 {
 	struct bpf_ksym *ksym;
-	char *ret = NULL;
+	int ret = 0;
 
 	rcu_read_lock();
 	ksym = bpf_ksym_find(addr);
@@ -700,9 +700,8 @@ const char *__bpf_address_lookup(unsigned long addr, unsigned long *size,
 		unsigned long symbol_start = ksym->start;
 		unsigned long symbol_end = ksym->end;
 
-		strncpy(sym, ksym->name, KSYM_NAME_LEN);
+		ret = strlcpy(sym, ksym->name, KSYM_NAME_LEN);
 
-		ret = sym;
 		if (size)
 			*size = symbol_end - symbol_start;
 		if (off)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 016d997131d43..cd216ebb14cc3 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -399,12 +399,12 @@ int kallsyms_lookup_size_offset(unsigned long addr, unsigned long *symbolsize,
 	       !!__bpf_address_lookup(addr, symbolsize, offset, namebuf);
 }
 
-static const char *kallsyms_lookup_buildid(unsigned long addr,
+static int kallsyms_lookup_buildid(unsigned long addr,
 			unsigned long *symbolsize,
 			unsigned long *offset, char **modname,
 			const unsigned char **modbuildid, char *namebuf)
 {
-	const char *ret;
+	int ret;
 
 	namebuf[KSYM_NAME_LEN - 1] = 0;
 	namebuf[0] = 0;
@@ -421,7 +421,7 @@ static const char *kallsyms_lookup_buildid(unsigned long addr,
 		if (modbuildid)
 			*modbuildid = NULL;
 
-		ret = namebuf;
+		ret = strlen(namebuf);
 		goto found;
 	}
 
@@ -453,8 +453,13 @@ const char *kallsyms_lookup(unsigned long addr,
 			    unsigned long *offset,
 			    char **modname, char *namebuf)
 {
-	return kallsyms_lookup_buildid(addr, symbolsize, offset, modname,
-				       NULL, namebuf);
+	int ret = kallsyms_lookup_buildid(addr, symbolsize, offset, modname,
+					  NULL, namebuf);
+
+	if (!ret)
+		return NULL;
+
+	return namebuf;
 }
 
 int lookup_symbol_name(unsigned long addr, char *symname)
@@ -489,19 +494,15 @@ static int __sprint_symbol(char *buffer, unsigned long address,
 {
 	char *modname;
 	const unsigned char *buildid;
-	const char *name;
 	unsigned long offset, size;
 	int len;
 
 	address += symbol_offset;
-	name = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid,
+	len = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid,
 				       buffer);
-	if (!name)
+	if (!len)
 		return sprintf(buffer, "0x%lx", address - symbol_offset);
 
-	if (name != buffer)
-		strcpy(buffer, name);
-	len = strlen(buffer);
 	offset -= symbol_offset;
 
 	if (add_offset)
diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index ef73ae7c89094..d6a74a5e1604f 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -321,14 +321,15 @@ void * __weak dereference_module_function_descriptor(struct module *mod,
  * For kallsyms to ask for address resolution.  NULL means not found.  Careful
  * not to lock to avoid deadlock on oopses, simply disable preemption.
  */
-const char *module_address_lookup(unsigned long addr,
-				  unsigned long *size,
-			    unsigned long *offset,
-			    char **modname,
-			    const unsigned char **modbuildid,
-			    char *namebuf)
+int module_address_lookup(unsigned long addr,
+			  unsigned long *size,
+			  unsigned long *offset,
+			  char **modname,
+			  const unsigned char **modbuildid,
+			  char *namebuf)
 {
-	const char *ret = NULL;
+	const char *sym;
+	int ret = 0;
 	struct module *mod;
 
 	preempt_disable();
@@ -344,13 +345,12 @@ const char *module_address_lookup(unsigned long addr,
 #endif
 		}
 
-		ret = find_kallsyms_symbol(mod, addr, size, offset);
-	}
-	/* Make a copy in here where it's safe */
-	if (ret) {
-		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
-		ret = namebuf;
+		sym = find_kallsyms_symbol(mod, addr, size, offset);
+
+		if (sym)
+			ret = strlcpy(namebuf, sym, KSYM_NAME_LEN - 1);
 	}
+
 	preempt_enable();
 
 	return ret;
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 05c0024815bf9..bc0eed24a5873 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6965,7 +6965,7 @@ allocate_ftrace_mod_map(struct module *mod,
 	return mod_map;
 }
 
-static const char *
+static int
 ftrace_func_address_lookup(struct ftrace_mod_map *mod_map,
 			   unsigned long addr, unsigned long *size,
 			   unsigned long *off, char *sym)
@@ -6986,21 +6986,18 @@ ftrace_func_address_lookup(struct ftrace_mod_map *mod_map,
 			*size = found_func->size;
 		if (off)
 			*off = addr - found_func->ip;
-		if (sym)
-			strscpy(sym, found_func->name, KSYM_NAME_LEN);
-
-		return found_func->name;
+		return strlcpy(sym, found_func->name, KSYM_NAME_LEN);
 	}
 
-	return NULL;
+	return 0;
 }
 
-const char *
+int
 ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
 		   unsigned long *off, char **modname, char *sym)
 {
 	struct ftrace_mod_map *mod_map;
-	const char *ret = NULL;
+	int ret;
 
 	/* mod_map is freed via call_rcu() */
 	preempt_disable();
-- 
2.39.2

Re: [PATCH 2/2] [v2] kallsyms: rework symbol lookup return codes

[Luis Chamberlain]

On Mon, Jul 24, 2023 at 03:53:02PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Building with W=1 in some configurations produces a false positive
> warning for kallsyms:
> 
> kernel/kallsyms.c: In function '__sprint_symbol.isra':
> kernel/kallsyms.c:503:17: error: 'strcpy' source argument is the same as destination [-Werror=restrict]
>   503 |                 strcpy(buffer, name);
>       |                 ^~~~~~~~~~~~~~~~~~~~
> 
> This originally showed up while building with -O3, but later started
> happening in other configurations as well, depending on inlining
> decisions. The underlying issue is that the local 'name' variable is
> always initialized to the be the same as 'buffer' in the called functions
> that fill the buffer, which gcc notices while inlining, though it could
> see that the address check always skips the copy.
> 
> The calling conventions here are rather unusual, as all of the internal
> lookup functions (bpf_address_lookup, ftrace_mod_address_lookup,
> ftrace_func_address_lookup, module_address_lookup and
> kallsyms_lookup_buildid) already use the provided buffer and either return
> the address of that buffer to indicate success, or NULL for failure,
> but the callers are written to also expect an arbitrary other buffer
> to be returned.
> 
> Rework the calling conventions to return the length of the filled buffer
> instead of its address, which is simpler and easier to follow as well
> as avoiding the warning. Leave only the kallsyms_lookup() calling conventions
> unchanged, since that is called from 16 different functions and
> adapting this would be a much bigger change.
> 
> Link: https://lore.kernel.org/all/20200107214042.855757-1-arnd@arndb.de/
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

For modules side of things:

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH 2/2] [v2] kallsyms: rework symbol lookup return codes

[Jiri Olsa]

On Mon, Jul 24, 2023 at 03:53:02PM +0200, Arnd Bergmann wrote:

SNIP

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 05c0024815bf9..bc0eed24a5873 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6965,7 +6965,7 @@ allocate_ftrace_mod_map(struct module *mod,
>  	return mod_map;
>  }
>  
> -static const char *
> +static int
>  ftrace_func_address_lookup(struct ftrace_mod_map *mod_map,
>  			   unsigned long addr, unsigned long *size,
>  			   unsigned long *off, char *sym)
> @@ -6986,21 +6986,18 @@ ftrace_func_address_lookup(struct ftrace_mod_map *mod_map,
>  			*size = found_func->size;
>  		if (off)
>  			*off = addr - found_func->ip;
> -		if (sym)
> -			strscpy(sym, found_func->name, KSYM_NAME_LEN);
> -
> -		return found_func->name;
> +		return strlcpy(sym, found_func->name, KSYM_NAME_LEN);

hi,
any reason not to call the original strscpy in here?

jirka

>  	}
>  
> -	return NULL;
> +	return 0;
>  }
>  
> -const char *
> +int
>  ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
>  		   unsigned long *off, char **modname, char *sym)
>  {
>  	struct ftrace_mod_map *mod_map;
> -	const char *ret = NULL;
> +	int ret;
>  
>  	/* mod_map is freed via call_rcu() */
>  	preempt_disable();
> -- 
> 2.39.2
> 

Re: [PATCH 2/2] [v2] kallsyms: rework symbol lookup return codes

[Arnd Bergmann]

On Wed, Jul 26, 2023, at 10:02, Jiri Olsa wrote:
> On Mon, Jul 24, 2023 at 03:53:02PM +0200, Arnd Bergmann wrote:

>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 05c0024815bf9..bc0eed24a5873 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -6965,7 +6965,7 @@ allocate_ftrace_mod_map(struct module *mod,
>>  	return mod_map;
>>  }
>>  
>> -static const char *
>> +static int
>>  ftrace_func_address_lookup(struct ftrace_mod_map *mod_map,
>>  			   unsigned long addr, unsigned long *size,
>>  			   unsigned long *off, char *sym)
>> @@ -6986,21 +6986,18 @@ ftrace_func_address_lookup(struct ftrace_mod_map *mod_map,
>>  			*size = found_func->size;
>>  		if (off)
>>  			*off = addr - found_func->ip;
>> -		if (sym)
>> -			strscpy(sym, found_func->name, KSYM_NAME_LEN);
>> -
>> -		return found_func->name;
>> +		return strlcpy(sym, found_func->name, KSYM_NAME_LEN);
>
> hi,
> any reason not to call the original strscpy in here?

No, that was a mistake. I replaced the other strcpy and strncpy
with strlcpy in order to get the desired behavior, but in fact
they should all be strscpy, and changing this one was an accident.

I'll send a v3.

     Arnd

Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning

[claudiu beznea]

On 24.07.2023 16:53, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Some configurations with gcc-12 or gcc-13 produce a warning for the source
> and destination of a memcpy() in atmel_sha_hmac_compute_ipad_hash() potentially
> overlapping:
> 
> In file included from include/linux/string.h:254,
>                   from drivers/crypto/atmel-sha.c:15:
> drivers/crypto/atmel-sha.c: In function 'atmel_sha_hmac_compute_ipad_hash':
> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 408 and 280 overlaps 1 or more bytes at offset 408 [-Werror=restrict]
>     57 | #define __underlying_memcpy     __builtin_memcpy
>        |                                 ^
> include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy'
>    648 |         __underlying_##op(p, q, __fortify_size);                        \
>        |         ^~~~~~~~~~~~~
> include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk'
>    693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>        |                          ^~~~~~~~~~~~~~~~~~~~
> drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro 'memcpy'
>   1773 |         memcpy(hmac->opad, hmac->ipad, bs);
>        |         ^~~~~~
> 
> The same thing happens in two more drivers that have the same logic:
> 
> drivers/crypto/chelsio/chcr_algo.c: In function 'chcr_ahash_setkey':
> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 260 and 132 overlaps 1 or more bytes at offset 260 [-Werror=restrict]
> drivers/crypto/bcm/cipher.c: In function 'ahash_hmac_setkey':
> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing between 129 and 4294967295 bytes at offsets 840 and 712 overlaps between 1 and 4294967167 bytes at offset 840 [-Werror=restrict]
> 
> I don't think it can actually happen because the size is strictly bounded
> to the available block sizes, at most 128 bytes, though inlining decisions
> could lead gcc to not see that.
> 
> Add an explicit size check to make sure gcc also sees this function is safe
> regardless of inlining.
> 
> Note that the -Wrestrict warning is currently disabled by default, but it
> would be nice to finally enable it, and these are the only false
> postives that I see at the moment. There are 9 other crypto drivers that
> also use an identical memcpy() but don't show up in randconfig build
> warnings for me, presumably because of different inlining decisions.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev> # atmel-sha

> ---
>   drivers/crypto/atmel-sha.c         | 3 +++
>   drivers/crypto/bcm/cipher.c        | 3 +++
>   drivers/crypto/chelsio/chcr_algo.c | 3 +++
>   3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
> index f2031f934be95..52a3c81b3a05a 100644
> --- a/drivers/crypto/atmel-sha.c
> +++ b/drivers/crypto/atmel-sha.c
> @@ -1770,6 +1770,9 @@ static int atmel_sha_hmac_compute_ipad_hash(struct atmel_sha_dev *dd)
>   	size_t bs = ctx->block_size;
>   	size_t i, num_words = bs / sizeof(u32);
>   
> +	if (bs > sizeof(hmac->opad))
> +		return -EINVAL;
> +
>   	memcpy(hmac->opad, hmac->ipad, bs);
>   	for (i = 0; i < num_words; ++i) {
>   		hmac->ipad[i] ^= 0x36363636;
> diff --git a/drivers/crypto/bcm/cipher.c b/drivers/crypto/bcm/cipher.c
> index 70b911baab26d..8633ca0286a10 100644
> --- a/drivers/crypto/bcm/cipher.c
> +++ b/drivers/crypto/bcm/cipher.c
> @@ -2327,6 +2327,9 @@ static int ahash_hmac_setkey(struct crypto_ahash *ahash, const u8 *key,
>   		 __func__, ahash, key, keylen, blocksize, digestsize);
>   	flow_dump("  key: ", key, keylen);
>   
> +	if (blocksize > sizeof(ctx->opad))
> +		return -EINVAL;
> +
>   	if (keylen > blocksize) {
>   		switch (ctx->auth.alg) {
>   		case HASH_ALG_MD5:
> diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
> index 0eade4fa6695b..5c8e10ee010ff 100644
> --- a/drivers/crypto/chelsio/chcr_algo.c
> +++ b/drivers/crypto/chelsio/chcr_algo.c
> @@ -2201,6 +2201,9 @@ static int chcr_ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
>   
>   	SHASH_DESC_ON_STACK(shash, hmacctx->base_hash);
>   
> +	if (bs > sizeof(hmacctx->opad))
> +		return -EINVAL;
> +
>   	/* use the key to calculate the ipad and opad. ipad will sent with the
>   	 * first request's data. opad will be sent with the final hash result
>   	 * ipad in hmacctx->ipad and opad in hmacctx->opad location

Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning

[claudiu beznea]

On 24.07.2023 16:53, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Some configurations with gcc-12 or gcc-13 produce a warning for the source
> and destination of a memcpy() in atmel_sha_hmac_compute_ipad_hash() potentially
> overlapping:
> 
> In file included from include/linux/string.h:254,
>                   from drivers/crypto/atmel-sha.c:15:
> drivers/crypto/atmel-sha.c: In function 'atmel_sha_hmac_compute_ipad_hash':
> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 408 and 280 overlaps 1 or more bytes at offset 408 [-Werror=restrict]
>     57 | #define __underlying_memcpy     __builtin_memcpy
>        |                                 ^
> include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy'
>    648 |         __underlying_##op(p, q, __fortify_size);                        \
>        |         ^~~~~~~~~~~~~
> include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk'
>    693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>        |                          ^~~~~~~~~~~~~~~~~~~~
> drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro 'memcpy'
>   1773 |         memcpy(hmac->opad, hmac->ipad, bs);
>        |         ^~~~~~
> 
> The same thing happens in two more drivers that have the same logic:
> 
> drivers/crypto/chelsio/chcr_algo.c: In function 'chcr_ahash_setkey':
> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 260 and 132 overlaps 1 or more bytes at offset 260 [-Werror=restrict]
> drivers/crypto/bcm/cipher.c: In function 'ahash_hmac_setkey':
> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing between 129 and 4294967295 bytes at offsets 840 and 712 overlaps between 1 and 4294967167 bytes at offset 840 [-Werror=restrict]
> 
> I don't think it can actually happen because the size is strictly bounded
> to the available block sizes, at most 128 bytes, though inlining decisions
> could lead gcc to not see that.
> 
> Add an explicit size check to make sure gcc also sees this function is safe
> regardless of inlining.
> 
> Note that the -Wrestrict warning is currently disabled by default, but it
> would be nice to finally enable it, and these are the only false
> postives that I see at the moment. There are 9 other crypto drivers that
> also use an identical memcpy() but don't show up in randconfig build
> warnings for me, presumably because of different inlining decisions.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev> # atmel-sha

> ---
>   drivers/crypto/atmel-sha.c         | 3 +++
>   drivers/crypto/bcm/cipher.c        | 3 +++
>   drivers/crypto/chelsio/chcr_algo.c | 3 +++
>   3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
> index f2031f934be95..52a3c81b3a05a 100644
> --- a/drivers/crypto/atmel-sha.c
> +++ b/drivers/crypto/atmel-sha.c
> @@ -1770,6 +1770,9 @@ static int atmel_sha_hmac_compute_ipad_hash(struct atmel_sha_dev *dd)
>   	size_t bs = ctx->block_size;
>   	size_t i, num_words = bs / sizeof(u32);
>   
> +	if (bs > sizeof(hmac->opad))
> +		return -EINVAL;
> +
>   	memcpy(hmac->opad, hmac->ipad, bs);
>   	for (i = 0; i < num_words; ++i) {
>   		hmac->ipad[i] ^= 0x36363636;
> diff --git a/drivers/crypto/bcm/cipher.c b/drivers/crypto/bcm/cipher.c
> index 70b911baab26d..8633ca0286a10 100644
> --- a/drivers/crypto/bcm/cipher.c
> +++ b/drivers/crypto/bcm/cipher.c
> @@ -2327,6 +2327,9 @@ static int ahash_hmac_setkey(struct crypto_ahash *ahash, const u8 *key,
>   		 __func__, ahash, key, keylen, blocksize, digestsize);
>   	flow_dump("  key: ", key, keylen);
>   
> +	if (blocksize > sizeof(ctx->opad))
> +		return -EINVAL;
> +
>   	if (keylen > blocksize) {
>   		switch (ctx->auth.alg) {
>   		case HASH_ALG_MD5:
> diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
> index 0eade4fa6695b..5c8e10ee010ff 100644
> --- a/drivers/crypto/chelsio/chcr_algo.c
> +++ b/drivers/crypto/chelsio/chcr_algo.c
> @@ -2201,6 +2201,9 @@ static int chcr_ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
>   
>   	SHASH_DESC_ON_STACK(shash, hmacctx->base_hash);
>   
> +	if (bs > sizeof(hmacctx->opad))
> +		return -EINVAL;
> +
>   	/* use the key to calculate the ipad and opad. ipad will sent with the
>   	 * first request's data. opad will be sent with the final hash result
>   	 * ipad in hmacctx->ipad and opad in hmacctx->opad location

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning

[Herbert Xu]

On Mon, Jul 24, 2023 at 03:53:01PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Some configurations with gcc-12 or gcc-13 produce a warning for the source
> and destination of a memcpy() in atmel_sha_hmac_compute_ipad_hash() potentially
> overlapping:
> 
> In file included from include/linux/string.h:254,
>                  from drivers/crypto/atmel-sha.c:15:
> drivers/crypto/atmel-sha.c: In function 'atmel_sha_hmac_compute_ipad_hash':
> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 408 and 280 overlaps 1 or more bytes at offset 408 [-Werror=restrict]
>    57 | #define __underlying_memcpy     __builtin_memcpy
>       |                                 ^
> include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy'
>   648 |         __underlying_##op(p, q, __fortify_size);                        \
>       |         ^~~~~~~~~~~~~
> include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk'
>   693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>       |                          ^~~~~~~~~~~~~~~~~~~~
> drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro 'memcpy'
>  1773 |         memcpy(hmac->opad, hmac->ipad, bs);
>       |         ^~~~~~
> 
> The same thing happens in two more drivers that have the same logic:

Please send me the configurations which triggers these warnings.
As these are false positives, I'd like to enable them only on the
configurations where they actually cause a problem.

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 1/2] crypto: drivers - avoid memcpy size warning

[Herbert Xu]

On Mon, Jul 24, 2023 at 03:53:01PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Some configurations with gcc-12 or gcc-13 produce a warning for the source
> and destination of a memcpy() in atmel_sha_hmac_compute_ipad_hash() potentially
> overlapping:
> 
> In file included from include/linux/string.h:254,
>                  from drivers/crypto/atmel-sha.c:15:
> drivers/crypto/atmel-sha.c: In function 'atmel_sha_hmac_compute_ipad_hash':
> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 408 and 280 overlaps 1 or more bytes at offset 408 [-Werror=restrict]
>    57 | #define __underlying_memcpy     __builtin_memcpy
>       |                                 ^
> include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy'
>   648 |         __underlying_##op(p, q, __fortify_size);                        \
>       |         ^~~~~~~~~~~~~
> include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk'
>   693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>       |                          ^~~~~~~~~~~~~~~~~~~~
> drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro 'memcpy'
>  1773 |         memcpy(hmac->opad, hmac->ipad, bs);
>       |         ^~~~~~
> 
> The same thing happens in two more drivers that have the same logic:

Please send me the configurations which triggers these warnings.
As these are false positives, I'd like to enable them only on the
configurations where they actually cause a problem.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning

[Arnd Bergmann]

On Fri, Aug 4, 2023, at 10:16, Herbert Xu wrote:
> On Mon, Jul 24, 2023 at 03:53:01PM +0200, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> Some configurations with gcc-12 or gcc-13 produce a warning for the source
>> and destination of a memcpy() in atmel_sha_hmac_compute_ipad_hash() potentially
>> overlapping:
>> 
>> In file included from include/linux/string.h:254,
>>                  from drivers/crypto/atmel-sha.c:15:
>> drivers/crypto/atmel-sha.c: In function 'atmel_sha_hmac_compute_ipad_hash':
>> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 408 and 280 overlaps 1 or more bytes at offset 408 [-Werror=restrict]
>>    57 | #define __underlying_memcpy     __builtin_memcpy
>>       |                                 ^
>> include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy'
>>   648 |         __underlying_##op(p, q, __fortify_size);                        \
>>       |         ^~~~~~~~~~~~~
>> include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk'
>>   693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>>       |                          ^~~~~~~~~~~~~~~~~~~~
>> drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro 'memcpy'
>>  1773 |         memcpy(hmac->opad, hmac->ipad, bs);
>>       |         ^~~~~~
>> 
>> The same thing happens in two more drivers that have the same logic:
>
> Please send me the configurations which triggers these warnings.
> As these are false positives, I'd like to enable them only on the
> configurations where they actually cause a problem.

See https://pastebin.com/raw/ip3tfpJF for a config that triggers this
on x86 with the chelsio and atmel drivers. The bcm driver is only
available on arm64, so you won't hit that one here. I also
see this with allmodconfig, as well as defconfig after enabling
CONFIG_FORTIFY_SOURCE and the three crypto drivers.

I see that commit df8fc4e934c12 ("kbuild: Enable -fstrict-flex-arrays=3")
turned on the strict flex-array behavior that triggers the
warning, so this did not show up until linux-6.5-rc1.
In linux-next, I see no other code hit this warning after all
my other patches for it got merged, regardless strict flex
arrays.

At the moment, -Wrestrict is completely disabled in all builds,
so you have to add a patch to enable it in the build system,
this is what I use locally to enable it at the W=1 level,
though you can probably just replace the cc-disable-warning
line with a -Wrestrict line.

     Arnd

--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -49,9 +49,6 @@ KBUILD_CFLAGS += -Wno-pointer-sign
 # globally built with -Wcast-function-type.
 KBUILD_CFLAGS += $(call cc-option, -Wcast-function-type)
 
-# Another good warning that we'll want to enable eventually
-KBUILD_CFLAGS += $(call cc-disable-warning, restrict)
-
 # The allocators already balk at large sizes, so silence the compiler
 # warnings for bounds checks involving those possible values. While
 # -Wno-alloc-size-larger-than would normally be used here, earlier versions
@@ -93,6 +90,7 @@ export KBUILD_EXTRA_WARN
 ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
 
 KBUILD_CFLAGS += -Wextra -Wunused -Wno-unused-parameter
+KBUILD_CFLAGS += $(call cc-option, -Wrestrict)
 KBUILD_CFLAGS += -Wmissing-format-attribute
 KBUILD_CFLAGS += -Wold-style-definition
 KBUILD_CFLAGS += -Wmissing-include-dirs
@@ -105,6 +103,7 @@ else
 
 # Some diagnostics enabled by default are noisy.
 # Suppress them by using -Wno... except for W=1.
+KBUILD_CFLAGS += $(call cc-disable-warning, restrict)
 KBUILD_CFLAGS += $(call cc-disable-warning, packed-not-aligned)
 
 ifdef CONFIG_CC_IS_CLANG

Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning

[Arnd Bergmann]

On Fri, Aug 4, 2023, at 10:16, Herbert Xu wrote:
> On Mon, Jul 24, 2023 at 03:53:01PM +0200, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> Some configurations with gcc-12 or gcc-13 produce a warning for the source
>> and destination of a memcpy() in atmel_sha_hmac_compute_ipad_hash() potentially
>> overlapping:
>> 
>> In file included from include/linux/string.h:254,
>>                  from drivers/crypto/atmel-sha.c:15:
>> drivers/crypto/atmel-sha.c: In function 'atmel_sha_hmac_compute_ipad_hash':
>> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 408 and 280 overlaps 1 or more bytes at offset 408 [-Werror=restrict]
>>    57 | #define __underlying_memcpy     __builtin_memcpy
>>       |                                 ^
>> include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy'
>>   648 |         __underlying_##op(p, q, __fortify_size);                        \
>>       |         ^~~~~~~~~~~~~
>> include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk'
>>   693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>>       |                          ^~~~~~~~~~~~~~~~~~~~
>> drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro 'memcpy'
>>  1773 |         memcpy(hmac->opad, hmac->ipad, bs);
>>       |         ^~~~~~
>> 
>> The same thing happens in two more drivers that have the same logic:
>
> Please send me the configurations which triggers these warnings.
> As these are false positives, I'd like to enable them only on the
> configurations where they actually cause a problem.

See https://pastebin.com/raw/ip3tfpJF for a config that triggers this
on x86 with the chelsio and atmel drivers. The bcm driver is only
available on arm64, so you won't hit that one here. I also
see this with allmodconfig, as well as defconfig after enabling
CONFIG_FORTIFY_SOURCE and the three crypto drivers.

I see that commit df8fc4e934c12 ("kbuild: Enable -fstrict-flex-arrays=3")
turned on the strict flex-array behavior that triggers the
warning, so this did not show up until linux-6.5-rc1.
In linux-next, I see no other code hit this warning after all
my other patches for it got merged, regardless strict flex
arrays.

At the moment, -Wrestrict is completely disabled in all builds,
so you have to add a patch to enable it in the build system,
this is what I use locally to enable it at the W=1 level,
though you can probably just replace the cc-disable-warning
line with a -Wrestrict line.

     Arnd

--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -49,9 +49,6 @@ KBUILD_CFLAGS += -Wno-pointer-sign
 # globally built with -Wcast-function-type.
 KBUILD_CFLAGS += $(call cc-option, -Wcast-function-type)
 
-# Another good warning that we'll want to enable eventually
-KBUILD_CFLAGS += $(call cc-disable-warning, restrict)
-
 # The allocators already balk at large sizes, so silence the compiler
 # warnings for bounds checks involving those possible values. While
 # -Wno-alloc-size-larger-than would normally be used here, earlier versions
@@ -93,6 +90,7 @@ export KBUILD_EXTRA_WARN
 ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
 
 KBUILD_CFLAGS += -Wextra -Wunused -Wno-unused-parameter
+KBUILD_CFLAGS += $(call cc-option, -Wrestrict)
 KBUILD_CFLAGS += -Wmissing-format-attribute
 KBUILD_CFLAGS += -Wold-style-definition
 KBUILD_CFLAGS += -Wmissing-include-dirs
@@ -105,6 +103,7 @@ else
 
 # Some diagnostics enabled by default are noisy.
 # Suppress them by using -Wno... except for W=1.
+KBUILD_CFLAGS += $(call cc-disable-warning, restrict)
 KBUILD_CFLAGS += $(call cc-disable-warning, packed-not-aligned)
 
 ifdef CONFIG_CC_IS_CLANG


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning

[Herbert Xu]

On Mon, Aug 07, 2023 at 02:04:05PM +0200, Arnd Bergmann wrote:
>
> See https://pastebin.com/raw/ip3tfpJF for a config that triggers this
> on x86 with the chelsio and atmel drivers. The bcm driver is only
> available on arm64, so you won't hit that one here. I also
> see this with allmodconfig, as well as defconfig after enabling
> CONFIG_FORTIFY_SOURCE and the three crypto drivers.

OK I can reproduce this now:

In file included from ../include/linux/string.h:254,
                 from ../arch/x86/include/asm/page_32.h:18,
                 from ../arch/x86/include/asm/page.h:14,
                 from ../arch/x86/include/asm/processor.h:20,
                 from ../arch/x86/include/asm/timex.h:5,
                 from ../include/linux/timex.h:67,
                 from ../include/linux/time32.h:13,
                 from ../include/linux/time.h:60,
                 from ../include/linux/stat.h:19,
                 from ../include/linux/module.h:13,
                 from ../drivers/crypto/atmel-sha.c:15:
../drivers/crypto/atmel-sha.c: In function ‘atmel_sha_hmac_compute_ipad_hash’:
../include/linux/fortify-string.h:57:33: error: ‘__builtin_memcpy’ accessing 129 or more bytes at offsets 304 and 176 overlaps 1 or more bytes at offset 304 [-Werror=restrict]
   57 | #define __underlying_memcpy     __builtin_memcpy
      |                                 ^
../include/linux/fortify-string.h:648:9: note: in expansion of macro ‘__underlying_memcpy’
  648 |         __underlying_##op(p, q, __fortify_size);                        \
      |         ^~~~~~~~~~~~~
../include/linux/fortify-string.h:693:26: note: in expansion of macro ‘__fortify_memcpy_chk’
  693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
      |                          ^~~~~~~~~~~~~~~~~~~~
../drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro ‘memcpy’
 1773 |         memcpy(hmac->opad, hmac->ipad, bs);
      |         ^~~~~~
../include/linux/fortify-string.h:57:33: error: ‘__builtin_memcpy’ accessing 129 or more bytes at offsets 304 and 176 overlaps 1 or more bytes at offset 304 [-Werror=restrict]
   57 | #define __underlying_memcpy     __builtin_memcpy
      |                                 ^
../include/linux/fortify-string.h:648:9: note: in expansion of macro ‘__underlying_memcpy’
  648 |         __underlying_##op(p, q, __fortify_size);                        \
      |         ^~~~~~~~~~~~~
../include/linux/fortify-string.h:693:26: note: in expansion of macro ‘__fortify_memcpy_chk’
  693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
      |                          ^~~~~~~~~~~~~~~~~~~~
../drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro ‘memcpy’
 1773 |         memcpy(hmac->opad, hmac->ipad, bs);
      |         ^~~~~~
cc1: all warnings being treated as errors

But why are we turning these warnings on if they're giving completely
bogus false positives like this?

	struct atmel_sha_hmac_ctx {
		struct atmel_sha_ctx	base;

		struct atmel_sha_hmac_key	hkey;
		u32			ipad[SHA512_BLOCK_SIZE / sizeof(u32)];
		u32			opad[SHA512_BLOCK_SIZE / sizeof(u32)];
		atmel_sha_fn_t		resume;
	};

	struct atmel_sha_hmac_ctx *hmac = crypto_ahash_ctx(tfm);
	size_t bs = ctx->block_size;

	memcpy(hmac->opad, hmac->ipad, bs);

The block_size is set by the algorithm, you can easily grep for
it in atmel-sha.c and the biggest one there is SHA512_BLOCK_SIZE,
which is how big hmac->ipad/hmac->opad are.

So logically this code is perfectly fine.

There is no way for the compiler to know how big ctx->block_size is.
So why do we expect it to make deductions on how big bs can be?

This warning looks broken to me.

It looks like there is already a solution to this though.  Just use
unsafe_memcpy and be done with it.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning

[Herbert Xu]

On Mon, Aug 07, 2023 at 02:04:05PM +0200, Arnd Bergmann wrote:
>
> See https://pastebin.com/raw/ip3tfpJF for a config that triggers this
> on x86 with the chelsio and atmel drivers. The bcm driver is only
> available on arm64, so you won't hit that one here. I also
> see this with allmodconfig, as well as defconfig after enabling
> CONFIG_FORTIFY_SOURCE and the three crypto drivers.

OK I can reproduce this now:

In file included from ../include/linux/string.h:254,
                 from ../arch/x86/include/asm/page_32.h:18,
                 from ../arch/x86/include/asm/page.h:14,
                 from ../arch/x86/include/asm/processor.h:20,
                 from ../arch/x86/include/asm/timex.h:5,
                 from ../include/linux/timex.h:67,
                 from ../include/linux/time32.h:13,
                 from ../include/linux/time.h:60,
                 from ../include/linux/stat.h:19,
                 from ../include/linux/module.h:13,
                 from ../drivers/crypto/atmel-sha.c:15:
../drivers/crypto/atmel-sha.c: In function ‘atmel_sha_hmac_compute_ipad_hash’:
../include/linux/fortify-string.h:57:33: error: ‘__builtin_memcpy’ accessing 129 or more bytes at offsets 304 and 176 overlaps 1 or more bytes at offset 304 [-Werror=restrict]
   57 | #define __underlying_memcpy     __builtin_memcpy
      |                                 ^
../include/linux/fortify-string.h:648:9: note: in expansion of macro ‘__underlying_memcpy’
  648 |         __underlying_##op(p, q, __fortify_size);                        \
      |         ^~~~~~~~~~~~~
../include/linux/fortify-string.h:693:26: note: in expansion of macro ‘__fortify_memcpy_chk’
  693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
      |                          ^~~~~~~~~~~~~~~~~~~~
../drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro ‘memcpy’
 1773 |         memcpy(hmac->opad, hmac->ipad, bs);
      |         ^~~~~~
../include/linux/fortify-string.h:57:33: error: ‘__builtin_memcpy’ accessing 129 or more bytes at offsets 304 and 176 overlaps 1 or more bytes at offset 304 [-Werror=restrict]
   57 | #define __underlying_memcpy     __builtin_memcpy
      |                                 ^
../include/linux/fortify-string.h:648:9: note: in expansion of macro ‘__underlying_memcpy’
  648 |         __underlying_##op(p, q, __fortify_size);                        \
      |         ^~~~~~~~~~~~~
../include/linux/fortify-string.h:693:26: note: in expansion of macro ‘__fortify_memcpy_chk’
  693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
      |                          ^~~~~~~~~~~~~~~~~~~~
../drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro ‘memcpy’
 1773 |         memcpy(hmac->opad, hmac->ipad, bs);
      |         ^~~~~~
cc1: all warnings being treated as errors

But why are we turning these warnings on if they're giving completely
bogus false positives like this?

	struct atmel_sha_hmac_ctx {
		struct atmel_sha_ctx	base;

		struct atmel_sha_hmac_key	hkey;
		u32			ipad[SHA512_BLOCK_SIZE / sizeof(u32)];
		u32			opad[SHA512_BLOCK_SIZE / sizeof(u32)];
		atmel_sha_fn_t		resume;
	};

	struct atmel_sha_hmac_ctx *hmac = crypto_ahash_ctx(tfm);
	size_t bs = ctx->block_size;

	memcpy(hmac->opad, hmac->ipad, bs);

The block_size is set by the algorithm, you can easily grep for
it in atmel-sha.c and the biggest one there is SHA512_BLOCK_SIZE,
which is how big hmac->ipad/hmac->opad are.

So logically this code is perfectly fine.

There is no way for the compiler to know how big ctx->block_size is.
So why do we expect it to make deductions on how big bs can be?

This warning looks broken to me.

It looks like there is already a solution to this though.  Just use
unsafe_memcpy and be done with it.

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

Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning

[Arnd Bergmann]

On Fri, Aug 11, 2023, at 12:48, Herbert Xu wrote:
> On Mon, Aug 07, 2023 at 02:04:05PM +0200, Arnd Bergmann wrote:

>
> 	struct atmel_sha_hmac_ctx *hmac = crypto_ahash_ctx(tfm);
> 	size_t bs = ctx->block_size;
>
> 	memcpy(hmac->opad, hmac->ipad, bs);
>
> The block_size is set by the algorithm, you can easily grep for
> it in atmel-sha.c and the biggest one there is SHA512_BLOCK_SIZE,
> which is how big hmac->ipad/hmac->opad are.
>
> So logically this code is perfectly fine.

Right, that was my conclusion as well.

> There is no way for the compiler to know how big ctx->block_size is.
> So why do we expect it to make deductions on how big bs can be?
>
> This warning looks broken to me.

I'm still unsure how exactly it goes wrong here, but I suspect
it works as designed and just happens to run into a case in these
drivers that is just not that common. I also see that the kernel's
memcpy() declaration is missing the "restrict" annotation,
but the fortified version calls the __builtin_memcpy() variant
that has an implicit prototype with those annotations.

> It looks like there is already a solution to this though.  Just use
> unsafe_memcpy and be done with it.

Fine with me, I'll send a new version doing that.

     Arnd

Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning

[Arnd Bergmann]

On Fri, Aug 11, 2023, at 12:48, Herbert Xu wrote:
> On Mon, Aug 07, 2023 at 02:04:05PM +0200, Arnd Bergmann wrote:

>
> 	struct atmel_sha_hmac_ctx *hmac = crypto_ahash_ctx(tfm);
> 	size_t bs = ctx->block_size;
>
> 	memcpy(hmac->opad, hmac->ipad, bs);
>
> The block_size is set by the algorithm, you can easily grep for
> it in atmel-sha.c and the biggest one there is SHA512_BLOCK_SIZE,
> which is how big hmac->ipad/hmac->opad are.
>
> So logically this code is perfectly fine.

Right, that was my conclusion as well.

> There is no way for the compiler to know how big ctx->block_size is.
> So why do we expect it to make deductions on how big bs can be?
>
> This warning looks broken to me.

I'm still unsure how exactly it goes wrong here, but I suspect
it works as designed and just happens to run into a case in these
drivers that is just not that common. I also see that the kernel's
memcpy() declaration is missing the "restrict" annotation,
but the fortified version calls the __builtin_memcpy() variant
that has an implicit prototype with those annotations.

> It looks like there is already a solution to this though.  Just use
> unsafe_memcpy and be done with it.

Fine with me, I'll send a new version doing that.

     Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel