Thread (18 messages) 18 messages, 5 authors, 2023-09-25

Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Date: 2023-09-15 02:54:56
Also in: lkml

On Thu, 14 Sep 2023 15:11:02 +0200
Clément Léger [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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>
---
 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;
+		uaddr &= ~(sizeof(unsigned long) - 1);
+	}
 	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);
What we need are generic set_bit_aligned() and clear_bit_aligned(), which align the ptr
by unsigned long. (I think it should be done in set_bit/clear_bit, for architecture
which requires aligned access...)

#define LONG_ALIGN_DIFF(p)	(p) & (sizeof(long) -1)
#define LONG_ALINGNED(p)		(p) & ~(sizeof(long) - 1)

static inline void set_bit_aligned(int bit, unsigned long *ptr)
{
	int offs = LONG_ALIGN_DIFF(ptr) * 8;

#ifdef __BIGENDIAN
	if (bit >= offs) {
		set_bit(bit - offs, LONG_ALIGNED(ptr));
	} else {
		set_bit(bit + BITS_PER_LONG - offs, LONG_ALIGNED(ptr) + 1);
	}
#else
	if (bit < BITS_PER_LONG - offs) {
		set_bit(bit + offs, LONG_ALIGNED(ptr));
	} else {
		set_bit(bit - BITS_PER_LONG + offs, LONG_ALIGNED(ptr) + 1);
	}
#endif
}

And use it.

Thank you,
 
 	kunmap_local(kaddr);
 	unpin_user_pages_dirty_lock(&page, 1, true);
-- 
2.40.1

-- 
Masami Hiramatsu (Google) [off-list ref]
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help