Re: [PATCH] mm: vmalloc: Support more granular vrealloc() sizing

[Kees Cook <kees@kernel.org>]

On Thu, Apr 24, 2025 at 11:11:47AM +0200, Danilo Krummrich wrote:
> On Wed, Apr 23, 2025 at 07:31:23PM -0700, Kees Cook wrote:
> > Introduce struct vm_struct::requested_size so that the requested
> > (re)allocation size is retained separately from the allocated area
> > size. This means that KASAN will correctly poison the correct spans
> > of requested bytes. This also means we can support growing the usable
> > portion of an allocation that can already be supported by the existing
> > area's existing allocation.
> > 
> > Reported-by: Erhard Furtner <erhard_f@mailbox.org>
> > Closes: https://lore.kernel.org/all/20250408192503.6149a816@outsider.home/
> > Fixes: 3ddc2fefe6f3 ("mm: vmalloc: implement vrealloc()")
> > Signed-off-by: Kees Cook <kees@kernel.org>
> 
> Good catch!
> 
> One question below, otherwise
> 
> 	Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> 
> > @@ -4088,14 +4093,27 @@ void *vrealloc_noprof(const void *p, size_t size, gfp_t flags)
> >  	 * would be a good heuristic for when to shrink the vm_area?
> >  	 */
> >  	if (size <= old_size) {
> > -		/* Zero out spare memory. */
> > -		if (want_init_on_alloc(flags))
> > +		/* Zero out "freed" memory. */
> > +		if (want_init_on_free())
> >  			memset((void *)p + size, 0, old_size - size);
> > +		vm->requested_size = size;
> >  		kasan_poison_vmalloc(p + size, old_size - size);
> >  		kasan_unpoison_vmalloc(p, size, KASAN_VMALLOC_PROT_NORMAL);
> >  		return (void *)p;
> >  	}
> >  
> > +	/*
> > +	 * We already have the bytes available in the allocation; use them.
> > +	 */
> > +	if (size <= alloced_size) {
> > +		kasan_unpoison_vmalloc(p, size, KASAN_VMALLOC_PROT_NORMAL);
> > +		/* Zero out "alloced" memory. */
> > +		if (want_init_on_alloc(flags))
> > +			memset((void *)p + old_size, 0, size - old_size);
> > +		vm->requested_size = size;
> > +		kasan_poison_vmalloc(p + size, alloced_size - size);
> 
> Do we need this? We know that old_size < size <= alloced_size. And since
> previously [p + old_size, p + alloced_size) must have been poisoned,
> [p + size, p + alloced_size) must be poisoned already?
> 
> Maybe there was a reason, since in the above (size <= old_size) case
> kasan_unpoison_vmalloc() seems unnecessary too.

Honestly I was just copying the logic from the prior case. But yeah, it
should be possible (in both cases) to just apply the changed span. For
the "size <= old_size" case, it would just be:

	kasan_poison_vmalloc(p + size, old_size - size);

(i.e. the kasan_unpoison_vmalloc() call isn't needed at all, as you say.)

And in the "size <= alloced_size" case, it would just be:

	kasan_unpoison_vmalloc(p + old_size, size - old_size, KASAN_VMALLOC_PROT_NORMAL);

and no kasan_poison_vmalloc() should be needed.

Do the KASAN folks on CC have any opinion on best practices here?

Thanks for looking it over!

-Kees

-- 
Kees Cook