Messages in this thread Patch in this message | | | Date | Tue, 2 Mar 2021 09:56:05 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers from trace events |
| |
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
| |