diff mbox series

crypto: algapi - fix be32_to_cpu macro call in crypto_inc()

Message ID 1668606771-5382-1-git-send-email-khoroshilov@ispras.ru
State New
Headers
Series crypto: algapi - fix be32_to_cpu macro call in crypto_inc() |

Commit Message

Alexey Khoroshilov Nov. 16, 2022, 1:52 p.m. UTC 
  be32_to_cpu() macro in some cases may be expanded to an expression
that evaluates its arguments multiple times. Because of the decrement
in argument it has unexpected result in such cases.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Fixes: 7613636def82 ("[CRYPTO] api: Add crypto_inc and crypto_xor")
---
 crypto/algapi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Eric Biggers Nov. 16, 2022, 5:39 p.m. UTC | #1 
On Wed, Nov 16, 2022 at 04:52:51PM +0300, Alexey Khoroshilov wrote:
> be32_to_cpu() macro in some cases may be expanded to an expression
> that evaluates its arguments multiple times. 

When is that, exactly?

If that's true, then lots of other places in the kernel would need to be fixed
too.  Try running:

	git grep -E '[bl]e(16|32|64)_to_cpu\([^)]+\+\+\)'

If true, then it would be much better to fix the macros.

But more likely is that there isn't actually any problem.

- Eric
Alexey Khoroshilov Nov. 16, 2022, 11:15 p.m. UTC | #2 
On 16.11.2022 20:39, Eric Biggers wrote:
> On Wed, Nov 16, 2022 at 04:52:51PM +0300, Alexey Khoroshilov wrote:
>> be32_to_cpu() macro in some cases may be expanded to an expression
>> that evaluates its arguments multiple times. 
> 
> When is that, exactly?


I considered the path
#define be32_to_cpu __be32_to_cpu

#define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))

#define __swab32(x)				\
	(__builtin_constant_p((__u32)(x)) ?	\
	___constant_swab32(x) :			\
	__fswab32(x))


#define ___constant_swab32(x) ((__u32)(				\
	(((__u32)(x) & (__u32)0x000000ffUL) << 24) |		\
	(((__u32)(x) & (__u32)0x0000ff00UL) <<  8) |		\
	(((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |		\
	(((__u32)(x) & (__u32)0xff000000UL) >> 24)))


But you are right, it is protected by __builtin_constant_p() that
prevents that for memory access.


> If that's true, then lots of other places in the kernel would need to be fixed
> too.  Try running:
> 
> 	git grep -E '[bl]e(16|32|64)_to_cpu\([^)]+\+\+\)'
> 
> If true, then it would be much better to fix the macros.


And yes, anyway it makes sense to keep for be32_to_cpu()-like function
macro a contract to handle its arguments as a plain function.

Thanks,
Alexey
diff mbox series

Patch

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 5c69ff8e8fa5..18f14aed1658 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -987,7 +987,8 @@  void crypto_inc(u8 *a, unsigned int size)
 	if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) ||
 	    IS_ALIGNED((unsigned long)b, __alignof__(*b)))
 		for (; size >= 4; size -= 4) {
-			c = be32_to_cpu(*--b) + 1;
+			b--;
+			c = be32_to_cpu(*b) + 1;
 			*b = cpu_to_be32(c);
 			if (likely(c))
 				return;