| Message ID | 20240523215029.4160518-1-bjohannesmeyer@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: kmsan: Fix hook for unaligned accesses | expand |
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>
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
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)
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(-)