On Wed, Jul 21, 2021 at 11:42 AM Matthew Wilcox (Oracle)
<[email protected]> wrote:
>
> It's generally dangerous to allocate such large quantities of memory
> within the kernel owing to our propensity to use 'int' to represent
> a length. If somebody really needs it, we can add a kvmalloc_large()
> later, but let's default to "You can't allocate that much memory".
I really think that without the WARN_ON_ONCE(), this is just moving
that failure point from a known good place ("we know this must not
succeed") to a possibly bad place ("this might cause silent and
hard-to-understand failures elsewhere").
IOW, in seq_buf_alloc() there's no need to warn. It's clear that a
bigger allocation can never be valid.
But in kvmalloc(), it needs to warn, because if it ever triggers we
need to check what triggered it.
So this is not just moving code from one place to another equivalent one.
Linus
On Wed, Jul 21, 2021 at 01:46:09PM -0700, Linus Torvalds wrote:
> On Wed, Jul 21, 2021 at 11:42 AM Matthew Wilcox (Oracle)
> <[email protected]> wrote:
> >
> > It's generally dangerous to allocate such large quantities of memory
> > within the kernel owing to our propensity to use 'int' to represent
> > a length. If somebody really needs it, we can add a kvmalloc_large()
> > later, but let's default to "You can't allocate that much memory".
>
> I really think that without the WARN_ON_ONCE(), this is just moving
> that failure point from a known good place ("we know this must not
> succeed") to a possibly bad place ("this might cause silent and
> hard-to-understand failures elsewhere").
To a certain extent, yes. On the other hand, if you don't have any
error handling on your kvmalloc of 2GB, Qualys seems to have a reliable
way to run you out of vmalloc space, and that's going to get exercised.
My initial thought was to leverage the existing __GFP_NOWARN code:
if (size > PAGE_SIZE) {
- kmalloc_flags |= __GFP_NOWARN;
+ if (size <= INT_MAX)
+ kmalloc_flags |= __GFP_NOWARN;
because that dumps some interesting information (ratelimited), which
might help the sysadmin realise they're under attack. A WARN_ON_ONCE
is one-and-done, so an attacker can hide their tracks. Unfortunately,
we actually bail out before getting there:
if (unlikely(order >= MAX_ORDER)) {
WARN_ON_ONCE(!(gfp & __GFP_NOWARN));
return NULL;
}
... maybe that should call warn_alloc() too.
So I'm now thinking (relative to the earlier patch):
- if (size > INT_MAX)
+ if (size > INT_MAX) {
+ warn_alloc(flags, NULL, "oversized allocation:%zu", size);
return NULL;
+ }