[PATCH] x86: __memcpy_flushcache: fix wrong alignment if size > 2^32

[PATCH] x86: __memcpy_flushcache: fix wrong alignment if size > 2^32

[Mikulas Patocka]

The statement "min_t(unsigned, size, ALIGN(dest, 8) - dest);" casts both 
arguments to unsigned int and selects the smaller one. However, if the 
size is larger than 2^32, the truncation returns incorrect result.

For example:
	size == 0x100000001, dest == 0x200000002
	min_t(unsigned, size, ALIGN(dest, 8) - dest) == min_t(0x1, 0xe) == 0x1;
	...
	dest += 0x1;
so we copy just one byte and dest remains unaligned.

This patch fixes the bug by replacing unsigned with size_t.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 arch/x86/lib/usercopy_64.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/arch/x86/lib/usercopy_64.c
===================================================================
--- linux-2.6.orig/arch/x86/lib/usercopy_64.c	2020-04-17 14:06:32.039999000 +0200
+++ linux-2.6/arch/x86/lib/usercopy_64.c	2020-04-17 14:06:32.039999000 +0200
@@ -141,7 +141,7 @@ void __memcpy_flushcache(void *_dst, con
 
 	/* cache copy and flush to align dest */
 	if (!IS_ALIGNED(dest, 8)) {
-		unsigned len = min_t(unsigned, size, ALIGN(dest, 8) - dest);
+		size_t len = min_t(size_t, size, ALIGN(dest, 8) - dest);
 
 		memcpy((void *) dest, (void *) source, len);
 		clean_cache_range((void *) dest, len);

Re: [PATCH] x86: __memcpy_flushcache: fix wrong alignment if size > 2^32

[Linus Torvalds]

On Tue, Apr 19, 2022 at 6:56 AM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> The first "if" condition in __memcpy_flushcache is supposed to align the
> "dest" variable to 8 bytes and copy data up to this alignment. However,
> this condition may misbehave if "size" is greater than 4GiB.

You're not wrong, but I also don't think it would be wrong to just have a

        if (WARN_ON_ONCE(size > MAX_INT))
                return;

in there instead.

It' not like "> 2**32" should ever really be a valid thing for any
kind of copy in the kernel. Even if that were to be what you actually
wanted to do (which sounds very unlikely), you'd need to split it up
with cond_resched() just for latency reasons.

             Linus

[PATCH] x86: __memcpy_flushcache: fix wrong alignment if size > 2^32

[Mikulas Patocka]

The first "if" condition in __memcpy_flushcache is supposed to align the
"dest" variable to 8 bytes and copy data up to this alignment. However,
this condition may misbehave if "size" is greater than 4GiB.

The statement min_t(unsigned, size, ALIGN(dest, 8) - dest); casts both
arguments to unsigned int and selects the smaller one. However, the cast
truncates high bits in "size" and it results in misbehavior.

For example:
	suppose that size == 0x100000001, dest == 0x200000002
	min_t(unsigned, size, ALIGN(dest, 8) - dest) == min_t(0x1, 0xe) == 0x1;
	...
	dest += 0x1;
so we copy just one byte "and" dest remains unaligned.

This patch fixes the bug by replacing unsigned with size_t.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 arch/x86/lib/usercopy_64.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/arch/x86/lib/usercopy_64.c
===================================================================
--- linux-2.6.orig/arch/x86/lib/usercopy_64.c
+++ linux-2.6/arch/x86/lib/usercopy_64.c
@@ -119,7 +119,7 @@ void __memcpy_flushcache(void *_dst, con
 
 	/* cache copy and flush to align dest */
 	if (!IS_ALIGNED(dest, 8)) {
-		unsigned len = min_t(unsigned, size, ALIGN(dest, 8) - dest);
+		size_t len = min_t(size_t, size, ALIGN(dest, 8) - dest);
 
 		memcpy((void *) dest, (void *) source, len);
 		clean_cache_range((void *) dest, len);