xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Anthony Perard" <anthony.perard@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH 05/16] x86/shstk: Introduce Supervisor Shadow Stack support
Date: Tue, 12 May 2020 15:54:58 +0200	[thread overview]
Message-ID: <dc585fec-e325-70a4-94e3-32205d84b1ea@suse.com> (raw)
In-Reply-To: <00302d53-499a-7f6e-76a5-a5eec4e11252@citrix.com>

On 11.05.2020 17:46, Andrew Cooper wrote:
> On 04/05/2020 14:52, Jan Beulich wrote:
>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>
>> On 02.05.2020 00:58, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/Kconfig
>>> +++ b/xen/arch/x86/Kconfig
>>> @@ -34,6 +34,9 @@ config ARCH_DEFCONFIG
>>>  config INDIRECT_THUNK
>>>  	def_bool $(cc-option,-mindirect-branch-register)
>>>  
>>> +config HAS_AS_CET
>>> +	def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64)
>> I see you add as-instr here as a side effect. Until the other
>> similar checks get converted, I think for the time being we should
>> use the old model, to have all such checks in one place. I realize
>> this means you can't have a Kconfig dependency then.
> 
> No.  That's like asking me to keep on using bool_t, and you are the
> first person to point out and object to that in newly submitted patches.

These are entirely different things. The bool_t => bool
conversion is agreed upon. The conversion to record tool chain
capabilities in xen/.config isn't. I've raised my reservations
against this elsewhere. I can be convinced, but not by trying to
introduce such functionality as a side effect of an unrelated
change.

>> Also why do you check multiple insns, when just one (like we do
>> elsewhere) would suffice?
> 
> Because CET-SS and CET-IBT are orthogonal ABIs, but you wanted a single
> CET symbol, rather than a CET_SS symbol.
> 
> I picked a sample of various instructions to get broad coverage of CET
> without including every instruction.

I wanted HAS_AS_CET rather than HAS_AS_CET_SS and HAS_AS_CET_IBT
because both got added to gas at the same time, and hence there's
little point in having separate symbols. If there was a reason to
assume assemblers might be out there which support one but not
the other, then we indeed ought to have two symbols.

>> The crucial insns to check are those which got changed pretty
>> close before the release of 2.29 (in the cover letter you
>> mention 2.32): One of incssp{d,q}, setssbsy, or saveprevssp.
>> There weren't official binutils releases with the original
>> insns, but distros may still have picked up intermediate
>> snapshots.
> 
> I've got zero interest in catering to distros which are still using
> obsolete pre-release toolchains.  Bleeding edge toolchains are one
> thing, but this is like asking us to support the middle changeset of the
> series introducing CET, which is absolutely a non-starter.
> 
> If the instructions were missing from real binutils releases, then
> obviously we should exclude those releases, but that doesn't appear to
> be the case.

But you realize that there's no special effort needed? We merely
need to check for the right insns. Their operands not matching
for binutils from the intermediate time window is enough for our
purposes. With my remark I merely meant to guide which of the
three insns you've picked needs to remain, and which would imo
better be dropped.

>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -95,6 +95,36 @@ unsigned long __initdata highmem_start;
>>>  size_param("highmem-start", highmem_start);
>>>  #endif
>>>  
>>> +static bool __initdata opt_xen_shstk = true;
>>> +
>>> +static int parse_xen(const char *s)
>>> +{
>>> +    const char *ss;
>>> +    int val, rc = 0;
>>> +
>>> +    do {
>>> +        ss = strchr(s, ',');
>>> +        if ( !ss )
>>> +            ss = strchr(s, '\0');
>>> +
>>> +        if ( (val = parse_boolean("shstk", s, ss)) >= 0 )
>>> +        {
>>> +#ifdef CONFIG_XEN_SHSTK
>>> +            opt_xen_shstk = val;
>>> +#else
>>> +            no_config_param("XEN_SHSTK", "xen", s, ss);
>>> +#endif
>>> +        }
>>> +        else
>>> +            rc = -EINVAL;
>>> +
>>> +        s = ss + 1;
>>> +    } while ( *ss );
>>> +
>>> +    return rc;
>>> +}
>>> +custom_param("xen", parse_xen);
>> What's the idea here going forward, i.e. why the new top level
>> "xen" option? Almost all options are for Xen itself, after all.
>> Did you perhaps mean this to be "cet"?
> 
> I forgot an RFC for this, as I couldn't think of anything better.  "cet"
> as a top level option isn't going to gain more than {no-}shstk and
> {no-}ibt as suboptions.

Imo that's still better than "xen=".

>>> --- a/xen/scripts/Kconfig.include
>>> +++ b/xen/scripts/Kconfig.include
>>> @@ -31,6 +31,10 @@ cc-option = $(success,$(CC) -Werror $(CLANG_FLAGS) $(1) -E -x c /dev/null -o /de
>>>  # Return y if the linker supports <flag>, n otherwise
>>>  ld-option = $(success,$(LD) -v $(1))
>>>  
>>> +# $(as-instr,<instr>)
>>> +# Return y if the assembler supports <instr>, n otherwise
>>> +as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler -o /dev/null -)
>> CLANG_FLAGS caught my eye here, then noticing that cc-option
>> also uses it. Anthony - what's the deal with this? It doesn't
>> look to get defined anywhere, and I also don't see what clang-
>> specific about these constructs.
> 
> This is as it inherits from Linux.  There is obviously a reason.
> 
> However, I'm not interested in diving into that rabbit hole in an
> unrelated series.

That's fine - my question was directed at Anthony after all.

Jan


  reply	other threads:[~2020-05-12 13:55 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-01 22:58 [PATCH 00/16] x86: Support for CET Supervisor Shadow Stacks Andrew Cooper
2020-05-01 22:58 ` [PATCH 01/16] x86/traps: Drop last_extable_addr Andrew Cooper
2020-05-04 12:44   ` Jan Beulich
2020-05-11 14:53     ` Andrew Cooper
2020-05-11 15:00       ` Jan Beulich
2020-05-01 22:58 ` [PATCH 02/16] x86/traps: Clean up printing in do_reserved_trap()/fatal_trap() Andrew Cooper
2020-05-04 13:08   ` Jan Beulich
2020-05-11 15:01     ` Andrew Cooper
2020-05-11 15:09       ` Jan Beulich
2020-05-18 16:54         ` Andrew Cooper
2020-05-19  8:50           ` Jan Beulich
2020-05-26 15:38             ` Andrew Cooper
2020-05-27  6:54               ` Jan Beulich
2020-05-01 22:58 ` [PATCH 03/16] x86/traps: Factor out exception_fixup() and make printing consistent Andrew Cooper
2020-05-04 13:20   ` Jan Beulich
2020-05-11 15:14     ` Andrew Cooper
2020-05-12 13:05       ` Jan Beulich
2020-05-26 18:06         ` Andrew Cooper
2020-05-27  7:01           ` Jan Beulich
2020-05-01 22:58 ` [PATCH 04/16] x86/smpboot: Write the top-of-stack block in cpu_smpboot_alloc() Andrew Cooper
2020-05-04 13:22   ` Jan Beulich
2020-05-01 22:58 ` [PATCH 05/16] x86/shstk: Introduce Supervisor Shadow Stack support Andrew Cooper
2020-05-04 13:52   ` Jan Beulich
2020-05-11 15:46     ` Andrew Cooper
2020-05-12 13:54       ` Jan Beulich [this message]
2020-05-15 16:21     ` Anthony PERARD
2020-05-01 22:58 ` [PATCH 06/16] x86/traps: Implement #CP handler and extend #PF for shadow stacks Andrew Cooper
2020-05-04 14:10   ` Jan Beulich
2020-05-11 17:20     ` Andrew Cooper
2020-05-12 13:58       ` Jan Beulich
2020-05-01 22:58 ` [PATCH 07/16] x86/shstk: Re-layout the stack block " Andrew Cooper
2020-05-04 14:24   ` Jan Beulich
2020-05-11 17:48     ` Andrew Cooper
2020-05-12 14:07       ` Jan Beulich
2020-05-01 22:58 ` [PATCH 08/16] x86/shstk: Create " Andrew Cooper
2020-05-04 14:55   ` Jan Beulich
2020-05-04 15:08     ` Andrew Cooper
2020-05-01 22:58 ` [PATCH 09/16] x86/cpu: Adjust enable_nmis() to be shadow stack compatible Andrew Cooper
2020-05-05 14:48   ` Jan Beulich
2020-05-11 18:48     ` Andrew Cooper
2020-05-01 22:58 ` [PATCH 10/16] x86/cpu: Adjust reset_stack_and_jump() " Andrew Cooper
2020-05-07 13:17   ` Jan Beulich
2020-05-11 20:07     ` Andrew Cooper
2020-05-01 22:58 ` [PATCH 11/16] x86/spec-ctrl: Adjust DO_OVERWRITE_RSB " Andrew Cooper
2020-05-07 13:22   ` Jan Beulich
2020-05-07 13:25     ` Andrew Cooper
2020-05-07 13:38       ` Jan Beulich
2020-05-01 22:58 ` [PATCH 12/16] x86/extable: Adjust extable handling " Andrew Cooper
2020-05-07 13:35   ` Jan Beulich
2020-05-11 21:09     ` Andrew Cooper
2020-05-12 14:31       ` Jan Beulich
2020-05-12 16:14         ` Andrew Cooper
2020-05-13  9:22           ` Jan Beulich
2020-05-01 22:58 ` [PATCH 13/16] x86/ioemul: Rewrite stub generation " Andrew Cooper
2020-05-07 13:46   ` Jan Beulich
2020-05-01 22:58 ` [PATCH 14/16] x86/alt: Adjust _alternative_instructions() to not create shadow stacks Andrew Cooper
2020-05-07 13:49   ` Jan Beulich
2020-05-01 22:58 ` [PATCH 15/16] x86/entry: Adjust guest paths to be shadow stack compatible Andrew Cooper
2020-05-07 14:12   ` Jan Beulich
2020-05-07 15:50     ` Andrew Cooper
2020-05-07 16:15       ` Jan Beulich
2020-05-11 21:45         ` Andrew Cooper
2020-05-12 14:56           ` Jan Beulich
2020-05-01 22:58 ` [PATCH 16/16] x86/shstk: Activate Supervisor Shadow Stacks Andrew Cooper
2020-05-07 14:54   ` Jan Beulich
2020-05-11 23:46     ` Andrew Cooper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=dc585fec-e325-70a4-94e3-32205d84b1ea@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=anthony.perard@citrix.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).