[PATCH] x86: kmsan: Fix hook for unaligned accesses

[PATCH] x86: kmsan: Fix hook for unaligned accesses

[Brian Johannesmeyer]

When called with a 'from' that is not 4-byte-aligned,
string_memcpy_fromio() calls the movs() macro to copy the first few bytes,
so that 'from' becomes 4-byte-aligned before calling rep_movs(). This
movs() macro modifies 'to', and the subsequent line modifies 'n'.

As a result, on unaligned accesses, kmsan_unpoison_memory() uses the
updated (aligned) values of 'to' and 'n'. Hence, it does not unpoison the
entire region.

This patch saves the original values of 'to' and 'n', and passes those to
kmsan_unpoison_memory(), so that the entire region is unpoisoned.

Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
---
 arch/x86/lib/iomem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/lib/iomem.c b/arch/x86/lib/iomem.c
index e0411a3774d4..5eecb45d05d5 100644
--- a/arch/x86/lib/iomem.c
+++ b/arch/x86/lib/iomem.c
@@ -25,6 +25,9 @@ static __always_inline void rep_movs(void *to, const void *from, size_t n)
 
 static void string_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
 {
+	const void *orig_to = to;
+	const size_t orig_n = n;
+
 	if (unlikely(!n))
 		return;
 
@@ -39,7 +42,7 @@ static void string_memcpy_fromio(void *to, const volatile void __iomem *from, si
 	}
 	rep_movs(to, (const void *)from, n);
 	/* KMSAN must treat values read from devices as initialized. */
-	kmsan_unpoison_memory(to, n);
+	kmsan_unpoison_memory(orig_to, orig_n);
 }
 
 static void string_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
-- 
2.34.1

Re: [PATCH] x86: kmsan: Fix hook for unaligned accesses

[Alexander Potapenko]

On Thu, May 23, 2024 at 11:50 PM Brian Johannesmeyer
<bjohannesmeyer@gmail.com> wrote:
>
> When called with a 'from' that is not 4-byte-aligned,
> string_memcpy_fromio() calls the movs() macro to copy the first few bytes,
> so that 'from' becomes 4-byte-aligned before calling rep_movs(). This
> movs() macro modifies 'to', and the subsequent line modifies 'n'.
>
> As a result, on unaligned accesses, kmsan_unpoison_memory() uses the
> updated (aligned) values of 'to' and 'n'. Hence, it does not unpoison the
> entire region.
>
> This patch saves the original values of 'to' and 'n', and passes those to
> kmsan_unpoison_memory(), so that the entire region is unpoisoned.

Nice catch! Does it fix any known bugs?

> Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
Reviewed-by: Alexander Potapenko <glider@google.com>

Re: [PATCH] x86: kmsan: Fix hook for unaligned accesses

[Brian Johannesmeyer]

On Fri, May 24, 2024 at 10:28:05AM +0200, Alexander Potapenko wrote:
> Nice catch! Does it fix any known bugs?

Not that I know of. Based on my cursory testing, it seems that
string_memcpy_fromio() is rarely called with an unaligned `from`, so
this is a bit of an edge case.

On that note: I tried creating a unit test for this, to verify that
an unaligned memcpy_fromio() would yield uninitialized data without the
patch, and would yield initialized data with the patch. However, what I
found is that kmsan_unpoison_memory() seems to always unpoison an entire
4-byte word, even if called with a `size` of less than 4. However, this
issue is somewhat unrelated to the patch at hand, so I'll create a
separate patch to demonstrate what I mean.

Thanks,
Brian