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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210303012139.GA11703@nchen>
Date:   Wed, 3 Mar 2021 09:21:39 +0800
From:   Peter Chen <peter.chen@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Pawel Laszczak <pawell@...ence.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Jacob Wen <jian.w.wen@...cle.com>,
        Felipe Balbi <balbi@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>
Subject: Re: [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers
 from trace events

On 21-03-02 09:56:05, Steven Rostedt wrote:
> On Tue, 2 Mar 2021 16:23:55 +0800
> Peter Chen <peter.chen@...nel.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.
> 

Hi Steve,

Regarding this issue, I have one question:
- If the virtual address is got from dma_alloc_coherent, can't we print
this address using %pa to get its physical address (the same with DMA address),
or its DMA address using %pad? req->trb is the virtual address got from
dma_alloc_coherent. And what's the logic for this "unsafe dereference" warning?
Thanks.

-- 

Thanks,
Peter Chen