RE: [PATCH v2] tracing: Add test for user space strings when filtering on string pointers

From: David Laight <David.Laight@ACULAB.COM>
To: 'Sven Schnelle' <svens@linux.ibm.com>,
	Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Pingfan Liu <kernelfans@gmail.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Tom Zanussi <zanussi@kernel.org>,
	"hca@linux.ibm.com" <hca@linux.ibm.com>,
	"deller@gmx.de" <deller@gmx.de>
Subject: RE: [PATCH v2] tracing: Add test for user space strings when filtering on  string pointers
Date: Thu, 13 Jan 2022 22:11:35 +0000	[thread overview]
Message-ID: <e66911061f9344fa8de2da982ab98a7b@AcuMS.aculab.com> (raw)
In-Reply-To: <yt9dwnj3wcke.fsf@linux.ibm.com>

From: Sven Schnelle
> Sent: 13 January 2022 21:28
> 
> Hi Steve,
> 
> Steven Rostedt <rostedt@goodmis.org> writes:
> 
> > On Tue, 11 Jan 2022 21:55:53 +0100
> > Sven Schnelle <svens@linux.ibm.com> wrote:
> >
> >> > Isn't there also at least one architecture where you can't differentiate
> >> > between user and kernel pointers by looking at the address?
> >> > (Something like sparc ASI is used for user accesses so both user
> >> > and kernel get the full 4G address range. But it isn't sparc (or pdp/11))
> >> > ISTR it causing issues with the code for kernel_setsockopt() and
> >> > required a separate flag.
> >>
> >> On s390 TASK_SIZE is defined as -PAGE_SIZE, so with the patch above the
> >> kernel would always try to fetch it from user space. I think it would be
> >> the same for parisc.
> >
> > As a work around for these cases, would something like this work?
> 
> Hmm, i don't see how. On s390, TASK_SIZE is -PAGE_SIZE, which means
> 0xfffffffffffff000 so i think the if() condition below is always true.

Isn't TASK_SIZE also dependant on current->xxxx on at least one architecture?
Possibly even x86-64.

> Too bad that the __user attribute is stripped during a normal compile.
> But couldn't we add the information whether a pointer belongs to user
> or kernel space in the trace event definition? For syscall tracing it's
> easy, because pointer types in SYSCALL_DEFINE() and friends are always
> userspace pointers?

Also, when the __user pointer is saved it MUST be checked for being
a valid user pointer (eg with access_ok(ptr, 1).

You really do need to remember whether the pointer is user or kernel
when you save it.

I also suspect that you need to check for contexts where 'current'
isn't really valid (eg any kind on interrupt) and ensure the user
reads aren't even attempted.
The excuse of being 'root' in order to request/read trace isn't
really a very good one.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)