Re: [PATCH] fgraph: record function return value

[Changbin Du]

Hi Matthew,
On Sat, Jan 12, 2019 at 04:21:13AM -0800, Matthew Wilcox wrote:
> On Sat, Jan 12, 2019 at 02:57:01PM +0800, Changbin Du wrote:
> > This patch adds a new trace option 'funcgraph-retval' and is disabled by
> > default. When this option is enabled, fgraph tracer will show the return
> > value of each function. This is useful to find/analyze a original error
> > source in a call graph.
> > 
> > One limitation is that kernel doesn't know the prototype of functions. So
> > fgraph assumes all functions have a retvalue of type int. You must ignore
> > the value of *void* function. And if the retvalue looks like an error code
> > then both hexadecimal and decimal number are displayed.
> 
> I don't think we can do this.  You're leaking a _lot_ of kernel addresses
> this way, and we've been trying very hard to avoid doing that because
> it gives a lot of information to attackers.
> 
> Something more clever that prints out only errors (ie IS_ERR_VALUE())
> might be acceptable.  I would think printing return values that are
> between 0 and 4095 should also be OK since they can't be real pointers.
> We'd leak whether a function called kmalloc(0, x) since that returns
> 16, but that seems like a not-very-useful information leak.
> 
Regarding to security concern, I don't think this is a problem. Because using
ftrace requires *root* permission. And if the attack has got root previlege,
he/her can obtain nearly all kernel inner information through variant tools.
For example, we can write an eBPF program to probe kernel functions and we
just need root previlege to load our program. We also can use the *crash*
tool to retreive kernel data.

> >  3)   0.247 us    |    mutex_unlock(); /* ret=0xffff8880738ed040 */
> >  3)               |    kvm_arch_vcpu_create() {
> >  3)               |      vmx_create_vcpu() {
> >  3) + 17.969 us   |        kmem_cache_alloc(); /* ret=0xffff88813a980040 */
> >  3) + 15.948 us   |        kmem_cache_alloc(); /* ret=0xffff88813aa99200 */
> >  3)   0.653 us    |        allocate_vpid.part.88(); /* ret=0x1 */
> >  3)   6.964 us    |        kvm_vcpu_init(); /* ret=0xfffffffb */
> >  3)   0.323 us    |        free_vpid.part.89(); /* ret=0x1 */
> >  3)   9.985 us    |        kmem_cache_free(); /* ret=0x80000000 */
> >  3)   9.491 us    |        kmem_cache_free(); /* ret=0x80000000 */
> >  3) + 69.858 us   |      } /* ret=0xfffffffffffffffb/-5 */
> >  3) + 70.631 us   |    } /* ret=0xfffffffffffffffb/-5 */
> >  3)               |    mutex_lock() {
> >  3)               |      _cond_resched() {
> >  3)   0.199 us    |        rcu_all_qs(); /* ret=0x80000000 */
> >  3)   0.594 us    |      } /* ret=0x0 */
> >  3)   1.067 us    |    } /* ret=0x0 */
> >  3)   0.337 us    |    mutex_unlock(); /* ret=0xffff8880738ed040 */
> >  3) + 92.730 us   |  } /* ret=0xfffffffffffffffb/-5 */

-- 
Cheers,
Changbin Du