On Tue, May 31, 2022 at 05:34:50PM +0000, Edgecombe, Rick P wrote:

On Tue, 2022-05-31 at 19:36 +0300, Mike Rapoport wrote:

quoted

quoted

WRSS is a feature where you would usually want to lock it as
disabled,
but WRSS cannot be enabled if shadow stack is not enabled. Locking
shadow stack and WRSS off together doesn't have any security
benefits
in theory. so I'm thinking glibc doesn't need to do this. The
kernel
could even refuse to lock WRSS without shadow stack being enabled.
Could we avoid the extra ptrace functionality then?

What I see for is that a program can support shadow stack, glibc
enables
shadow stack, does not enable WRSS and than calls

        arch_prctl(ARCH_X86_FEATURE_LOCK,
                   LINUX_X86_FEATURE_SHSTK | LINUX_X86_FEATURE_WRSS);

I see the logic is glibc will lock SHSTK|IBT if either is enabled in
the elf header. I guess that is why I didn't see the locking happening
for me, because my manual enablement test doesn't have either set in
the header.

The locking was quite a surprise for me when I moved from standalone test
to a system with CET-enabled glibc :)
 

It can't see where that glibc knows about WRSS though...

Right, it was my mistake, as H.J. said glibc locks SHSTK and IBT.
 

The glibc logic seems wrong to me also, because shadow stack or IBT
could be force-disabled via glibc tunables. I don't see why the elf
header bit should exclusively control the feature locking. Or why both
should be locked if only one is in the header.

quoted

so that WRSS cannot be re-enabled.

For the programs that do not support shadow stack, both SHSTK and
WRSS are
disabled, but still there is the same call to
arch_prctl(ARCH_X86_FEATURE_LOCK, ...) and then neither shadow stack
nor
WRSS can be enabled.

My original plan was to run CRIU with no shadow stack, enable shadow
stack
and WRSS in the restored tasks using arch_prct() and after the shadow
stack
contents is restored disable WRSS.

Obviously, this didn't work with glibc I have :)

Were you disabling shadow stack via glibc tunnable? Or was the elf
header marked for IBT? If it was a plain old binary, the code looks to
me like it should not lock any features.

I built criu as a plain old binary, there were no SHSTK or IBT markers. And
I've seen that there was a call to arch_prctl that locked the features as
disabled. 
 

quoted

On the bright side, having a ptrace call to unlock shadow stack and
wrss
allows running CRIU itself with shadow stack.

Yea, having something working is really great. My only hesitancy is
that, per a discussion on the LAM patchset, we are going to make this
enabling API CET only (same semantics for though). I suppose the
locking API arch_prctl() could still be support other arch features,
but it might be a second CET only regset. It's not the end of the
world.

The support for CET in criu is anyway experimental for now, if the kernel
API will be slightly different in the end, we'll update criu.
The important things are the ability to control tracee shadow stack
from ptrace, the ability to map the shadow stack at fixed address and the
ability to control the features at least from ptrace.
As long as we have APIs that provide those, it should be Ok.
 

I guess the other consideration is tieing CRIU to glibc peculiarities.
Like even if we fix glibc, then CRIU may not work with some other libc
or app that force disables for some weird reason. Is it supposed to be
libc-agnostic?

Actually using the ptrace to control the CET features does not tie criu to
glibc. The current proposal for the arch_prctl() allows libc to lock CET
features and having a ptrace call to control the lock makes criu agnostic
to libc behaviour.

-- 
Sincerely yours,
Mike.