On 7/19/22 15:45, Feng Tang wrote:
> Hi Vlastimil,
>
> On Fri, Jul 15, 2022 at 04:29:22PM +0800, Tang, Feng wrote:
> [...]
>> > >> - the knowledge of actual size could be used to improve poisoning checks as
>> > >> well, detect cases when there's buffer overrun over the orig_size but not
>> > >> cache's size. e.g. if you kmalloc(48) and overrun up to 64 we won't detect
>> > >> it now, but with orig_size stored we could?
>> > >
>> > > The above patch doesn't touch this. As I have a question, for the
>> > > [orib_size, object_size) area, shall we fill it with POISON_XXX no matter
>> > > REDZONE flag is set or not?
>> >
>> > Ah, looks like we use redzoning, not poisoning, for padding from
>> > s->object_size to word boundary. So it would be more consistent to use the
>> > redzone pattern (RED_ACTIVE) and check with the dynamic orig_size. Probably
>> > no change for RED_INACTIVE handling is needed though.
>>
>> Thanks for clarifying, will go this way and do more test. Also I'd
>> make it a separate patch, as it is logically different from the space
>> wastage.
>
> I made a draft to redzone the wasted space, which basically works (patch
> pasted at the end of the mail) as detecting corruption of below test code:
>
> size = 256;
> buf = kmalloc(size + 8, GFP_KERNEL);
> memset(buf + size + size/2, 0xff, size/4);
> print_section(KERN_ERR, "Corruptted-kmalloc-space", buf, size * 2);
> kfree(buf);
>
> However when it is enabled globally, there are many places reporting
> corruption. I debugged one case, and found that the network(skb_buff)
> code already knows this "wasted" kmalloc space and utilize it which is
> detected by my patch.
>
> The allocation stack is:
>
> [ 0.933675] BUG kmalloc-2k (Not tainted): kmalloc unused part overwritten
> [ 0.933675] -----------------------------------------------------------------------------
> [ 0.933675]
> [ 0.933675] 0xffff888237d026c0-0xffff888237d026e3 @offset=9920. First byte 0x0 instead of 0xcc
> [ 0.933675] Allocated in __alloc_skb+0x8e/0x1d0 age=5 cpu=0 pid=1
> [ 0.933675] __slab_alloc.constprop.0+0x52/0x90
> [ 0.933675] __kmalloc_node_track_caller+0x129/0x380
> [ 0.933675] kmalloc_reserve+0x2a/0x70
> [ 0.933675] __alloc_skb+0x8e/0x1d0
> [ 0.933675] audit_buffer_alloc+0x3a/0xc0
> [ 0.933675] audit_log_start.part.0+0xa3/0x300
> [ 0.933675] audit_log+0x62/0xc0
> [ 0.933675] audit_init+0x15c/0x16f
>
> And the networking code which touches the [orig_size, object_size) area
> is in __build_skb_around(), which put a 'struct skb_shared_info' at the
> end of this area:
>
> static void __build_skb_around(struct sk_buff *skb, void *data,
> unsigned int frag_size)
> {
> struct skb_shared_info *shinfo;
> unsigned int size = frag_size ? : ksize(data);
Hmm so it's a ksize() user, which should be legitimate way to use the
"waste" data. Hopefully it should be then enough to patch __ksize() to set
the object's tracked waste to 0 (orig_size to size) - assume that if
somebody called ksize() they intend to use the space. That would also make
the debugfs report more truthful.
