linux-kernel - Re: [PATCH v2 0/5] mm/slub: Improve data handling of krealloc() when orig

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zyiv40cZcaCKlGtM@feng-clx.sh.intel.com>
Date: Mon, 4 Nov 2024 19:28:35 +0800
From: Feng Tang <feng.tang@...el.com>
To: Vlastimil Babka <vbabka@...e.cz>, Kees Cook <keescook@...omium.org>
CC: Kees Cook <keescook@...omium.org>, Marco Elver <elver@...gle.com>, "Andrew
 Morton" <akpm@...ux-foundation.org>, Christoph Lameter <cl@...ux.com>, "Pekka
 Enberg" <penberg@...nel.org>, David Rientjes <rientjes@...gle.com>, "Joonsoo
 Kim" <iamjoonsoo.kim@....com>, Roman Gushchin <roman.gushchin@...ux.dev>,
	Hyeonggon Yoo <42.hyeyoo@...il.com>, Andrey Konovalov <andreyknvl@...il.com>,
	Shuah Khan <skhan@...uxfoundation.org>, David Gow <davidgow@...gle.com>,
	Danilo Krummrich <dakr@...nel.org>, Alexander Potapenko <glider@...gle.com>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>, Dmitry Vyukov <dvyukov@...gle.com>,
	Vincenzo Frascino <vincenzo.frascino@....com>, "linux-mm@...ck.org"
	<linux-mm@...ck.org>, "kasan-dev@...glegroups.com"
	<kasan-dev@...glegroups.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH v2 0/5] mm/slub: Improve data handling of krealloc() when
 orig_size is enabled

On Mon, Oct 14, 2024 at 10:20:36PM +0800, Tang, Feng wrote:
> On Mon, Oct 14, 2024 at 03:12:09PM +0200, Vlastimil Babka wrote:
> > > 
> > >> So I think in __do_krealloc() we should do things manually to determine ks
> > >> and not call ksize(). Just not break any of the cases ksize() handles
> > >> (kfence, large kmalloc).
> > > 
> > > OK, originally I tried not to expose internals of __ksize(). Let me
> > > try this way.
> > 
> > ksize() makes assumptions that a user outside of slab itself is calling it.
> > 
> > But we (well mostly Kees) also introduced kmalloc_size_roundup() to avoid
> > querying ksize() for the purposes of writing beyond the original
> > kmalloc(size) up to the bucket size. So maybe we can also investigate if the
> > skip_orig_size_check() mechanism can be removed now?
> 
> I did a quick grep, and fortunately it seems that the ksize() user are
> much less than before. We used to see some trouble in network code, which
> is now very clean without the need to skip orig_size check. Will check
> other call site later.
 

I did more further check about ksize() usage, and there are still some
places to be handled. The thing stands out is kfree_sensitive(), and
another potential one is sound/soc/codecs/cs-amp-lib-test.c

Some details:

* Thanks to Kees Cook, who has cured many cases of ksize() as below:
  
  drivers/base/devres.c:        total_old_size = ksize(container_of(ptr, struct devres, data));
  drivers/net/ethernet/intel/igb/igb_main.c:        } else if (size > ksize(q_vector)) {   
  net/core/skbuff.c:        *size = ksize(data);
  net/openvswitch/flow_netlink.c:        new_acts_size = max(next_offset + req_size, ksize(*sfa) * 2);
  kernel/bpf/verifier.c:        alloc_bytes = max(ksize(orig), kmalloc_size_roundup(bytes));

* Some callers use ksize() mostly for calculation or sanity check,
  and not for accessing those extra space, which are fine:

  drivers/gpu/drm/drm_managed.c:        WARN_ON(dev + 1 > (struct drm_device *) (container + ksize(container)));
  lib/kunit/string-stream-test.c:        actual_bytes_used = ksize(stream);
  lib/kunit/string-stream-test.c:                actual_bytes_used += ksize(frag_container);
  lib/kunit/string-stream-test.c:                actual_bytes_used += ksize(frag_container->fragment);
  mm/nommu.c:                return ksize(objp);
  mm/util.c:                        memcpy(n, kasan_reset_tag(p), ksize(p));
  security/tomoyo/gc.c:        tomoyo_memory_used[TOMOYO_MEMORY_POLICY] -= ksize(ptr);
  security/tomoyo/memory.c:                const size_t s = ksize(ptr);
  drivers/md/dm-vdo/memory-alloc.c:                        add_kmalloc_block(ksize(p));
  drivers/md/dm-vdo/memory-alloc.c:                add_kmalloc_block(ksize(p));
  drivers/md/dm-vdo/memory-alloc.c:                        remove_kmalloc_block(ksize(ptr));
	
* One usage may need to be handled 
 
  sound/soc/codecs/cs-amp-lib-test.c:        KUNIT_ASSERT_GE_MSG(test, ksize(buf), priv->cal_blob->size, "Buffer to small");

* bigger problem is the kfree_sensitive(), which will use ksize() to
  get the total size and then zero all of them.
  
  One solution for this could be get the kmem_cache first, and
  do the skip_orig_size_check() 

Thanks,
Feng