Random arrays on kernel stack..

Random arrays on kernel stack..

[Linus Torvalds]

So I finally have an arm64 laptop that I'm playing with, and as a
result building the kernel the way I usually do - with warnings as
errors.

And I get this:

  drivers/net/wireguard/allowedips.c: In function ‘root_remove_peer_lists’:
  drivers/net/wireguard/allowedips.c:77:1: error: the frame size of
1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
     77 | }
        | ^
  drivers/net/wireguard/allowedips.c: In function ‘root_free_rcu’:
  drivers/net/wireguard/allowedips.c:64:1: error: the frame size of
1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
     64 | }
        | ^

and clearly it only happens for me because it turns out Asahi has for
some odd reason picked that low CONFIG_FRAME_WARN of just 1kB.

So the fix for the warning was just to update my .config file, no biggie.

But when I look at the code that generates that warning, it just
worries me. It has that magical constant 128.

Sure, that same constant is also in push_rcu().

And there it is randomly as a warning, and then it will happily
overflow the stack frame.

That's not ok.

I think that

 (a) that constant should be a bit lower, so that we *can* use a 1kB
stack frame warning on 64-bit architectures

 (b) it should be documented some way as a #define

 (c) push_rcu() should damn well not "warn and corrupt the stack". It
should warn-and-not-corrupt the stack, even if that then means that
the thing isn't pushed at all.

Hmm?

                 Linus

Re: Random arrays on kernel stack..

[Jason A. Donenfeld]

Hey Linus,

On Thu, Jul 28, 2022 at 04:51:58PM -0700, Linus Torvalds wrote:
> So I finally have an arm64 laptop that I'm playing with, and as a

Some intrepid diving into Asahi world, eh? I admit that after playing
with the M1 and benching/optimizing some code on it, I wasn't man enough
to daily drive the thing, and I wound up selling the (already
second hand) Mac Mini shortly after acquiring it.

> result building the kernel the way I usually do - with warnings as
> errors.
> 
> And I get this:
> 
>   drivers/net/wireguard/allowedips.c: In function ‘root_remove_peer_lists’:
>   drivers/net/wireguard/allowedips.c:77:1: error: the frame size of
> 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>      77 | }
>         | ^
>   drivers/net/wireguard/allowedips.c: In function ‘root_free_rcu’:
>   drivers/net/wireguard/allowedips.c:64:1: error: the frame size of
> 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>      64 | }
>         | ^
> 
> and clearly it only happens for me because it turns out Asahi has for
> some odd reason picked that low CONFIG_FRAME_WARN of just 1kB.

Arnd (now CC'd) and I had a discussion about this a few years ago, and
IIRC the conclusion was that the kernel targets 1280 bytes on 64-bit
platforms and 1024 bytes on 32-bit platforms. There's been gradual work
done over the years to get gcc to generate code that matched this
expectation.

So root_remove_peer_lists and root_free_rcu are really nestling up
against the boundary. We'd looked into ways of avoid that, but decided
in the end it was fine, and probably the most okay way to implement
those algorithms, all things considered.

> But when I look at the code that generates that warning, it just
> worries me. It has that magical constant 128.
> 
> Sure, that same constant is also in push_rcu().

I could (should?) put it in a constant. But as context, note that it
doesn't look _so_ magical for anyone hacking on the code. It's a trie
for IPv4 and IPv6 addresses. If you're writing networking code, the
128-bitness of IPv6 addresses is nearly always at the top of your brain,
its general inconvenience an elephant not possible to ignore.

I guess `sizeof(struct in6_addr) * 8` would be a more accurate
reference.

>  (b) it should be documented some way as a #define

Alright, will do.

> And there it is randomly as a warning, and then it will happily
> overflow the stack frame.
> 
> That's not ok.
>
>  (c) push_rcu() should damn well not "warn and corrupt the stack". It
> should warn-and-not-corrupt the stack, even if that then means that
> the thing isn't pushed at all.

That's a good point. Though note that the current WARN_ON is also
dependent on DEBUG being set. The purpose is to catch errors when the
selftest in drivers/net/wireguard/selftest/allowedips.c runs. That file
has this snippet:

  /* These will hit the WARN_ON(len >= 128) in free_node if something
   * goes wrong.
   */
  for (i = 0; i < 128; ++i) {
    part = cpu_to_be64(~(1LLU << (i % 64)));
    memset(&ip, 0xff, 16);
    memcpy((u8 *)&ip + (i < 64) * 8, &part, 8);
    wg_allowedips_insert_v6(&t, &ip, 128, a, &mutex);
  }
  wg_allowedips_free(&t, &mutex);

Anyway, I'll conditionalize it so the stack isn't corrupted.

>  (a) that constant should be a bit lower, so that we *can* use a 1kB
> stack frame warning on 64-bit architectures

That's not so much possible. This needs to do a DFS traversal of the
trie, which means it can be 128 nodes deep since IPv6 addresses have
128 bits. I could just malloc, instead, but malloc'ing in order to free
something is kind of annoying. And it *does* fit after all. I could
suppress that option for just that file, but that might hide other bugs.
And there's the 1280 target I mentioned earlier, so maybe this is all
fine? Unless you have a different suggestion?

I think, anyhow, this is roughly where Arnd and I left off a few years
ago.

Jason

Re: Random arrays on kernel stack..

[Linus Torvalds]

On Fri, Jul 29, 2022 at 10:12 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> That's a good point. Though note that the current WARN_ON is also
> dependent on DEBUG being set.

Sure. And I think that's ok.

> >  (a) that constant should be a bit lower, so that we *can* use a 1kB
> > stack frame warning on 64-bit architectures
>
> That's not so much possible. This needs to do a DFS traversal of the
> trie, which means it can be 128 nodes deep since IPv6 addresses have
> 128 bits.

Ahh. Ok. That at least explains where the constant comes from. That
would be a good thing to have somewhere in that definition of the
value ;)

And I agree that malloc() isn't a great choice.

I don't really worry about the stack frame warning (I just raised it
to the same 2k limit I have on my x86-64 box), so doing that 1k
allocation is fine.

And with that constant 128 explained, I don't think it's wrong to not
even test for overflow. We don't test for things that can't happen.

But *if* you test for it for debug purposes, then at that point I
think you need to just do the "warn and don't corrupt stack".

                     Linus