1. Patchew
  2. Xen
  3. [Xen-devel] [PATCH v2 0/2] xmalloc patches
  4. View patch

[Xen-devel] [PATCH v2 2/2] xmalloc: add a Kconfig option to poison free pool memory

Paul Durrant posted 2 patches 6 years, 7 months ago
There is a newer version of this series
[Xen-devel] [PATCH v2 2/2] xmalloc: add a Kconfig option to poison free pool memory
Posted by Paul Durrant 6 years, 7 months ago 
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(+)

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. */
-- 
2.20.1.2.gb21ebb671


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xmalloc: add a Kconfig option to poison free pool memory
Posted by Jan Beulich 6 years, 7 months ago 
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xmalloc: add a Kconfig option to poison free pool memory
Posted by Paul Durrant 6 years, 7 months ago 
> -----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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.