linux-kernel - Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

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]

Date:   Mon, 3 Jul 2023 09:08:37 +0200
From:   Helge Deller <deller@....de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Guenter Roeck <linux@...ck-us.net>
Cc:     stable@...r.kernel.org, linux-kernel@...r.kernel.org,
        akpm@...ux-foundation.org,
        linux-parisc <linux-parisc@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        John David Anglin <dave.anglin@...l.net>
Subject: Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

Hi Linus,

On 7/3/23 08:20, Linus Torvalds wrote:
> On Sun, 2 Jul 2023 at 22:33, Guenter Roeck <linux@...ck-us.net> wrote:
>>
>> Here you are:
>>
>> [   31.188688] stack expand failed: ffeff000-fff00000 (ffefeff2)
>
> Ahhah!
>
> I think the problem is actually ridiculously simple.
>
> The thing is, the parisc stack expands upwards. That's obvious. I've
> mentioned it several times in just this thread as being the thing that
> makes parisc special.
>
> But it's *so* obvious that I didn't even think about what it really implies.
>
> And part of all the changes was this part in expand_downwards():
>
>          if (!(vma->vm_flags & VM_GROWSDOWN))
>                  return -EFAULT;
>
> and that will *always* fail on parisc, because - as said multiple
> times - the parisc stack expands upwards. It doesn't have VM_GROWSDOWN
> set.
>
> What a dum-dum I am.
>
> And I did it that way because the *normal* stack expansion obviously
> wants it that way and putting the check there not only made sense, but
> simplified other code.
>
> But fs/execve.c is special - and only special for parisc - in that it
> really wants to  expand a normally upwards-growing stack downwards
> unconditionally.
>
> Anyway, I think that new check in expand_downwards() is the right
> thing to do, and the real fix here is to simply make vm_flags reflect
> reality.
>
> Because during execve, that stack that will _eventually_ grow upwards,
> does in fact grow downwards.  Let's make it reflect that.
>
> We already do magical extra setup for the stack flags during setup
> (VM_STACK_INCOMPLETE_SETUP), so extending that logic to contain
> VM_GROWSDOWN seems sane and the right thing to do.
>
> IOW, I think a patch like the attached will fix the problem for real.
>
> It needs a good commit log and maybe a code comment or two, but before
> I bother to do that, let's verify that yes, it does actually fix
> things.
>
> In the meantime, I will actually go to bed, but I'm pretty sure this is it.

Great, that patch fixes it!

I wonder if you want to
#define VM_STACK_EARLY VM_GROWSDOWN
even for the case where the stack grows down too (instead of 0),
just to make clear that in both cases the stack goes downwards initially.

Helge