Re: [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers from trace events

From: Steven Rostedt <rostedt@goodmis.org>
To: Peter Chen <peter.chen@kernel.org>
Cc: Pawel Laszczak <pawell@cadence.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Jacob Wen <jian.w.wen@oracle.com>,
	Felipe Balbi <balbi@kernel.org>,
	Greg KH <gregkh@linuxfoundation.org>
Subject: Re: [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers from trace events
Date: Tue, 2 Mar 2021 09:56:05 -0500	[thread overview]
Message-ID: <20210302095605.7b2881cd@gandalf.local.home> (raw)
In-Reply-To: <20210302082355.GA8633@nchen>

On Tue, 2 Mar 2021 16:23:55 +0800
Peter Chen <peter.chen@kernel.org> wrote:

s it looks like it uses %pa which IIUC from the printk code, it
> > >> dereferences the pointer to find it's virtual address. The event has
> > >> this as the field:
> > >>
> > >>                 __field(struct cdns3_trb *, start_trb_addr)
> > >>
> > >> Assigns it with:
> > >>
> > >>                 __entry->start_trb_addr = req->trb;
> > >>
> > >> And prints that with %pa, which will dereference pointer at the time of
> > >> reading, where the address in question may no longer be around. That
> > >> looks to me as a potential bug.  
> 
> Steven, thanks for reporting. Do you mind sending patch to fix it?
> If you have no time to do it, I will do it later.
> 

I would have already fixed it, but I wasn't exactly sure how this is used.

In Documentation/core-api/printk-formats.rst we have:

   Physical address types phys_addr_t
   ----------------------------------

   ::

           %pa[p]  0x01234567 or 0x0123456789abcdef

   For printing a phys_addr_t type (and its derivatives, such as
   resource_size_t) which can vary based on build options, regardless of the
   width of the CPU data path.

So it only looks like it is used to for the size of the pointer.

I guess something like this might work:

diff --git a/drivers/usb/cdns3/cdns3-trace.h b/drivers/usb/cdns3/cdns3-trace.h
index 8648c7a7a9dd..d3b8624fc427 100644
--- a/drivers/usb/cdns3/cdns3-trace.h
+++ b/drivers/usb/cdns3/cdns3-trace.h
@@ -214,7 +214,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request,
 		__field(int, no_interrupt)
 		__field(int, start_trb)
 		__field(int, end_trb)
-		__field(struct cdns3_trb *, start_trb_addr)
+		__field(phys_addr_t, start_trb_addr)
 		__field(int, flags)
 		__field(unsigned int, stream_id)
 	),
@@ -230,7 +230,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request,
 		__entry->no_interrupt = req->request.no_interrupt;
 		__entry->start_trb = req->start_trb;
 		__entry->end_trb = req->end_trb;
-		__entry->start_trb_addr = req->trb;
+		__entry->start_trb_addr = *(const phys_addr_t *)req->trb;
 		__entry->flags = req->flags;
 		__entry->stream_id = req->request.stream_id;
 	),
@@ -244,7 +244,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request,
 		__entry->status,
 		__entry->start_trb,
 		__entry->end_trb,
-		__entry->start_trb_addr,
+		/* %pa dereferences */ &__entry->start_trb_addr,
 		__entry->flags,
 		__entry->stream_id
 	)


Can you please test it? I don't have the hardware, but I also want to make
sure I don't break anything.

Thanks,

-- Steve
