| Message ID | 20190705090249.1935-3-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xmalloc patches | expand |
On 05.07.2019 11:02, 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 XMEM_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. There looks to be a spaces vs tabs problem here: Only the help text has tabs for initial indentation. > --- a/xen/common/xmalloc_tlsf.c > +++ b/xen/common/xmalloc_tlsf.c > @@ -238,6 +238,11 @@ 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_XMEM_POOL_POISON > + if ( (b->size & BLOCK_SIZE_MASK) > MIN_BLOCK_SIZE ) > + ASSERT(!memchr_inv(b->ptr.buffer + MIN_BLOCK_SIZE, 0xAA, > + (b->size & BLOCK_SIZE_MASK) - MIN_BLOCK_SIZE)); > +#endif /* CONFIG_XMEM_POOL_POISON */ > } > > /** > @@ -245,6 +250,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_XMEM_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_XMEM_POOL_POISON */ Can you tie together the two instances of 0xAA via a #define, please? It would also be nice if both #ifdef blocks were separated from their neighboring code by a blank line, despite the file using blank lines rather sparingly so far. With the adjustments here I think it would be best if you could also resend patch 1 with the slightly adjusted commit message. Jan
> -----Original Message----- > From: Jan Beulich <JBeulich@suse.com> > Sent: 05 July 2019 14:41 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: 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 v2 2/2] xmalloc: add a Kconfig option to poison free pool memory > > On 05.07.2019 11:02, 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 XMEM_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. > > There looks to be a spaces vs tabs problem here: Only the help > text has tabs for initial indentation. Oh yes. I'll fix that. > > > --- a/xen/common/xmalloc_tlsf.c > > +++ b/xen/common/xmalloc_tlsf.c > > @@ -238,6 +238,11 @@ 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_XMEM_POOL_POISON > > + if ( (b->size & BLOCK_SIZE_MASK) > MIN_BLOCK_SIZE ) > > + ASSERT(!memchr_inv(b->ptr.buffer + MIN_BLOCK_SIZE, 0xAA, > > + (b->size & BLOCK_SIZE_MASK) - MIN_BLOCK_SIZE)); > > +#endif /* CONFIG_XMEM_POOL_POISON */ > > } > > > > /** > > @@ -245,6 +250,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_XMEM_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_XMEM_POOL_POISON */ > > Can you tie together the two instances of 0xAA via a #define, > please? > Ok, sure. > It would also be nice if both #ifdef blocks were separated from > their neighboring code by a blank line, despite the file using > blank lines rather sparingly so far. > > With the adjustments here I think it would be best if you could > also resend patch 1 with the slightly adjusted commit message. Alright, will do. Paul > > Jan
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug index daacf85141..fe5792a3d0 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 XMEM_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/string.c b/xen/common/string.c index a2bbe7dc97..af3d96ad0f 100644 --- a/xen/common/string.c +++ b/xen/common/string.c @@ -423,6 +423,26 @@ void *(memchr)(const void *s, int c, size_t n) } #endif +/** + * memchr_inv - Find an unmatching character in an area of memory. + * @s: The memory area + * @c: The byte that is expected + * @n: The size of the area. + * + * returns the address of the first occurrence of a character other than @c, + * or %NULL if the whole buffer contains just @c. + */ +void *memchr_inv(const void *s, int c, size_t n) +{ + const unsigned char *p = s; + + while (n--) + if ((unsigned char)c != *p++) + return (void *)(p - 1); + + return NULL; +} + /* * Local variables: * mode: C diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index e4e476a27c..c5f5611c63 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -238,6 +238,11 @@ 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_XMEM_POOL_POISON + if ( (b->size & BLOCK_SIZE_MASK) > MIN_BLOCK_SIZE ) + ASSERT(!memchr_inv(b->ptr.buffer + MIN_BLOCK_SIZE, 0xAA, + (b->size & BLOCK_SIZE_MASK) - MIN_BLOCK_SIZE)); +#endif /* CONFIG_XMEM_POOL_POISON */ } /** @@ -245,6 +250,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_XMEM_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_XMEM_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; diff --git a/xen/include/xen/string.h b/xen/include/xen/string.h index 711cb60a7d..4b3b57e74f 100644 --- a/xen/include/xen/string.h +++ b/xen/include/xen/string.h @@ -106,6 +106,8 @@ void *memchr(const void *, int, size_t); #define memchr(s, c, n) __builtin_memchr(s, c, n) #endif +void *memchr_inv(const void *, int, size_t); + #define is_char_array(x) __builtin_types_compatible_p(typeof(x), char[]) /* safe_xxx always NUL-terminates and returns !=0 if result is truncated. */
This patch adds XMEM_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> v2: - Change Kconfig option name to XMEM_POOL_POISON - Add an implementation of memchr_inv() and use that --- xen/Kconfig.debug | 7 +++++++ xen/common/string.c | 20 ++++++++++++++++++++ xen/common/xmalloc_tlsf.c | 10 ++++++++++ xen/include/xen/string.h | 2 ++ 4 files changed, 39 insertions(+)