Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: Boot regression with bacf6b499e11 ("x86/mm: Use a struct to
 reduce parameters for SME PGD mapping") on top of -rc8
To:     Laura Abbott <labbott@redhat.com>, Ingo Molnar <mingo@kernel.org>
Cc:     Gabriel C <nix.or.die@gmail.com>, Borislav Petkov <bp@suse.de>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Brijesh Singh <brijesh.singh@amd.com>, X86 ML <x86@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <9fdddcb1-d122-7d52-9204-7066ada5ccba@redhat.com>
 <CAEJqkggBANYY98EFxWFDtuh1q3Ov7=UGnn2GYo_T661=LnVNPA@mail.gmail.com>
 <44505ab1-237b-88ea-1fb1-f80de9b3025a@redhat.com>
 <CAEJqkgjwD+Ucj+J7taBuQm+qMbOXCccgHBsfnutEa=jcPBYAbw@mail.gmail.com>
 <6277b77a-d4a0-5669-e5aa-7a850436227c@amd.com>
 <6b616ebc-1a8f-0bd7-68be-3674ff99500b@redhat.com>
 <20180120123338.buixvrhlxqo3ev5p@gmail.com>
 <20180120131308.bfm5cxxigw2xzgw2@gmail.com>
 <d2b236d9-ccfc-0cd0-f097-9daba70b86ff@redhat.com>
From:   Tom Lendacky <thomas.lendacky@amd.com>
Message-ID: <b7a8e990-9d1d-39b2-671d-a44d5647dbec@amd.com>
Date:   Sat, 20 Jan 2018 11:34:10 -0600
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <d2b236d9-ccfc-0cd0-f097-9daba70b86ff@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Received-SPF: None (protection.outlook.com: amd.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jan 2018 17:34:15.0119 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 96ae44c7-bd15-4775-f363-08d5602c0770
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR12MB1143
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 1/20/2018 10:52 AM, Laura Abbott wrote:
> On 01/20/2018 05:13 AM, Ingo Molnar wrote:
>>
>> * Ingo Molnar <mingo@kernel.org> wrote:
>>
>>> 2)
>>>
>>> using global variables, which is unsafe in early code if the kernel is
>>> relocatable.
>>>
>>> The bisected to commit uses a new sme_populate_pgd_data to collect
>>> variables that
>>> were already on the stack, which should be position independent and safe.
>>>
>>> But the other commits use sme_active(), which does:
>>>
>>> bool sme_active(void)
>>> {
>>>          return sme_me_mask && !sev_enabled;
>>> }
>>> EXPORT_SYMBOL(sme_active);
>>>
>>> And that looks PIC-unsafe to me, as both are globals:
>>>
>>> u64 sme_me_mask __section(.data) = 0;
>>> EXPORT_SYMBOL(sme_me_mask);
>>>
>>> Does the code start working if you force sme_active() to 0 while
>>> keeping the
>>> function call, i.e. something like the hack below?
>>
>> BTW., this aspect of the boot code is really fragile, and depending on
>> compiler
>> there could be unsafe relocations generated without it being 'obvious'
>> from the
>> patch itself. It's also pretty compiler and code layout dependent ...
>>
>> A good way to check this I think would be to turn off
>> CONFIG_RELOCATABLE=y in the
>> .config - does that make the kernel boot again?
>>
>> If that makes a difference then we need to take a look at the
>> relocations in the
>> two key files, with CONFIG_RELOCATABLE=y turned back on:
>>
>>    objdump -r arch/x86/kernel/head64.o
>>    objdump -r arch/x86/mm/mem_encrypt.o
>>
>> There's three types of relocations that should be there normally:
>>
>> #define R_X86_64_64             1       /* Direct 64 bit  */
>> #define R_X86_64_PC32           2       /* PC relative 32 bit signed */
>> #define R_X86_64_32S            11      /* Direct 32 bit sign extended */
>>
>> Only R_X86_64_PC32 is safe as-is, R_X86_64_32S needs to be used via
>> fixup_pointer().
>>
>> What makes this difficult in the SME context is that the early boot
>> portion of
>> arch/x86/mm/mem_encrypt.c is not separated out, but mixed in with later
>> code.
>>
>> I missed this aspect when reviewing and merging this code :-(
>>
>> Maybe a diff of the list of relocations of the before/after commit
>> points would be
>> nice.
>>
>> I.e. does something like:
>>
>>    git checkout <last_working_commit_sha1>
>>    objdump -r arch/x86/mm/mem_encrypt.o  | grep R_X86 | cut -d' ' -f2- >
>> working.relocs
>>
>>    git checkout <first_broken_commit_sha1>
>>    objdump -r arch/x86/mm/mem_encrypt.o  | grep R_X86 | cut -d' ' -f2- >
>> broken.relocs
>>
>>    diff -up working.relocs broken.relocs
>>
>> show any changes to the relocations?
>>
>> Side note:
>>
>> Regardless of whether it's the root cause for this regression we
>> definitely need
>> to improve the relocations robustness of early boot code: at minimum we
>> should
>> isolate all critical functionality into a separate section, and then add
>> tooling
>> checks to make sure all relocations are safe.
>>
>> Thanks,
>>
>>     Ingo
>>
> 
> For the previous question, changing it to sme_active _does_ make the
> kernel work. Unfortunately, I can't test without relocations since
> I need to boot with CONFIG_EFI_STUB, but the relocations did show
> something interesting:
> 
> +R_X86_64_PC32     __stack_chk_fail-0x0000000000000004
> 
> There's a new call to __stack_chk_fail and if I dump the end of
> sme_encrypt_kernel I do see that stuck in there. I bet the size
> of struct sme_populate_pgd_data is now large enough to trigger
> a stack check. If I add __nostackprotector to sme_encrypt_kernel
> like sme_enable has, it boots fine. This would explain why that
> particular commit showed as the problem in bisection.

Great find Laura.  It must have something to do with compiler levels
since my level didn't insert that check.

Thanks,
Tom

> 
> Thanks,
> Laura