Re: [PATCH -next] mm: delete oversized WARN_ON() in kvmalloc() calls

[Bixuan Cui <cuibixuan@xxxxxxxxxxxxxxxxx>]

在 2021/12/2 上午11:46, Kees Cook 写道:

On Thu, Dec 02, 2021 at 10:06:24AM +0800, Bixuan Cui wrote:

Delete the WARN_ON() and return NULL directly for oversized parameter
in kvmalloc() calls.
Also add unlikely().

Fixes: 7661809d493b ("mm: don't allow oversized kvmalloc() calls")
Signed-off-by: Bixuan Cui <cuibixuan@xxxxxxxxxxxxxxxxx>
---
There are a lot of oversize warnings and patches about kvmalloc() calls
recently. Maybe these warnings are not very necessary.

It seems these warnings are working, yes? i.e. we're finding the places
where giant values are coming in?

    Yes,  It's working.

https://lore.kernel.org/all/YadOjJXMTjP85MQx@unreal

The example of size check in __do_kmalloc_node():
__do_kmalloc_node(size_t size, gfp_t flags, int node, unsigned long caller)
{
        struct kmem_cache *cachep;
        void *ret;

        if (unlikely(size > KMALLOC_MAX_CACHE_SIZE))
                return NULL;
        cachep = kmalloc_slab(size, flags);

 mm/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/util.c b/mm/util.c
index 7e433690..d26f19c 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -587,7 +587,7 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
 		return ret;
 
 	/* Don't even allow crazy sizes */
-	if (WARN_ON_ONCE(size > INT_MAX))
+	if (unlikely(size > INT_MAX))
 		return NULL;

If we're rejecting the value, then it's still a pathological size, so
shouldn't the check be happening in the caller? I think the WARN is
doing exactly what it was supposed to do: find the places where bad
sizes can reach vmalloc.

    In this way, we must check whether the size from
      the user exceeds INT_MAX
    before calling kvmalloc() calls.  Generally
      speaking, the oversize check is rarely
    done before.
    
    Thanks,
    Bixuan Cui

-Kees

 
 	return __vmalloc_node(size, 1, flags, node,
-- 
1.8.3.1

-- 
Kees Cook