[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible


  • To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 14 Feb 2022 17:16:17 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=OvZ1Hsbafmn8mdBKKTMNfbLt7J6kHZgqQ5YFa+tjGqs=; b=QpwhI+xPmzZn9rGkKBbmKeSsS8qSvSFOTmZUJc3wzB9Bc7+76NcccTr9pZwx+iouCcm20AMV3aeHiNTcNZ3K6X6G4ue8XvNfEJ32JhHCNXxhJRvQT7DpVG2iutO4mj1gSLwtyrwX5osfPEt2hjQHaNN7xUjZNNuIll1vpeGJMEXubI1hpUZ+NHLmikteyUsnWgu5se8oEv0f/ImA55AX78aIPA1MiGqknjDP004Qkkqzv9TcNfhRObh0ZXUt9rSEpVSJzc3w+LPBXB5La7JqKNRlgC17gpzc60P5ofDVTj6X3y5fnnhXp3kfUCGVYwtL3eJOMV7XkO3z4/RV3IsSEA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=huvZy9v7JSIxPNnaq1b01CGzZjaiMxu8G1+B+LkrsCdW0rA6wqJBokrNLNz8oIkhbed8ereJimIs+htPyMjW2ija1PEvgFhOrziJ+ladwee9GPQaY05eB3LGJBqCAtEkgfO6hvbE+Q7d38cqwUOW23bG5ww2mrgijZsU0QlGVne7gBaRBZuh5bYh/lwEWBBaMOG0qpk6+N8qI4w9a49ONcXa7QoFwKGywIu5rDgyWv3/zAQACwJiIFDyv5pD90M187YP8xY/s5QiAOkMkCe7O/aH7mcU8Kmy8iDWQuolbHLJgBOA2GQ6MRH9RPf7O7xRrSu9HGJWWDzxBjPAxZrgWA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 14 Feb 2022 16:16:26 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 14.02.2022 17:03, Andrew Cooper wrote:
> On 14/02/2022 13:51, Jan Beulich wrote:
>> On 14.02.2022 14:31, Andrew Cooper wrote:
>>> On 14/02/2022 13:06, Jan Beulich wrote:
>>>> On 14.02.2022 13:56, Andrew Cooper wrote:
>>>>> @@ -330,6 +333,41 @@ static void init_or_livepatch 
>>>>> _apply_alternatives(struct alt_instr *start,
>>>>>          add_nops(buf + a->repl_len, total_len - a->repl_len);
>>>>>          text_poke(orig, buf, total_len);
>>>>>      }
>>>>> +
>>>>> +    /*
>>>>> +     * Clobber endbr64 instructions now that altcall has finished 
>>>>> optimising
>>>>> +     * all indirect branches to direct ones.
>>>>> +     */
>>>>> +    if ( force && cpu_has_xen_ibt )
>>>>> +    {
>>>>> +        void *const *val;
>>>>> +        unsigned int clobbered = 0;
>>>>> +
>>>>> +        /*
>>>>> +         * This is some minor structure (ab)use.  We walk the entire 
>>>>> contents
>>>>> +         * of .init.{ro,}data.cf_clobber as if it were an array of 
>>>>> pointers.
>>>>> +         *
>>>>> +         * If the pointer points into .text, and at an endbr64 
>>>>> instruction,
>>>>> +         * nop out the endbr64.  This causes the pointer to no longer be 
>>>>> a
>>>>> +         * legal indirect branch target under CET-IBT.  This is a
>>>>> +         * defence-in-depth measure, to reduce the options available to 
>>>>> an
>>>>> +         * adversary who has managed to hijack a function pointer.
>>>>> +         */
>>>>> +        for ( val = __initdata_cf_clobber_start;
>>>>> +              val < __initdata_cf_clobber_end;
>>>>> +              val++ )
>>>>> +        {
>>>>> +            void *ptr = *val;
>>>>> +
>>>>> +            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
>>>>> +                continue;
>>>>> +
>>>>> +            add_nops(ptr, 4);
>>>> This literal 4 would be nice to have a #define next to where the ENDBR64
>>>> encoding has its central place.
>>> We don't have an encoding of ENDBR64 in a central place.
>>>
>>> The best you can probably have is
>>>
>>> #define ENDBR64_LEN 4
>>>
>>> in endbr.h ?
>> Perhaps. That's not in this series nor in staging already, so it's a little
>> hard to check. By "central place" I really meant is_enbr64() if that's the
>> only place where the encoding actually appears.
> 
> endbr.h is the header which contains is_endbr64(), and deliberately does
> not contain the raw encoding.

Well, yes, it's intentionally the inverted encoding, but I thought
you would get the point.

>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>> @@ -221,6 +221,12 @@ SECTIONS
>>>>>         *(.initcall1.init)
>>>>>         __initcall_end = .;
>>>>>  
>>>>> +       . = ALIGN(POINTER_ALIGN);
>>>>> +       __initdata_cf_clobber_start = .;
>>>>> +       *(.init.data.cf_clobber)
>>>>> +       *(.init.rodata.cf_clobber)
>>>>> +       __initdata_cf_clobber_end = .;
>>>>> +
>>>>>         *(.init.data)
>>>>>         *(.init.data.rel)
>>>>>         *(.init.data.rel.*)
>>>> With r/o data ahead and r/w data following, may I suggest to flip the
>>>> order of the two section specifiers you add?
>>> I don't follow.  This is all initdata which is merged together into a
>>> single section.
>>>
>>> The only reason const data is split out in the first place is to appease
>>> the toolchains, not because it makes a difference.
>> It's marginal, I agree, but it would still seem more clean to me if all
>> (pseudo) r/o init data lived side by side.
> 
> I still don't understand what you're asking.
> 
> There is no such thing as actually read-only init data.
> 
> Wherever the .init.rodata goes in here, it's bounded by .init.data.

Well, looking at the linker script again I notice that while r/o items
like .init.setup and .initcall*.init come first, some further ones
(.init_array etc) come quite late. Personally I'd prefer if all r/o
items sat side by side, no matter that currently we munge them all
into a single section. Then, if we decided to stop this practice, all
it would take would be to insert an output section closing and re-
opening. (Or it would have been so until now; with your addition it
wouldn't be as simple anymore anyway.)

But anyway, if at this point I still didn't get my point across, then
please leave things as you have them.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.