On Fri, Mar 21, 2025 at 02:50:58AM -0700, Gleb Smirnoff wrote: > On Fri, Mar 21, 2025 at 05:34:16AM -0400, Mark Johnston wrote: > M> On Fri, Mar 21, 2025 at 01:56:01AM -0700, Gleb Smirnoff wrote: > M> > On Thu, Mar 20, 2025 at 07:52:19PM +0000, Bjoern A. Zeeb wrote: > M> > B> He's hitting a ... somewhere in i915kms.ko (here's the two instances I > M> > B> have): > M> > B> REDZONE: Buffer underflow detected. 16 bytes corrupted before > 0xfffffe089bc65000 (262148 bytes allocated). > M> > B> REDZONE: Buffer underflow detected. 16 bytes corrupted before > 0xfffffe08a7e70000 (262148 bytes allocated). > M> > > M> > I looked a bit into the problem and it actually seems very trivial to me. > M> > Please re-check my observations. > M> > > M> > A contigmalloc(9) allocation doesn't get redzone protection, see > kern_malloc.c. > M> > But free(9) always does contigmalloc check. This makes deprecation of > M> > contigfree(9) incompatible with redzone(9). And looks like > M> > 19df0c5abcb9d4e951e610b6de98d4d8a00bd5f9 is our first bump into this sad > fact. > M> > M> Can we not just add redzone padding to contigmalloc() allocations? > > I was about to suggest that, but was afraid it is too naive :) But > if that works, why not? We probably should document that for > contigmalloc() the redzone would provide protection of the virtual > space, but not the physical.
I'm not sure what you mean by this? As implemented, the patch effectively rounds up the allocation size, so the redzone will also be physically contiguous. Though, I see now that this will result in an non-page-aligned allocation, which callers of contigmalloc() might not tolerate... Actually, for malloc_large() and contigmalloc() allocations it's probably a bit easier to just provide guard pages around the allocation, like we do for kernel stacks. That is, if the caller asks for N pages, then allocate N+2 pages of virtual address space and back pages [1, N] with physical memory. Then any overflow will trap at the site of the overflow, which is probably more useful than what redzone(9). Actually, KASAN provides the same checking, but currently we don't pad allocations when KASAN is enabled. > M> Compile-tested patch below: > > Why do you increase the size with redzone_size_ntor() before passing > it to redzone_setup()? Other calls to redzone_setup() don't do that. > My reading is that argument to redzone_setup() shall be exactly the > amount of bytes requested by the malloc KPI user. If you pass > increased value, the canaries will be written after a gap. I pass "osize", the original allocation size, to redzone_setup().
