Re: [PATCH v5 4/4] mm/slub: extend redzone check to extra allocated kmalloc space than requested

[Feng Tang <feng.tang@intel.com>]

On Fri, Sep 09, 2022 at 02:26:34PM +0800, Hyeonggon Yoo wrote:
> On Wed, Sep 07, 2022 at 03:10:23PM +0800, Feng Tang wrote:
> > kmalloc will round up the request size to a fixed size (mostly power
> > of 2), so there could be a extra space than what is requested, whose
> > size is the actual buffer size minus original request size.
> > 
> > To better detect out of bound access or abuse of this space, add
> > redzone sanity check for it.
> > 
> > And in current kernel, some kmalloc user already knows the existence
> > of the space and utilizes it after calling 'ksize()' to know the real
> > size of the allocated buffer. So we skip the sanity check for objects
> > which have been called with ksize(), as treating them as legitimate
> > users.
> > 
> > Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > ---
[...]

> > -	if (s->flags & SLAB_RED_ZONE)
> > +	if (s->flags & SLAB_RED_ZONE) {
> >  		memset(p - s->red_left_pad, val, s->red_left_pad);
> >  
> > +		if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) {
> > +			unsigned int zone_start;
> > +
> > +			orig_size = get_orig_size(s, object);
> > +			zone_start = orig_size;
> > +
> > +			if (!freeptr_outside_object(s))
> > +				zone_start = max_t(unsigned int, orig_size,
> > +						s->offset + sizeof(void *));
> > +
> > +			/*
> > +			 * Redzone the extra allocated space by kmalloc
> > +			 * than requested.
> > +			 */
> > +			if (zone_start < s->object_size)
> > +				memset(p + zone_start, val,
> > +					s->object_size - zone_start);
> > +		}
> > +	}
> > +
> >  	if (s->flags & __OBJECT_POISON) {
> > -		memset(p, POISON_FREE, s->object_size - 1);
> > -		p[s->object_size - 1] = POISON_END;
> > +		memset(p, POISON_FREE, orig_size - 1);
> > +		p[orig_size - 1] = POISON_END;
> >  	}
> >  
> >  	if (s->flags & SLAB_RED_ZONE)
> > @@ -1103,6 +1139,7 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
> >  {
> >  	u8 *p = object;
> >  	u8 *endobject = object + s->object_size;
> > +	unsigned int orig_size;
> >  
> >  	if (s->flags & SLAB_RED_ZONE) {
> >  		if (!check_bytes_and_report(s, slab, object, "Left Redzone",
> > @@ -1112,6 +1149,20 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
> >  		if (!check_bytes_and_report(s, slab, object, "Right Redzone",
> >  			endobject, val, s->inuse - s->object_size))
> >  			return 0;
> > +
> > +		if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) {
> > +			orig_size = get_orig_size(s, object);
> > +
> > +			if (!freeptr_outside_object(s))
> > +				orig_size = max_t(unsigned int, orig_size,
> > +						s->offset + sizeof(void *));
> > +			if (s->object_size > orig_size  &&
> > +				!check_bytes_and_report(s, slab, object,
> > +					"kmalloc Redzone", p + orig_size,
> > +					val, s->object_size - orig_size)) {
> > +				return 0;
> > +			}
> > +		}
> >  	} else {
> >  		if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) {
> >  			check_bytes_and_report(s, slab, p, "Alignment padding",
> > -- 
> > 2.34.1
> > 
> 
> Looks good, but what about putting
> free pointer outside object when slub_debug_orig_size(s)?
 
Sounds good to me. This makes all kmalloc slabs covered by redzone
check. I just gave the code a shot and it just works with my test
case! Thanks!

- Feng


> diff --git a/mm/slub.c b/mm/slub.c
> index 9d1a985c9ede..7e57d9f718d1 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -970,22 +970,15 @@ static void init_object(struct kmem_cache *s, void *object, u8 val)
>  		memset(p - s->red_left_pad, val, s->red_left_pad);
>  
>  		if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) {
> -			unsigned int zone_start;
> -
>  			orig_size = get_orig_size(s, object);
> -			zone_start = orig_size;
> -
> -			if (!freeptr_outside_object(s))
> -				zone_start = max_t(unsigned int, orig_size,
> -						s->offset + sizeof(void *));
>  
>  			/*
>  			 * Redzone the extra allocated space by kmalloc
>  			 * than requested.
>  			 */
> -			if (zone_start < s->object_size)
> -				memset(p + zone_start, val,
> -					s->object_size - zone_start);
> +			if (orig_size < s->object_size)
> +				memset(p + orig_size, val,
> +				       s->object_size - orig_size);
>  		}
>  	}
>  
> @@ -1153,9 +1146,6 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>  		if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) {
>  			orig_size = get_orig_size(s, object);
>  
> -			if (!freeptr_outside_object(s))
> -				orig_size = max_t(unsigned int, orig_size,
> -						s->offset + sizeof(void *));
>  			if (s->object_size > orig_size  &&
>  				!check_bytes_and_report(s, slab, object,
>  					"kmalloc Redzone", p + orig_size,
> @@ -4234,7 +4224,8 @@ static int calculate_sizes(struct kmem_cache *s)
>  	 */
>  	s->inuse = size;
>  
> -	if ((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
> +	if (slub_debug_orig_size(s) ||
> +	    (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
>  	    ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) ||
>  	    s->ctor) {
>  		/*
> 
> -- 
> Thanks,
> Hyeonggon
>