On Tue, 19 Apr 2022, Linus Torvalds wrote:
> On Tue, Apr 19, 2022 at 6:56 AM Mikulas Patocka <[email protected]> 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.
If you skip copying, it could in theory have security or reliability
implications (the user may be reading stale data that he is not supposed
to read). So, I think it's better to just add WARN_ON_ONCE and proceed
with the copying.
> 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
For example, the dm-stats subsystem (drivers/md/dm-stats.c) can allocate
extremely large blocks of memory using vmalloc (it is only limited to 1/4
of total system memory) - though, it doesn't use memcpy on it.
> wanted to do (which sounds very unlikely), you'd need to split it up
> with cond_resched() just for latency reasons.
>
> Linus
From: Mikulas Patocka <[email protected]>
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.
As the function is not supposed to be used with large size, the patch also
adds a WARN_ON_ONCE there.
Signed-off-by: Mikulas Patocka <[email protected]>
---
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
@@ -117,9 +117,11 @@ void __memcpy_flushcache(void *_dst, con
unsigned long dest = (unsigned long) _dst;
unsigned long source = (unsigned long) _src;
+ WARN_ON_ONCE(size > INT_MAX);
+
/* 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);