Page MenuHomeFreeBSD

uma: Insert KASAN redzones after slab-allocated items
Needs ReviewPublic

Authored by markj on Wed, Jul 15, 8:57 PM.
Tags
None
Referenced Files
F163594224: D58271.diff
Fri, Jul 24, 7:44 PM
F163594050: D58271.diff
Fri, Jul 24, 7:41 PM
F163519163: D58271.diff
Fri, Jul 24, 1:42 AM
F163519143: D58271.diff
Fri, Jul 24, 1:42 AM
Unknown Object (File)
Wed, Jul 22, 8:21 PM
Unknown Object (File)
Wed, Jul 22, 1:07 AM
Unknown Object (File)
Tue, Jul 21, 5:09 PM
Unknown Object (File)
Tue, Jul 21, 3:01 AM
Subscribers

Details

Reviewers
alc
rlibby
Summary

Without this, KASAN has the deficiency that inter-object overflows are
not detected most of the time[*] when keg_layout() is able to perfectly
pack a slab. Try to overcome this by adjusting the allocation size to
include a redzone following the object.

With this change, we automatically get a redzone following each item, so
any overflow into the redzone will trigger a panic. Most of UMA doesn't
need to know about this: at slab allocation time, the whole slab is
poisoned, and then kasan_mark_item_valid() will unpoison only the buffer
that is available to the consumer.

Note that in most zones, most objects will follow another object's
redzone, so there is some protection against underflow as well. It
might be worthwhile to provide a stronger guarantee here.

Add an assertion to item_ctor() that the returned item is properly
aligned. I couldn't see any pre-existing checks which verify this.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 74841
Build 71724: arc lint + arc unit

Event Timeline

markj requested review of this revision.Wed, Jul 15, 8:57 PM

Add an assertion to item_ctor() that the returned item is properly
aligned. I couldn't see any pre-existing checks which verify this.

It comes from the math of slab_item(), although that relies on slab_data() being aligned which is technically not asserted. I don't object to the new assert, and it should be cheap anyway. From a quick scan, I also don't see any check that the alignment is not larger than page size.

sys/vm/uma_core.c
541–542

(Not new, but I don't get why this would be a requirement, we already seem to do all the necessary rounding.)

2270–2271

The size passed to slab_ipers_hdr needs to be the redzone request size.

2315–2317

Consider whether you find it more readable to keep the alignsize and size adjustments each together with two ifdef KASAN blocks or whether you find it more readable to put the KASAN adjustments together under one ifdef KASAN.

Also, shouldn't the alignment adjustment technically also be guarded by a UMA_ZONE_NOKASAN check? Not that it actually makes a difference, due to UMA_SMALLEST_UNIT.

2324–2334

I would suggest reworking the first comment, and maybe integrating the second comment. We now have three sizes to consider:

  • uk_size, the original request size, which the client can actually use,
  • size, the redzone request size, which for layout we want to treat as the request size to ensure it is always present,
  • rsize = uk_rsize, the aligned object size -- which for the last item in a slab is not necessarily the "real" size.

Here's a hack, for your consideration (feel free to rework further):

	/*
	 * Calculate the size of each allocation.  uk_size is the original
	 * request size that the client may use.  size is the request size
	 * after adjusting for an optional redzone after each item.  rsize is
	 * the final size between item start addresses, after adjusting for
	 * required item alignment and the UMA minimum allocation size.  The
	 * difference between rsize and size may not be present for the last
	 * item in a slab.
	 */
2329–2337

The outcome as you have it is fine, but I think logically these should be the other order.

size = keg->uk_size;
#ifdef KASAN
if (...)
    size += KASAN_SHADOW_SCALE;
#endif
size = MAX(size, UMA_SMALLEST_UNIT);
rsize = roundup2(size, alignsize);

Imagine UMA_SMALLEST_UNIT were much larger. You don't need to add a quantum to that much larger minimum size, you can already use it as redzone.

2335–2336

I don't understand why the PCPU check is here. Can you explain?

I also wonder if we should have an option to add this redzone without KASAN. We could come up with a cheap-ish debugging feature to verify the redzone with uma_junk when we don't have KASAN. Not asking for that in this patch.