Re: [PATCH v9 16/42] mm: Add guard pages around a shadow stack.

[Edgecombe, Rick P]

On Fri, 2023-06-23 at 13:17 +0100, Mark Brown wrote:
> On Fri, Jun 23, 2023 at 10:40:00AM +0300, Mike Rapoport wrote:
> > On Thu, Jun 22, 2023 at 06:27:40PM +0000, Edgecombe, Rick P wrote:
> 
> > > Yes, I couldn't find another place for it. This was the
> > > reasoning:
> > > 
> https://lore.kernel.org/lkml/07deaffc10b1b68721bbbce370e145d8fec2a494.camel@xxxxxxxxx/
> 
> > > Did you have any particular place in mind?
> 
> > Since it's near CONFIG_X86_USER_SHADOW_STACK the comment in mm.h
> could be 
> 
> > /*
> >   * VMA is used for shadow stack and implies guard pages.
> >   * See arch/x86/kernel/shstk.c for details
> >   */
> 
> > and the long reasoning comment can be moved near alloc_shstk in
> > arch/x86/kernel/shstk.h

Makes sense. Not sure why I didn't think of this earlier.

> 
> This isn't an x86 specific concept, arm64 has a very similar
> extension
> called Guarded Control Stack (which I should be publishing changes
> for
> in the not too distant future) and riscv also has something.  For
> arm64
> I'm using the generic mm changes wholesale, we have a similar need
> for
> guard pages around the GCS and while the mechanics of accessing are
> different the requirement ends up being the same.  Perhaps we could
> just
> rewrite the comment to say that guard pages prevent over/underflow of
> the stack by userspace and that a single page is sufficient for all
> current architectures, with the details of the working for x86 put in
> some x86 specific place?

Something sort of similar came up in regards to the riscv series, about
adding something like an is_shadow_stack_vma() helper. The plan was to
not make too many assumptions about the final details of the other
shadow stack features and leave that for refactoring. I think some kind
of generic comment like you suggest makes sense, but I don't want to
try to assert any arch specifics for features that are not upstream. It
should be very easy to tweak the comment when the time comes.

The points about x86 details not belonging in non-arch headers and
having some arch generic explanation in the file are well taken though.