Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
From: Beau Belgrave <hidden>
Date: 2023-09-14 16:42:32
Also in:
lkml
On Thu, Sep 14, 2023 at 03:11:02PM +0200, Clément Léger wrote:
enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
this will result in a misaligned access on 64 bits architectures since
set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
On architecture that do not support misaligned access, this will crash
the kernel. Align uaddr on unsigned long size to avoid such behavior.
This bug was found while running kselftests on RISC-V.
Fixes: 7235759084a4 ("tracing/user_events: Use remote writes for event enablement")
Signed-off-by: Clément Léger <redacted>Thanks for fixing! I have a few comments on this. I unfortunately do not have RISC-V hardware to validate this on.
quoted hunk ↗ jump to hunk
--- kernel/trace/trace_events_user.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c index 6f046650e527..580c0fe4b23e 100644 --- a/kernel/trace/trace_events_user.c +++ b/kernel/trace/trace_events_user.c@@ -479,7 +479,7 @@ static int user_event_enabler_write(struct user_event_mm *mm, bool fixup_fault, int *attempt) { unsigned long uaddr = enabler->addr; - unsigned long *ptr; + unsigned long *ptr, bit_offset; struct page *page; void *kaddr; int ret;@@ -511,13 +511,19 @@ static int user_event_enabler_write(struct user_event_mm *mm, } kaddr = kmap_local_page(page); + + bit_offset = uaddr & (sizeof(unsigned long) - 1); + if (bit_offset) { + bit_offset *= 8;
I think for future readers of this code it would be more clear to use BITS_PER_BYTE instead of the hardcoded 8. Given we always align on a "natural" boundary, I believe the bit_offset will always be 32 bits. A comment here might help clarify why we do this as well in case folks don't see the change description.
+ uaddr &= ~(sizeof(unsigned long) - 1);
Shouldn't this uaddr change be done before calling pin_user_pages_remote() to ensure things cannot go bad? (I don't think they can, but it looks a little odd). Thanks, -Beau
+ } ptr = kaddr + (uaddr & ~PAGE_MASK); /* Update bit atomically, user tracers must be atomic as well */ if (enabler->event && enabler->event->status) - set_bit(ENABLE_BIT(enabler), ptr); + set_bit(ENABLE_BIT(enabler) + bit_offset, ptr); else - clear_bit(ENABLE_BIT(enabler), ptr); + clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr); kunmap_local(kaddr); unpin_user_pages_dirty_lock(&page, 1, true); -- 2.40.1