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(+)

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;
-- 
2.20.1.2.gb21ebb671


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

> -----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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel