Re: [PATCH v2 06/14] x86/shstk: Create shadow stacks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.05.2020 23:45, Andrew Cooper wrote:
> On 29/05/2020 20:35, Andrew Cooper wrote:
>>>> +    }
>>>> +    map_pages_to_xen((unsigned long)p, virt_to_mfn(p), 1, 
>>>> PAGE_HYPERVISOR_SHSTK);
>>> As already hinted at in reply to the previous patch, I think this wants
>>> to remain _PAGE_NONE when we don't use CET-SS.
>> The commit message discussed why that is not an option (currently), and
>> why I don't consider it a good idea to make possible.
> 
> Apologies.  I thought I'd written it in the commit message, but it was
> half in the previous patch, and not terribly clear.  I've reworked both.

Thanks, I've looked at them, but it's still not really clear to me:

> The current practical reason is to do with clone_mappings() in the XPTI
> case.

What exactly is the problem here? clone_mapping(), afaict, deals
fine with non-present PTEs. The original memguard_is_stack_guard_page()
check was more as a safe guard, to avoid establishing a mapping of
a guard page as much as possible.

> A wild off-stack read is far far less likely than hitting the guard page
> with a push first, which means that a R/O guard page is about the same
> usefulness to us, but results in a much more simple stack setup, as it
> doesn't vary attributes with enabled features.

While OoB stack reads may indeed be less likely, such aren't necessarily
"wild" (assuming my understanding of the term is what you mean): A
function epilogue can certainly pop (by the respective insn or by
incrementing %rsp by too much) too many slots, which would be detected
earlier with non-present mappings than with r/o ones. So I'd prefer to
stick to non-present guard pages when CET-SS is not in use, unless
there's indeed a technical reason not to do so. The two uses of
PAGE_HYPERVISOR_SHSTK can't be that bad a "variation" to alternatively
make _PAGE_NONE. In fact PAGE_HYPERVISOR_SHSTK could itself resolve
to _PAGE_NONE when !cpu_has_xen_shstk ...

Jan