| Message ID | 20190702163840.2107-4-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xmalloc patches | expand |
On 02.07.2019 18:38, Paul Durrant wrote: > --- a/xen/Kconfig.debug > +++ b/xen/Kconfig.debug > @@ -105,6 +105,13 @@ config DEBUG_TRACE > either directly to the console or are printed to console in case of > a system crash. > > +config POOL_POISON May I suggest this to become XMEM_POOL_POISON, to parallel the names of the involved functions? > @@ -240,6 +244,10 @@ static inline void EXTRACT_BLOCK(struct bhdr *b, struct xmem_pool *p, int fl, > } > } > b->ptr.free_ptr = (struct free_ptr) {NULL, NULL}; > +#ifdef CONFIG_POOL_POISON > + for ( i = MIN_BLOCK_SIZE; i < (b->size & BLOCK_SIZE_MASK); i++ ) > + ASSERT(b->ptr.buffer[i] == 0xAA); > +#endif /* CONFIG_POOL_POISON */ May I further suggest that we clone Linux'es memchr_inv() for this purpose (perhaps with a more simplistic implementation along the lines of memchr())? We should then also use it in check_one_page(). Jan
> -----Original Message----- > From: Jan Beulich <JBeulich@suse.com> > Sent: 03 July 2019 11:07 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson > <Ian.Jackson@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH 3/3] xmalloc: add a Kconfig option to poison free pool memory > > On 02.07.2019 18:38, Paul Durrant wrote: > > --- a/xen/Kconfig.debug > > +++ b/xen/Kconfig.debug > > @@ -105,6 +105,13 @@ config DEBUG_TRACE > > either directly to the console or are printed to console in case of > > a system crash. > > > > +config POOL_POISON > > May I suggest this to become XMEM_POOL_POISON, to parallel the names > of the involved functions? > Sure. > > @@ -240,6 +244,10 @@ static inline void EXTRACT_BLOCK(struct bhdr *b, struct xmem_pool *p, int fl, > > } > > } > > b->ptr.free_ptr = (struct free_ptr) {NULL, NULL}; > > +#ifdef CONFIG_POOL_POISON > > + for ( i = MIN_BLOCK_SIZE; i < (b->size & BLOCK_SIZE_MASK); i++ ) > > + ASSERT(b->ptr.buffer[i] == 0xAA); > > +#endif /* CONFIG_POOL_POISON */ > > May I further suggest that we clone Linux'es memchr_inv() for this > purpose (perhaps with a more simplistic implementation along the > lines of memchr())? We should then also use it in check_one_page(). > Yes, that sounds like a worthy thing to do. Paul > Jan
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug index daacf85141..4f18a1144e 100644 --- a/xen/Kconfig.debug +++ b/xen/Kconfig.debug @@ -105,6 +105,13 @@ config DEBUG_TRACE either directly to the console or are printed to console in case of a system crash. +config POOL_POISON + bool "Poison free xenpool blocks" + default DEBUG + ---help--- + Poison free blocks with 0xAA bytes and verify them when a block is + allocated in order to spot use-after-free issues. + endif # DEBUG || EXPERT endmenu diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index 71597c3590..a12dbc8e11 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -223,6 +223,10 @@ static inline void EXTRACT_BLOCK_HDR(struct bhdr *b, struct xmem_pool *p, int fl static inline void EXTRACT_BLOCK(struct bhdr *b, struct xmem_pool *p, int fl, int sl) { +#ifdef CONFIG_POOL_POISON + unsigned int i; +#endif /* CONFIG_POOL_POISON */ + if ( b->ptr.free_ptr.next ) b->ptr.free_ptr.next->ptr.free_ptr.prev = b->ptr.free_ptr.prev; @@ -240,6 +244,10 @@ static inline void EXTRACT_BLOCK(struct bhdr *b, struct xmem_pool *p, int fl, } } b->ptr.free_ptr = (struct free_ptr) {NULL, NULL}; +#ifdef CONFIG_POOL_POISON + for ( i = MIN_BLOCK_SIZE; i < (b->size & BLOCK_SIZE_MASK); i++ ) + ASSERT(b->ptr.buffer[i] == 0xAA); +#endif /* CONFIG_POOL_POISON */ } /** @@ -247,6 +255,11 @@ static inline void EXTRACT_BLOCK(struct bhdr *b, struct xmem_pool *p, int fl, */ static inline void INSERT_BLOCK(struct bhdr *b, struct xmem_pool *p, int fl, int sl) { +#ifdef CONFIG_POOL_POISON + if ( (b->size & BLOCK_SIZE_MASK) > MIN_BLOCK_SIZE ) + memset(b->ptr.buffer + MIN_BLOCK_SIZE, 0xAA, + (b->size & BLOCK_SIZE_MASK) - MIN_BLOCK_SIZE); +#endif /* CONFIG_POOL_POISON */ b->ptr.free_ptr = (struct free_ptr) {NULL, p->matrix[fl][sl]}; if ( p->matrix[fl][sl] ) p->matrix[fl][sl]->ptr.free_ptr.prev = b;
This patch adds POOL_POISON to the Kconfig DEBUG options. If set, free blocks (greater than MIN_BLOCK_SIZE) will be poisoned with 0xAA bytes which will then be verified when memory is subsequently allocated. This can help in spotting heap corruption, particularly use-after-free. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wl@xen.org> --- xen/Kconfig.debug | 7 +++++++ xen/common/xmalloc_tlsf.c | 13 +++++++++++++ 2 files changed, 20 insertions(+)