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:   Fri, 14 Apr 2023 14:07:59 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc:     "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH 3/3] mm/mmap: Regression fix for unmapped_area{_topdown}

* Edgecombe, Rick P <rick.p.edgecombe@...el.com> [230414 13:53]:
> On Fri, 2023-04-14 at 13:29 -0400, Liam R. Howlett wrote:
> > * Liam R. Howlett <Liam.Howlett@...cle.com> [230414 13:26]:
> > > * Edgecombe, Rick P <rick.p.edgecombe@...el.com> [230414 12:27]:
> > > > On Fri, 2023-04-14 at 10:57 -0400, Liam R. Howlett wrote:<br>
> > > > > +       tmp = mas_next(&mas, ULONG_MAX);
> > > > > +       if (tmp && (tmp->vm_flags & VM_GROWSDOWN)) {
> > > > 
> > > > Why also check VM_GROWSDOWN here (and VM_GROWSUP below)?
> > > > vm_start/end_gap() already have checks inside.
> > > 
> > > An artifact of a plan that was later abandoned.
> > > 
> > > > 
> > > > > +               if (vm_start_gap(tmp) < gap + length - 1) {
> > > > > +                       low_limit = tmp->vm_end;
> > > > > +                       mas_reset(&mas);
> > > > > +                       goto retry;
> > > > > +               }
> > > > > +       } else {
> > > > > +               tmp = mas_prev(&mas, 0);
> > > > > +               if (tmp && (tmp->vm_flags & VM_GROWSUP) &&
> > > > > +                   vm_end_gap(tmp) > gap) {
> > > > > +                       low_limit = vm_end_gap(tmp); 
> > > > > +                       mas_reset(&mas);
> > > > > +                       goto retry; 
> > > > > +               }
> > > > > +       } 
> > > > > +
> > > > 
> > > > Could it be like this?
> > > 
> > > Yes, I'll make this change.  Thanks for the suggestion.
> > 
> > 
> > Wait, I like how it is.
> > 
> > In my version, if there is a stack that is VM_GROWSDOWN there, but
> > does
> > not intercept the gap, then I won't check the prev.. in yours, we
> > will
> > never avoid checking prev.
> 
> Hmm, I see. I guess I'm thinking ahead a bit to adding the shadow stack
> guard gap, but I can always add to these vm_flags checks.
> 
> But are you sure this optimization is even possible? The old
> vma_compute_gap() had this comment:
> /*
>  * Note: in the rare case of a VM_GROWSDOWN above a VM_GROWSUP, we
>  * allow two stack_guard_gaps between them here, and when choosing
>  * an unmapped area; whereas when expanding we only require one.
>  * That's a little inconsistent, but keeps the code here simpler.
>  */

I didn't think this was possible.  ia64 (orphaned in 96ec72a3425d) did
this.

> 
> Assuming this is a real scenario, if you have VM_GROWSDOWN above and
> VM_GROWSUP below, don't you need to check the gaps for above and below?
> Again thinking about adding shadow stack guard pages, something like
> that could be a more common scenario. Not that you need to fix my out
> of tree issues, but I would probably need to adjust it to check both
> directions.
> 
> I guess there is no way to embed this inside maple tree search so we
> don't need to retry? (sorry if this is a dumb question, it's an opaque
> box to me).

Absolutely, and I'm working on this as well, but right now I'm trying
to fix my regression for this and past releases.  Handling this in the
maple tree is more involved and so there's more risk.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ