On Mon, Mar 10, 2025 at 10:37 AM Catalin Marinas catalin.marinas@arm.com wrote:
On Fri, Mar 07, 2025 at 07:36:31PM -0800, Kees Cook wrote:
On Fri, Mar 07, 2025 at 06:33:13PM -0800, Peter Collingbourne wrote:
The optimized strscpy() and dentry_string_cmp() routines will read 8 unaligned bytes at a time via the function read_word_at_a_time(), but this is incompatible with MTE which will fault on a partially invalid read. The attributes on read_word_at_a_time() that disable KASAN are invisible to the CPU so they have no effect on MTE. Let's fix the bug for now by disabling the optimizations if the kernel is built with HW tag-based KASAN and consider improvements for followup changes.
Why is faulting on a partially invalid read a problem? It's still invalid, so ... it should fault, yes? What am I missing?
read_word_at_a_time() is used to read 8 bytes, potentially unaligned and beyond the end of string. The has_zero() function is then used to check where the string ends. For this uses, I think we can go with load_unaligned_zeropad() which handles a potential fault and pads the rest with zeroes.
Agreed, strscpy() should be using load_unaligned_zeropad() if available. We can also disable the code that checks for page boundaries if load_unaligned_zeropad() is available.
The only other use of read_word_at_a_time() is the one in dentry_string_cmp(). At the time that I sent the patch I hadn't noticed the comment in dentry_string_cmp() stating that its argument to read_word_at_a_time() is aligned. Since calling read_word_at_a_time() is fine if the pointer is aligned but partially invalid (not possible with MTE but it is possible with SW KASAN which uses a 1:1 shadow mapping), I left this user as-is. In other words, I didn't make read_word_at_a_time() arch-specific as in Vincenzo's series.
I sent a v2 with my patch to switch strscpy() over to using load_unaligned_zeropad() if available, as well as the patch adding tests from Vincenzo's series (which had some issues that I fixed).
Peter
Signed-off-by: Peter Collingbourne pcc@google.com Link: https://linux-review.googlesource.com/id/If4b22e43b5a4ca49726b4bf98ada827fdf... Fixes: 94ab5b61ee16 ("kasan, arm64: enable CONFIG_KASAN_HW_TAGS") Cc: stable@vger.kernel.org
fs/dcache.c | 2 +- lib/string.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)
Why are DCACHE_WORD_ACCESS and HAVE_EFFICIENT_UNALIGNED_ACCESS separate things? I can see at least one place where it's directly tied:
arch/arm/Kconfig:58: select DCACHE_WORD_ACCESS if HAVE_EFFICIENT_UNALIGNED_ACCESS
DCACHE_WORD_ACCESS requires load_unaligned_zeropad() which handles the faults. For some reason, read_word_at_a_time() doesn't expect to fault and it is only used with HAVE_EFFICIENT_UNALIGNED_ACCESS. I guess arm32 only enabled load_unaligned_zeropad() on hardware that supports efficient unaligned accesses (v6 onwards), hence the dependency.
Would it make sense to sort this out so that KASAN_HW_TAGS can be taken into account at the Kconfig level instead?
I don't think we should play with config options but rather sort out the fault path (load_unaligned_zeropad) or disable MTE temporarily. I'd go with the former as long as read_word_at_a_time() is only used for strings in conjunction with has_zero(). I haven't checked.
-- Catalin