From: Mikulas Patocka <mpatocka@redhat.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
Dave Hansen <dave.hansen@linux.intel.com>,
"H. Peter Anvin" <hpa@zytor.com>,
the arch/x86 maintainers <x86@kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: [PATCH v2] x86: __memcpy_flushcache: fix wrong alignment if size > 2^32
Date: Tue, 19 Apr 2022 16:11:11 -0400 (EDT) [thread overview]
Message-ID: <alpine.LRH.2.02.2204191551430.16728@file01.intranet.prod.int.rdu2.redhat.com> (raw)
In-Reply-To: <CAHk-=wjNHZ1_sT4iN_TVu+2cmd_n2W+EYfd9vSL3L82kNJTmPg@mail.gmail.com>
On Tue, 19 Apr 2022, Linus Torvalds wrote:
> 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.
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 <mpatocka@redhat.com>
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 <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
@@ -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);
prev parent reply other threads:[~2022-04-19 20:11 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-19 13:56 [PATCH] x86: __memcpy_flushcache: fix wrong alignment if size > 2^32 Mikulas Patocka
2022-04-19 17:13 ` Linus Torvalds
2022-04-19 20:11 ` Mikulas Patocka [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LRH.2.02.2204191551430.16728@file01.intranet.prod.int.rdu2.redhat.com \
--to=mpatocka@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.