[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 3/3] fuzz: Add callbacks for dma-access functions
From: |
Stefan Hajnoczi |
Subject: |
Re: [RFC PATCH 3/3] fuzz: Add callbacks for dma-access functions |
Date: |
Fri, 26 Jun 2020 16:44:41 +0100 |
On Tue, Jun 23, 2020 at 10:55:00AM -0400, Alexander Bulekov wrote:
> On 200623 1514, Stefan Hajnoczi wrote:
> > On Thu, Jun 11, 2020 at 01:56:51AM -0400, Alexander Bulekov wrote:
> > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > > ---
> > > exec.c | 17 ++++++++++++++++-
> > > include/exec/memory.h | 8 ++++++++
> > > include/exec/memory_ldst_cached.inc.h | 9 +++++++++
> > > include/sysemu/dma.h | 5 ++++-
> > > memory_ldst.inc.c | 12 ++++++++++++
> > > 5 files changed, 49 insertions(+), 2 deletions(-)
> >
> > Please rename dma_read_cb() to fuzz_dma_read_cb() so the purpose of the
> > function is clear.
> >
> > The ifdefs can be avoided by defining an empty function when CONFIG_FUZZ
> > is undefined. In a header file:
> >
> > #ifdef CONFIG_FUZZ
> > void fuzz_dma_read_cb(size_t addr, size_t len);
> > #else
> > static inline void fuzz_dma_read_cb(size_t addr, size_t len)
> > {
> > /* Do nothing */
> > }
> > #endif
> >
> > Now the compiler should eliminate the deadcode:
> >
> > #ifdef CONFIG_FUZZ
> > if (as->root == get_system_memory()) {
> > dma_read_cb(addr, len);
> > }
> > #endif
> >
> > becomes:
> >
> > if (as->root == get_system_memory()) {
> > fuzz_dma_read_cb(addr, len);
> > }
> >
> > Hopefully gcc and clang will eliminate this and emit no instructions
> > when CONFIG_FUZZ is undefined. If not, you can simply pass in 'as' and
> > 'is_write' too:
> >
> > void fuzz_dma_read_cb(AddressSpace *as, bool is_write, size_t addr,
> > size_t len)
> >
> > This way the conditional is moved inside fuzz_dma_read_cb() and deadcode
> > elimination becomes trivial for the compiler:
> >
> > fuzz_read_cb(as, is_write, addr, len);
>
> Do you think it would be better to have a "trace_dma_read" or
> "trace_device_read_from_guest_memory"? I haven't looked under the hood
> too much for the tracepoint api, but these would just be standard
> tracepoints(disabled for the majority of builds). When we build the
> fuzzer, we could compile with --wrap="trace_dma_read" and implement
> a __wrap_trace_dma_read in the generic fuzzer. I looked at the symbols
> for a qemu build and it looks like trace_* are actual functions, rather
> than preprocessor magic, so maybe this could work?
I think plain old functions are fine for what you are doing.
QEMU's tracing does not provide callbacks that are invoked when a trace
event is emitted. The fuzzing code wouldn't be able to hook
trace_device_read_from_guest_memory, you could only analyze a trace
after the fact.
Stefan
signature.asc
Description: PGP signature