Re: [PATCH 0/6 V2] CCIX Protocol error reporting.
Jonathan, On Tue, 20 Aug 2019, Jonathan Cameron wrote: Cc+ linux-s...@vger.kernel.org > The following boilerplate is granting rights to the kernel. > Note that I haven't applied the CCIX copyright notice anywhere in this > series because we aren't quoting from the specification. That is > much more likely to happen in documentation patches than in code. > > Like anything else in this series it is open to comment. > > This patch is being distributed by the CCIX Consortium, Inc. (CCIX) to > you and other parties that are participating (the "participants") in the > Linux kernel with the understanding that the participants will use CCIX's > name and trademark only when this patch is used in association with the > Linux kernel and associated user space. The code is licensed under GPLV2, so what precludes any other GPLV2 project to import that code? If there is a mentioning of CCIX Consortium in the imported code then you cannot impose that this needs to be removed because it ends up in something which is neither Linux kernel nor associated user space. And that's especially true when this ends up being a copyright notice. > CCIX is also distributing this patch to these participants with the > understanding that if any portion of the CCIX specification will be > used or referenced in the Linux kernel, the participants will not modify > the cited portion of the CCIX specification and will give CCIX proper > copyright attribution by including the following copyright notice with > the cited part of the CCIX specification: > "© 2019 CCIX CONSORTIUM, INC. ALL RIGHTS RESERVED." Just to prove the point. Thanks, tglx
Re: [PATCH 1/3] efi/x86: turn EFI runtime semaphore into a global lock
On Wed, 9 Jan 2019, Hedi Berriche wrote: > Make efi_runtime_lock semaphore global so that it can be used by EFI > runtime callers that may be defined outside efi/runtime-wrappers.c. The changelog should mention why the lock is renamed. I have no strong opinion, but to apply that I need an Ack from Ard. Thanks, tglx
Re: [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot.
On Mon, 5 Nov 2018, Ard Biesheuvel wrote: > On 4 November 2018 at 21:10, Sai Praneeth Prakhya > wrote: > Thanks Sai. > > I have queued these up in efi/next for now, but I'd still like a fresh > ack from the x86 maintainers. I've reviewed the CPA part and for the rest feel free to add Acked-by: Thomas Gleixner
Re: [PATCH V3 1/3] x86/mm/pageattr: Introduce helper function to unmap EFI boot services
On Sun, 4 Nov 2018, Sai Praneeth Prakhya wrote: > Ideally, after kernel assumes control of the platform, firmware > shouldn't access EFI boot services code/data regions. But, it's noticed > that this is not so true in many x86 platforms. Hence, during boot, > kernel reserves EFI boot services code/data regions [1] and maps [2] > them to efi_pgd so that call to set_virtual_address_map() doesn't fail. > After returning from set_virtual_address_map(), kernel frees the > reserved regions [3] but they still remain mapped. Hence, introduce > kernel_unmap_pages_in_pgd() which will later be used to unmap EFI boot > services code/data regions. > > While at it modify kernel_map_pages_in_pgd() by > 1. Adding __init modifier because it's always used *only* during boot. > 2. Add a warning if it's used after SMP is initialized because it uses >__flush_tlb_all() which flushes mappings only on current CPU. > > Unmapping EFI boot services code/data regions will result in clearing > PAGE_PRESENT bit and it shouldn't bother L1TF cases because it's already > handled by protnone_mask() at arch/x86/include/asm/pgtable-invert.h. > > [1] efi_reserve_boot_services() > [2] efi_map_region() -> __map_region() -> kernel_map_pages_in_pgd() > [3] efi_free_boot_services() > > Signed-off-by: Sai Praneeth Prakhya > Cc: Borislav Petkov > Cc: Ingo Molnar > Cc: Andy Lutomirski > Cc: Dave Hansen > Cc: Bhupesh Sharma > Cc: Thomas Gleixner > Cc: Peter Zijlstra > Cc: Ard Biesheuvel Reviewed-by: Thomas Gleixner
RE: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Sai, On Mon, 29 Oct 2018, Thomas Gleixner wrote: > On Mon, 29 Oct 2018, Prakhya, Sai Praneeth wrote: > > > Hi Thomas and Peter, > > > > [off the mailing list because I wasn't sure if it's a good idea to spam > > others with my questions] > > In general it's good to do on list because others can learn from the > answers as well. So you somehow managed to keep everyone and the lists in CC nevertheless. Did not pay attention to the CC list until I got my reply on the list :) > > > > > + retval = __change_page_attr_set_clr(, 0); > > > > > + __flush_tlb_all(); > > > > > > > > So this looks like you copied it from kernel_map_pages_in_pgd() which > > > > has been discussed before to be not sufficient, but it can't be > > > > changed right now due to locking issues. > > > > > > Managed to confuse myself. The place which cannot be changed is a > > > different > > > one, but still for your call site __flush_tlb_all() might not be > > > sufficient. > > > > Since the mappings are changed, I thought a flush_tlb() might be needed. > > But, (to be honest), I don't completely understand the x86/mm code. So, > > could you > > please elaborate the issue more for my better understanding? > > > > So some questions I have are, > > 1. What did Peter Z mean here? "How is that not a TLB invalidation bug ?" > > __flush_tlb_all() flushes only the mapping on the current CPU. So in a SMP > environment it's not sufficient. > > > 2. I assumed kernel_map_pages_in_pgd() to be a good code but you said that > > it has some locking issues. So, could you please elaborate more on that. > > I corrected myself. It's the other function which cannot use the proper > cpa_flush.*() functions. > > > Or, could you please provide me some pointers in the source code that I > > can take a look at so that I could understand the issue much better. > > I'll have a second look at the whole thing and reply on list. So actually for the problem at hand __flush_tlb_all() works, because that is called way before the secondary CPUs are up. Ditto for kernel_map_pages_in_pgd(). But both functions want to be marked __init and aside of that they both should contain a warning when they are called after the secondary CPUs have been brought up. Thanks, tglx
RE: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Sai, On Mon, 29 Oct 2018, Prakhya, Sai Praneeth wrote: > Hi Thomas and Peter, > > [off the mailing list because I wasn't sure if it's a good idea to spam > others with my questions] In general it's good to do on list because others can learn from the answers as well. > > > > + retval = __change_page_attr_set_clr(, 0); > > > > + __flush_tlb_all(); > > > > > > So this looks like you copied it from kernel_map_pages_in_pgd() which > > > has been discussed before to be not sufficient, but it can't be > > > changed right now due to locking issues. > > > > Managed to confuse myself. The place which cannot be changed is a different > > one, but still for your call site __flush_tlb_all() might not be sufficient. > > Since the mappings are changed, I thought a flush_tlb() might be needed. > But, (to be honest), I don't completely understand the x86/mm code. So, could > you > please elaborate the issue more for my better understanding? > > So some questions I have are, > 1. What did Peter Z mean here? "How is that not a TLB invalidation bug ?" __flush_tlb_all() flushes only the mapping on the current CPU. So in a SMP environment it's not sufficient. > 2. I assumed kernel_map_pages_in_pgd() to be a good code but you said that > it has some locking issues. So, could you please elaborate more on that. I corrected myself. It's the other function which cannot use the proper cpa_flush.*() functions. > Or, could you please provide me some pointers in the source code that I > can take a look at so that I could understand the issue much better. I'll have a second look at the whole thing and reply on list. Thanks, tglx
Re: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Sai,
On Mon, 29 Oct 2018, Thomas Gleixner wrote:
> On Fri, 26 Oct 2018, Sai Praneeth Prakhya wrote:
> >
> > +int kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
> > + unsigned long numpages)
> > +{
> > + int retval;
> > +
> > + /*
> > +* The typical sequence for unmapping is to find a pte through
> > +* lookup_address_in_pgd() (ideally, it should never return NULL because
> > +* the address is already mapped) and change it's protections.
> > +* As pfn is the *target* of a mapping, it's not useful while unmapping.
> > +*/
> > + struct cpa_data cpa = {
> > + .vaddr = ,
> > + .pgd= pgd,
> > + .numpages = numpages,
> > + .mask_set = __pgprot(0),
> > + .mask_clr = __pgprot(_PAGE_PRESENT | _PAGE_RW),
> > + .flags = 0,
> > + };
> > +
> > + retval = __change_page_attr_set_clr(, 0);
> > + __flush_tlb_all();
>
> So this looks like you copied it from kernel_map_pages_in_pgd() which has
> been discussed before to be not sufficient, but it can't be changed right
> now due to locking issues.
Managed to confuse myself. The place which cannot be changed is a different
one, but still for your call site __flush_tlb_all() might not be sufficient.
Thanks,
tglx
Re: [PATCH V2 2/2] x86/efi: Move efi__boot_services() to arch/x86
On Fri, 26 Oct 2018, Sai Praneeth Prakhya wrote: > efi__boot_services() are x86 specific quirks and as such > should be in asm/efi.h, so move them from linux/efi.h. Also, call > efi_free_boot_services() from __efi_enter_virtual_mode() as it is x86 > specific call and ideally shouldn't be part of init/main.c > > Signed-off-by: Sai Praneeth Prakhya Acked-by: Thomas Gleixner
Re: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Sai,
On Fri, 26 Oct 2018, Sai Praneeth Prakhya wrote:
>
> +int kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
> + unsigned long numpages)
> +{
> + int retval;
> +
> + /*
> + * The typical sequence for unmapping is to find a pte through
> + * lookup_address_in_pgd() (ideally, it should never return NULL because
> + * the address is already mapped) and change it's protections.
> + * As pfn is the *target* of a mapping, it's not useful while unmapping.
> + */
> + struct cpa_data cpa = {
> + .vaddr = ,
> + .pgd= pgd,
> + .numpages = numpages,
> + .mask_set = __pgprot(0),
> + .mask_clr = __pgprot(_PAGE_PRESENT | _PAGE_RW),
> + .flags = 0,
> + };
> +
> + retval = __change_page_attr_set_clr(, 0);
> + __flush_tlb_all();
So this looks like you copied it from kernel_map_pages_in_pgd() which has
been discussed before to be not sufficient, but it can't be changed right
now due to locking issues.
For this particular use case, this should not be an issue at all, so please
use the proper cpa_flush_*() variant.
> +
> + return retval;
> +}
> +
> /*
> * The testcases use internal knowledge of the implementation that shouldn't
> * be exposed to the rest of the kernel. Include these directly here.
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 669babcaf245..fb1c44b11235 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
While you are at it, can you please split the EFI part out into a separate
patch?
Thanks,
tglx
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On Wed, 10 Oct 2018, Baoquan He wrote: > Hi Boris, > > On 10/10/18 at 10:59am, Borislav Petkov wrote: > > ... and we just picked up > > > > https://lkml.kernel.org/r/20181001140843.26137-1-msys.miz...@gmail.com > > > > and without having looked at the rest of your stuff, if people accept > > your solution, we don't need the silly parameter anymore, right? > > > > Which means, we should not rush the whole thing yet until the whole > > KASLR vs movable memory gets solved properly. > > Masa's patches solves the problem in memory region KASLR which later hot > added memory may be big than the default padding 10 TB. > > Chao's patches is trying to fix a conflict between 'movable_node' and > kernel text KASLR. If 'movable_node' specified, we rely on SRAT to get > which memory region is movable or immovable, and movable region can be > hot removed. But if kernel is randomized into movable memory, it can't > be hot removed any more, this is a regression after KASLR introduced. > So this is a different issue than Masa's. Yes, it's different, but if the SRAT information is available early, then the command line parameter can go away because then the required information for Masa's problem is available as well. Thanks, tglx
Re: [GIT PULL 0/1] EFI fix for v4.19-rc
On Tue, 18 Sep 2018, Ard Biesheuvel wrote: > The following changes since commit 11da3a7f84f19c26da6f86af878298694ede0804: > > Linux 4.19-rc3 (2018-09-09 17:26:43 -0700) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git efi-urgent > > for you to fetch changes up to d310959365942b100f79b56bcce859968fe7ca9c: > > efi/libstub/arm: default EFI_ARMSTUB_DTB_LOADER to y (2018-09-12 16:41:41 > +0200) > > > Apply a fix from Scott to make the ARM stub's DTB loader opt-out rather > than opt-in. Pulled. Thanks Ard! tglx
Re: [PATCH V6 2/2] x86/efi: Add efi page fault handler to recover from page faults caused by the firmware
On Tue, 11 Sep 2018, Sai Praneeth Prakhya wrote:
> diff --git a/drivers/firmware/efi/runtime-wrappers.c
> b/drivers/firmware/efi/runtime-wrappers.c
> index b18b2d864c2c..7455277a3b65 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -61,6 +61,11 @@ struct efi_runtime_work efi_rts_work;
> ({ \
> efi_rts_work.status = EFI_ABORTED; \
> \
> + if (!efi_enabled(EFI_RUNTIME_SERVICES)) { \
> + pr_info("Aborting! EFI Runtime Services disabled\n"); \
This probaby wants to be pr_info_ince().
Other than that:
Reviewed-by: Thomas Gleixner
Re: [PATCH V4 3/3] x86/efi: Introduce EFI_PAGE_FAULT_HANDLER
On Sat, 8 Sep 2018, Bhupesh Sharma wrote: > On Sat, Sep 8, 2018 at 12:52 AM, Thomas Gleixner wrote: > > If the distro patched their kernel to deal with buggy firmware, then: > > > > 1) why did they not upstream it ? > > Because some of the kernel fixes are (for such cases), well to be > honest, ugly.. and probably not suitable for placement in quirks > files/common code present upstream as they introduce lots of #ifdef > jugglery. > > Also the x86 machines with such buggy BIOS firmwares are too old (but > still used in some production environment) and the OS workarounds are > suggested by the vendors themselves and they historically had issues > getting the quirks upstream. Right, because they did not even try. And the distros just integrated that mess instead of telling them to fix the crappy firmware. And that can be fixed. I know for sure because the RT qualification program at RH has made vendors to fix their crap in order to get the rubber stamp. > > 2) why should we worry about that ? > > As this allows one to still promote upgrading such machines to > upstream kernel versions and keep the kernel running on them as close > as possible to mainline. If the firmware is buggy and trips faults in the EFI mess then mainline will just crash and burn. So how are you going to upgrade these machines to a mainline kernel with that fault handler disabled? Not at all. If you need to patch the kernel in order to make it work on mainline, then still that EFI fault handler has ZERO impact. Because those patches need to prevent the firmware from faulting in the first place. If they have magic fault handlers for this crap themself, then the patches will conflict anyway, config switch or not. You have to come up with something more convincing. Thanks, tglx
RE: [PATCH V4 3/3] x86/efi: Introduce EFI_PAGE_FAULT_HANDLER
On Fri, 7 Sep 2018, Prakhya, Sai Praneeth wrote: > > > So, if Thomas, Ingo, Andy, Ard and Boris are ok.. I will make it as > > > default (i.e. without config). Yes, that's the right thing to do. > > Also, some distributions already have specific ways to handle buggy > > firmwares which can be at times dependent on the underlying hardware > > and the firmware versions. If the distro patched their kernel to deal with buggy firmware, then: 1) why did they not upstream it ? 2) why should we worry about that ? > > I would suggest that we enable this under a CONFIG for the first round > > and once it is tested with wider variety of x86 machines which have > > buggy or orphaned firmware and linux (and reboot) works fine with them, > > we can drop the CONFIG option in future and enable this by default. Sure and then nobody enables it and the affected machines still crash or hang on reboot. The whole thing is simple enough now to make it unconditional. > Sounds fair to me, but, I would like to wait for someone experienced to > make the final call. Please get rid of that config knob. Buggy firmware exists and we better deal with it by default. Thanks, tglx
Re: [PATCH V3 3/5] x86/efi: Permanently save the EFI_MEMORY_MAP passed by the firmware
On Wed, 5 Sep 2018, Ard Biesheuvel wrote: > On 5 September 2018 at 14:56, Peter Zijlstra wrote: > > On Wed, Sep 05, 2018 at 02:27:49PM +0200, Ard Biesheuvel wrote: > >> Would we still need to preserve the old memory map in that case? > > > > I thought the reason for having this was being able to know the fault is > > in an EFI area. But of course, I'm not wel versed in this whole EFI > > crapola. > > I'm not entirely sure whether that really matters. The EFI services > access the stack and can access byref/pointer arguments which are not > covered by the EFI memory map as runtime services code/data, and so > they can trigger page faults by running off the vmapped stack or > writing to const byref arguments. > > EFI runtime services using boot services regions after they are no > longer available are a known source of headaches, but I don't see why > we should restrict ourselves to such cases if we bother to wire up > fault handling specifically for EFI services calls. > > So any page or permission fault occurring in the context of a EFI > runtime services invocation should be treated the same, I think. I agree. Keep it simple. If the EFI crap fails, then assist with the reboot and otherwise just kill it. Thanks, tglx
Re: [PATCH V2 4/6] x86/efi: Add efi page fault handler to fixup/recover from page faults caused by firmware
On Sun, 2 Sep 2018, Sai Praneeth Prakhya wrote: > Kernel panics/hangs because the memory region requested by the firmware > isn't mapped, which causes a page fault in ring 0 and the kernel fails > to handle it, leading to die(). To save kernel from hanging, add an efi > specific page fault handler which detects illegal accesses by the > firmware and > 1. If the illegally accessed region is EFI_BOOT_SERVICES_, >the efi page fault handler fixes it up by mapping the requested >region. > 2. If any other region (Eg: EFI_CONVENTIONAL_MEMORY or >EFI_LOADER_), then the efi page fault handler freezes >efi_rts_wq and schedules a new process. > 3. If the access is to any other efi region like above but if the efi >runtime service is efi_reset_system(), then the efi page fault >handler will reboot the machine through BIOS. > > Illegal accesses to EFI_BOOT_SERVICES_ and to other regions > are dealt differently in efi page fault handler because, *generally* > EFI_BOOT_SERVICES_ regions are smaller in size relative to > other efi regions and hence could be reserved and can be dynamically > mapped. But other EFI regions like EFI_CONVENTIONAL_MEMORY and > EFI_LOADER_ cannot be reserved as they are very huge in size > and reserving them will make the kernel un-bootable. > > The efi specific page fault handler offers us two advantages: > 1. Avoid panics/hangs caused by buggy firmware. > 2. Shout loud that the firmware is buggy and hence is not a kernel bug. > > Finally, this new mapping will not impact a reboot from kexec, as kexec > is only concerned about runtime memory regions. No. This is just a horrible hack to make completely bogus firmware work and never fixed. The proper thing to do is to have a minimal page fault handler which does: 1) Yell loudly if that ever happens 2) Handles the reboot request gracefully 3) Freeze and disable the EFI mess for all other cases That does not require any hackery to make these mappings work from atomic context and keeps the mess confined to the EFI code where it belongs. Ideally we just blacklist the offending system and be done with it. Thanks, tglx
Re: [PATCH V2 5/6] x86/mm: If in_atomic(), allocate pages without sleeping
On Mon, 3 Sep 2018, Peter Zijlstra wrote:
Thanks for CC'ing me. I wonder why the world and some more is on CC, but
x...@kernel.org is NOT.
> On Sun, Sep 02, 2018 at 02:46:33AM -0700, Sai Praneeth Prakhya wrote:
> > @@ -926,7 +926,13 @@ static void unmap_pud_range(p4d_t *p4d, unsigned long
> > start, unsigned long end)
> >
> > static int alloc_pte_page(pmd_t *pmd)
> > {
> > - pte_t *pte = (pte_t *)get_zeroed_page(GFP_KERNEL);
> > + pte_t *pte;
> > +
> > + if (in_atomic())
> > + pte = (pte_t *)get_zeroed_page(GFP_ATOMIC);
> > + else
> > + pte = (pte_t *)get_zeroed_page(GFP_KERNEL);
> > +
> > if (!pte)
> > return -1;
> >
>
> This looks like tinkering to me..
Yes it is and it's not going to happen.
Thanks,
tglx
Re: [PATCH] x86/efi: Fix incorrect invocation of PciIo->Attributes()
On Sun, 24 Jun 2018, Ard Biesheuvel wrote: > On 24 June 2018 at 15:43, Thomas Gleixner wrote: > > On Sun, 24 Jun 2018, Ard Biesheuvel wrote: > >> On 24 June 2018 at 15:16, Hans de Goede wrote: > >> > Ard, thank you for Cc-ing me on this. > >> > > >> > I've tested a 64 bit kernel build on a 32 bit UEFI firmware (so mixed > >> > mode) > >> > and this patch causes a reboot loop there. I do get grub (no surprise > >> > there > >> > as grub is unchanged), but as soon as the kernel loads the device resets. > >> > > >> > So I think we need some special casing there to deal with a 64 bit kernel > >> > calling into 32 bit UEFI. > >> > > >> > >> OK, so mixed mode rears its ugly hand again :-( > >> > >> Considering we had other weird issues involving uint64_t types with > >> the TPM code just this week, I wonder if this isn't a fundamental > >> problem with the mixed mode thunking, and so I need some help from the > >> x86 gurus (Ingo?) > >> > >> If the thunking code simply maps each 64-bit argument onto a 32-bit > >> stack slot, it is obvious that passing uint64_t type arguments is > >> impossible. > >> > >> So would it be possible to have a efi_config::call() variant that is > >> annotated as expecting the i386 calling convention, and let the > >> compiler handle this? All we'd need to do in the mixed mode thunking > >> code is pushing an additional word [as we do know] for the function > >> pointer. > > > > Grumbl. This thing just went into rc2 a few minutes ago. > > > > Good. Without that patch, 64-bit Linux on 64-bit EFI is broken, which > is much worse. > > It seems mixed mode is fundamentally broken anyway, so we can take a > bit of time to fix it properly Fair enough. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86/efi: Fix incorrect invocation of PciIo->Attributes()
On Sun, 24 Jun 2018, Ard Biesheuvel wrote: > On 24 June 2018 at 15:16, Hans de Goede wrote: > > Ard, thank you for Cc-ing me on this. > > > > I've tested a 64 bit kernel build on a 32 bit UEFI firmware (so mixed mode) > > and this patch causes a reboot loop there. I do get grub (no surprise there > > as grub is unchanged), but as soon as the kernel loads the device resets. > > > > So I think we need some special casing there to deal with a 64 bit kernel > > calling into 32 bit UEFI. > > > > OK, so mixed mode rears its ugly hand again :-( > > Considering we had other weird issues involving uint64_t types with > the TPM code just this week, I wonder if this isn't a fundamental > problem with the mixed mode thunking, and so I need some help from the > x86 gurus (Ingo?) > > If the thunking code simply maps each 64-bit argument onto a 32-bit > stack slot, it is obvious that passing uint64_t type arguments is > impossible. > > So would it be possible to have a efi_config::call() variant that is > annotated as expecting the i386 calling convention, and let the > compiler handle this? All we'd need to do in the mixed mode thunking > code is pushing an additional word [as we do know] for the function > pointer. Grumbl. This thing just went into rc2 a few minutes ago. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Wed, 4 Apr 2018, Peter Jones wrote: > That is to say, as a result of the way malware has been written, our way > of thinking about it is often that it's a way to build a boot loader for > a malicious kernel, so that's how we wind up talking about it. Are we > concerned with malware stealing your data? Yes, but Secure Boot is only > indirectly about that. It's primarily about denying the malware easy > mechanisms to build a persistence mechanism. The uid-0 != ring-0 aspect > is useful independent of Secure Boot, but Secure Boot without it falls > way short of accomplishing its goal. I think we can all agree that The uid-0 != ring-0 aspect is useful independent of Secure Boot There is probably resonable consensus about the second part of this sentence as well: but Secure Boot without it falls way short of accomplishing its goal. Now where the disagreement lies is the way how the uid/ring0 aspect is tied to secure boot, which makes it impossible to be useful independent of Secure Boot. So the real question is, how can we make 'lockdown' usable and useful without Secure Boot and at the same time not violate the constraints of the Secure Boot scenario. If we can agree on the above then I hope that we can focus on the technical problems instead of arguing in circles. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 00/38] x86: Secure Memory Encryption (AMD)
On Mon, 17 Jul 2017, Tom Lendacky wrote: > This patch series provides support for AMD's new Secure Memory Encryption > (SME) > feature. > > SME can be used to mark individual pages of memory as encrypted through the > page tables. A page of memory that is marked encrypted will be automatically > decrypted when read from DRAM and will be automatically encrypted when > written to DRAM. Details on SME can found in the links below. > > The SME feature is identified through a CPUID function and enabled through > the SYSCFG MSR. Once enabled, page table entries will determine how the > memory is accessed. If a page table entry has the memory encryption mask set, > then that memory will be accessed as encrypted memory. The memory encryption > mask (as well as other related information) is determined from settings > returned through the same CPUID function that identifies the presence of the > feature. > > The approach that this patch series takes is to encrypt everything possible > starting early in the boot where the kernel is encrypted. Using the page > table macros the encryption mask can be incorporated into all page table > entries and page allocations. By updating the protection map, userspace > allocations are also marked encrypted. Certain data must be accounted for > as having been placed in memory before SME was enabled (EFI, initrd, etc.) > and accessed accordingly. > > This patch series is a pre-cursor to another AMD processor feature called > Secure Encrypted Virtualization (SEV). The support for SEV will build upon > the SME support and will be submitted later. Details on SEV can be found > in the links below. Well done series. Thanks to all people involved, especially Tom and Boris! It was a pleasure to review that. Reviewed-by: Thomas Gleixner <t...@linutronix.de> -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 08/36] x86/mm: Add support to enable SME in early boot processing
On Wed, 21 Jun 2017, Tom Lendacky wrote: > On 6/21/2017 2:16 AM, Thomas Gleixner wrote: > > Why is this an unconditional function? Isn't the mask simply 0 when the MEM > > ENCRYPT support is disabled? > > I made it unconditional because of the call from head_64.S. I can't make > use of the C level static inline function and since the mask is not a > variable if CONFIG_AMD_MEM_ENCRYPT is not configured (#defined to 0) I > can't reference the variable directly. > > I could create a #define in head_64.S that changes this to load rax with > the variable if CONFIG_AMD_MEM_ENCRYPT is configured or a zero if it's > not or add a #ifdef at that point in the code directly. Thoughts on > that? See below. > > That does not make any sense. Neither the call to sme_encrypt_kernel() nor > > the following call to sme_get_me_mask(). > > > > __startup_64() is already C code, so why can't you simply call that from > > __startup_64() in C and return the mask from there? > > I was trying to keep it explicit as to what was happening, but I can > move those calls into __startup_64(). That's much preferred. And the return value wants to be documented in both C and ASM code. > I'll still need the call to sme_get_me_mask() in the secondary_startup_64 > path, though (depending on your thoughts to the above response). call verify_cpu movq$(init_top_pgt - __START_KERNEL_map), %rax So if you make that: /* * Sanitize CPU configuration and retrieve the modifier * for the initial pgdir entry which will be programmed * into CR3. Depends on enabled SME encryption, normally 0. */ call __startup_secondary_64 addq$(init_top_pgt - __START_KERNEL_map), %rax You can hide that stuff in C-code nicely without adding any cruft to the ASM code. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 10/36] x86/mm: Provide general kernel support for memory encryption
On Fri, 16 Jun 2017, Tom Lendacky wrote: > > +#ifndef pgprot_encrypted > +#define pgprot_encrypted(prot) (prot) > +#endif > + > +#ifndef pgprot_decrypted That looks wrong. It's not decrypted it's rather unencrypted, right? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 07/36] x86/mm: Don't use phys_to_virt in ioremap() if SME is active
On Fri, 16 Jun 2017, Tom Lendacky wrote: > Currently there is a check if the address being mapped is in the ISA > range (is_ISA_range()), and if it is then phys_to_virt() is used to > perform the mapping. When SME is active, however, this will result > in the mapping having the encryption bit set when it is expected that > an ioremap() should not have the encryption bit set. So only use the > phys_to_virt() function if SME is not active This does not make sense to me. What the heck has phys_to_virt() to do with the encryption bit. Especially why would the encryption bit be set on that mapping in the first place? I'm probably missing something, but this want's some coherent explanation understandable by mere mortals both in the changelog and the code comment. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/24] x86: Restrict MSR access when the kernel is locked down
On Wed, 5 Apr 2017, David Howells wrote: Can you please change the subsys in $subject to 'x86/msr:' ? > From: Matthew Garrett <matthew.garr...@nebula.com> > > Writing to MSRs should not be allowed if the kernel is locked down, since > it could lead to execution of arbitrary code in kernel mode. Based on a > patch by Kees Cook. > > Signed-off-by: Matthew Garrett <matthew.garr...@nebula.com> > Signed-off-by: David Howells <dhowe...@redhat.com> > Acked-by: Kees Cook <keesc...@chromium.org> > cc: x...@kernel.org Reviewed-by: Thomas Gleixner <t...@linutronix.de> -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/24] x86: Lock down IO port access when the kernel is locked down
On Wed, 5 Apr 2017, David Howells wrote: > From: Matthew Garrett <matthew.garr...@nebula.com> > > IO port access would permit users to gain access to PCI configuration > registers, which in turn (on a lot of hardware) give access to MMIO > register space. This would potentially permit root to trigger arbitrary > DMA, so lock it down by default. > > This also implicitly locks down the KDADDIO, KDDELIO, KDENABIO and > KDDISABIO console ioctls. > > Signed-off-by: Matthew Garrett <matthew.garr...@nebula.com> > Signed-off-by: David Howells <dhowe...@redhat.com> Reviewed-by: Thomas Gleixner <t...@linutronix.de> -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains page frame numbers
On Mon, 16 Nov 2015, Thomas Gleixner wrote:
> On Mon, 16 Nov 2015, Borislav Petkov wrote:
> > On Mon, Nov 16, 2015 at 09:19:01PM +0100, Thomas Gleixner wrote:
> > > On Sat, 14 Nov 2015, Matt Fleming wrote:
> > > > The x86 pageattr code is confused about the data that is stored
> > > > cpa->pfn, sometimes it's treated as a page frame number, sometimes
> > > > it's treated as an unshifted physical address, and in one place it's
> > > > treated as a pte.
> > >
> > > Yuck.
> >
> > This paragraph should read like this instead:
> >
> > "Boris used cpa->pfn as a scratch variable to contain the physical
> > address. He realizes now that he should've added a separate
> > cpa_data.phys_addr then, instead of confusing everybody."
> >
> > > > The result of this is that the mapping functions do not map the
> > > > intended physical address.
> > > >
> > > > This isn't a problem in practice because most of the addresses we're
> > > > mapping in the EFI code paths are already mapped in 'trampoline_pgd'
> > > > and so the pageattr mappings functions don't actually do anything in
> > > > this case. But when we move to using a separate page table for the EFI
> > > > runtime this will be an issue.
> > >
> > > Are you sure that this does not affect existing kernel versions?
> >
> > Shouldn't because with this new patchset we're copying all the PGDs from
> > the kernel page table before doing an EFI call, see
> > efi_sync_low_kernel_mappings() in patch 5.
> >
> > > > while (num_pages-- && start < end) {
> > > > -
> > > > - /* deal with the NX bit */
> > > > - if (!(pgprot_val(pgprot) & _PAGE_NX))
> > > > - cpa->pfn &= ~_PAGE_NX;
> > >
> > > That should be a seperate patch because this is just bogus code and
> > > has nothing to do with the pfn confusion.
> >
> > Why bogus?
>
> Even with cpa->pfn used as an address it cannot ever be set as the
> address is page aligned
Gah. Misread it. _PAGE_NX is bit 63 and it can be set when cpa->pfn is
abused as an address. So yes, it should go away with that patch.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Remove EFI memmap quirk for UV
On Mon, 16 Nov 2015, Alex Thorlton wrote:
CC'ing Matt under his correct e-mail address.
> Commit a5d90c923bcf ("x86/efi: Quirk out SGI UV") added a quirk to
> efi_apply_memmap_quirks to force SGI UV systems to fall back to the old
> EFI memmap mechanism. We have a BIOS fix for this issue now, so we no
> longer need this quirk in the kernel. This commit removes the quirk
> from the function in question.
>
> Signed-off-by: Alex Thorlton <athorl...@sgi.com>
> Acked-by: Mike Travis <tra...@sgi.com>
> Acked-by: Russ Anderson <r...@sgi.com>
> Cc: Matt Fleming <matt.flem...@intel.com>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Ingo Molnar <mi...@redhat.com>
> Cc: "H. Peter Anvin" <h...@zytor.com>
> Cc: Hedi Berriche <h...@sgi.com>
> Cc: Dimitri Sivanich <sivan...@sgi.com>
> Cc: x...@kernel.org
> Cc: linux-efi@vger.kernel.org
>
> ---
> arch/x86/platform/efi/quirks.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 1c7380d..96b417c 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -259,12 +259,6 @@ void __init efi_apply_memmap_quirks(void)
> pr_info("efi: Setup done, disabling due to 32/64-bit
> mismatch\n");
> efi_unmap_memmap();
> }
> -
> - /*
> - * UV doesn't support the new EFI pagetable mapping yet.
> - */
> - if (is_uv_system())
> - set_bit(EFI_OLD_MEMMAP, );
> }
>
> /*
> --
> 1.8.5.6
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains page frame numbers
On Mon, 16 Nov 2015, Borislav Petkov wrote:
> On Mon, Nov 16, 2015 at 09:19:01PM +0100, Thomas Gleixner wrote:
> > On Sat, 14 Nov 2015, Matt Fleming wrote:
> > > The x86 pageattr code is confused about the data that is stored
> > > cpa->pfn, sometimes it's treated as a page frame number, sometimes
> > > it's treated as an unshifted physical address, and in one place it's
> > > treated as a pte.
> >
> > Yuck.
>
> This paragraph should read like this instead:
>
> "Boris used cpa->pfn as a scratch variable to contain the physical
> address. He realizes now that he should've added a separate
> cpa_data.phys_addr then, instead of confusing everybody."
>
> > > The result of this is that the mapping functions do not map the
> > > intended physical address.
> > >
> > > This isn't a problem in practice because most of the addresses we're
> > > mapping in the EFI code paths are already mapped in 'trampoline_pgd'
> > > and so the pageattr mappings functions don't actually do anything in
> > > this case. But when we move to using a separate page table for the EFI
> > > runtime this will be an issue.
> >
> > Are you sure that this does not affect existing kernel versions?
>
> Shouldn't because with this new patchset we're copying all the PGDs from
> the kernel page table before doing an EFI call, see
> efi_sync_low_kernel_mappings() in patch 5.
>
> > > while (num_pages-- && start < end) {
> > > -
> > > - /* deal with the NX bit */
> > > - if (!(pgprot_val(pgprot) & _PAGE_NX))
> > > - cpa->pfn &= ~_PAGE_NX;
> >
> > That should be a seperate patch because this is just bogus code and
> > has nothing to do with the pfn confusion.
>
> Why bogus?
Even with cpa->pfn used as an address it cannot ever be set as the
address is page aligned
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] EFI urgent fix
On Wed, 4 Nov 2015, Matt Fleming wrote: > for you to fetch changes up to 5965d1bbeba70fe3626e4537f4729283cb0e75f7: > > x86/setup: Fix recent boot crash on 32-bit SMP machines (2015-11-04 > 09:26:24 +) I just picked that up manually :) -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
