Re: [PATCH v3 2/7] prctl: introduce PR_DEFAULT_MADV_HUGEPAGE for the process

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, May 20, 2025 at 01:01:38AM +0200, Jann Horn wrote:
> On Tue, May 20, 2025 at 12:33 AM Usama Arif <usamaarif642@gmail.com> wrote:
> > This is set via the new PR_SET_THP_POLICY prctl. It has 2 affects:
> > - It sets VM_HUGEPAGE and clears VM_NOHUGEPAGE on the default VMA flags
> >   (def_flags). This means that every new VMA will be considered for
> >   hugepage.
> > - Iterate through every VMA in the process and call hugepage_madvise
> >   on it, with MADV_HUGEPAGE policy.
> > The policy is inherited during fork+exec.
>
> As I replied to Lorenzo's series
> (https://lore.kernel.org/all/CAG48ez3-7EnBVEjpdoW7z5K0hX41nLQN5Wb65Vg-1p8DdXRnjg@mail.gmail.com/),
> it would be nice if you could avoid introducing new flags that have
> the combination of all the following properties:
>
> 1. persists across exec
> 2. not cleared on secureexec execution
> 3. settable without ns_capable(CAP_SYS_ADMIN)
> 4. settable without NO_NEW_PRIVS
>
> Flags that have all of these properties need to be reviewed extra
> carefully to see if there is any way they could impact the security of
> setuid binaries, for example by changing mmap() behavior in a way that
> makes addresses significantly more predictable.

Indeed, this series was meant to be as RFC as mine while we still figured this
out :) grr. Well, with the NACK it is - in effect - now an RFC.

Yes having something persistent like this is not great, the idea of
introducing this in my series was to provide an alternative generic version
of this approach that can be better controlled and isn't just a 'tacked on'
change specific to one company's needs but rather a more general idea of
'madvise() by default'.

I do wonder in this case, whether we need be so cautious however given the
_relatively_ safe nature of these flags?

I do absolutely agree we need to very carefully review whether:

1. It really even makes sense to do this
2. Any such restrictions need be made

I am weaker on the security side so very glad for your input here (thanks!)

I suspect probably we want ns_capable(CAP_SYS_ADMIN) _as a rule_ for this
kind of mm->def_flags change.

I also wanted to dig a little deeper into whether this was sensible as a
general approach.

I, however, do _very much_ prefer it to an mm->flags change (that'd
necessity a pre-requisite 'make mm->flags 64-bit on 32-bit kernels'
series anyway).