Re: [RFT PATCH] efi/x86: move x86 back to libstub

2014-10-16 Thread Josh Boyer
On Thu, Oct 16, 2014 at 10:42 AM, Matt Fleming m...@console-pimps.org wrote:
 On Mon, 06 Oct, at 01:06:43PM, Ard Biesheuvel wrote:
 This reverts commit 84be880560fb, which itself reverted my original
 attempt to move x86 from #include'ing .c files from across the tree
 to using the EFI stub built as a static library.

 The issue that affected the original approach was that splitting
 the implementation into several .o files resulted in the variable
 'efi_early' becoming a global with external linkage, which under
 -fPIC implies that references to it must go through the GOT. However,
 dealing with this additional GOT entry turned out to be troublesome
 on some EFI implementations. (GCC's visibility=hidden attribute is
 supposed to lift this requirement, but it turned out not to work on
 the 32-bit build.)

 Instead, use a pure getter function to get a reference to efi_early.
 This approach results in no additional GOT entries being generated,
 so there is no need for any changes in the early GOT handling.

 Cc: Maarten Lankhorst maarten.lankho...@canonical.com
 Cc: Josh Boyer jwbo...@fedoraproject.org
 Cc: Linus Torvalds torva...@linux-foundation.org
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---

 Gents,

 This is a request for testing: I would like to find out if this patch
 fixes Maarten's issue without breaking anything like it did for Josh
 and Linus the first time around.


  arch/x86/boot/compressed/Makefile |  3 ++-
  arch/x86/boot/compressed/eboot.c  |  8 
  arch/x86/boot/compressed/eboot.h  | 16 
  arch/x86/include/asm/efi.h| 24 
  drivers/firmware/efi/Makefile |  2 +-
  5 files changed, 31 insertions(+), 22 deletions(-)

 Ard, this is a very neat trick.

 Thanks for taking the time to reimplement this, it's much appreciated.

 Things work fine on my end, and I do have a Fedora 20/grub box in my
 testing farm now, so I'm confident this is a good patch.

 But if other people want to test it out, I'd be grateful of some more
 Tested-by tags (I've already picked up Maarten's).

I applied this (with minor massaging) on top if Linus' tree as of
commit 0429fbc0bdc297d64188483ba029a23773ae07b0 this morning.  It
built and booted on the macbook machine I originally reported the
problem with, so it does appear to be fixed in that case.  I'll try it
on some other machines likely tomorrow.

Sorry for the delay on this.  The merge window is particularly busy for me.

josh
--
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: Trusted kernel patchset for Secure Boot lockdown

2014-02-27 Thread Josh Boyer
On Wed, Feb 26, 2014 at 3:11 PM, Matthew Garrett
matthew.garr...@nebula.com wrote:
 The conclusion we came to at Plumbers was that this patchset was basically
 fine but that Linus hated the name securelevel more than I hate pickled
 herring, so after thinking about this for a few months I've come up with
 Trusted Kernel. This flag indicates that the kernel is, via some
 external mechanism, trusted and should behave that way. If firmware has
 some way to verify the kernel, it can pass that information on. If userspace
 has some way to verify the kernel, it can set the flag itself. However,
 userspace should not attempt to use the flag as a means to verify that the
 kernel was trusted - untrusted userspace could have set it on an untrusted
 kernel, but by the same metric an untrusted kernel could just set it itself.

FWIW, I've been running a kernel using this patchset in place of the
patchset Fedora typically carries for this purpose for a bit.  Things
appear to be working as expected and the protections remain the same.

It would be really nice to get this set of patches in so some of the
other patches that depend on them can start being pushed as well.

josh
--
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: Trusted kernel patchset for Secure Boot lockdown

2014-02-27 Thread Josh Boyer
On Thu, Feb 27, 2014 at 2:07 PM, Greg KH gre...@linuxfoundation.org wrote:
 On Thu, Feb 27, 2014 at 01:04:34PM -0500, Josh Boyer wrote:
 On Wed, Feb 26, 2014 at 3:11 PM, Matthew Garrett
 matthew.garr...@nebula.com wrote:
  The conclusion we came to at Plumbers was that this patchset was basically
  fine but that Linus hated the name securelevel more than I hate pickled
  herring, so after thinking about this for a few months I've come up with
  Trusted Kernel. This flag indicates that the kernel is, via some
  external mechanism, trusted and should behave that way. If firmware has
  some way to verify the kernel, it can pass that information on. If 
  userspace
  has some way to verify the kernel, it can set the flag itself. However,
  userspace should not attempt to use the flag as a means to verify that the
  kernel was trusted - untrusted userspace could have set it on an untrusted
  kernel, but by the same metric an untrusted kernel could just set it 
  itself.

 FWIW, I've been running a kernel using this patchset in place of the
 patchset Fedora typically carries for this purpose for a bit.  Things
 appear to be working as expected and the protections remain the same.

 It would be really nice to get this set of patches in so some of the
 other patches that depend on them can start being pushed as well.

 What other patches depend on this series?  Why aren't they also in this
 series?

The patches we have to import certificates from the UEFI db and dbx
vars, and MokListRT and apply them to signed module verification.
Looking at them closely, there are pieces that could be sent now as
they are slightly orthogonal to what this patchset is doing, which is
probably why they aren't in this patchset to begin with.  I'll have to
figure out which of those actually depend on anything in Matthew's
series.

josh
--
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 07/12] acpi: Ignore acpi_rsdp kernel parameter when securelevel is set

2013-11-26 Thread Josh Boyer
On Mon, Sep 9, 2013 at 11:49 AM, Matthew Garrett
matthew.garr...@nebula.com wrote:
 From: Josh Boyer jwbo...@redhat.com

 This option allows userspace to pass the RSDP address to the kernel, which
 makes it possible for a user to execute arbitrary code in the kernel.
 Disable this when securelevel is set.

 Signed-off-by: Josh Boyer jwbo...@redhat.com

Dredging up an old thread in the hopes that Matthew runs sed and resubmits...

Also, FWIW, I didn't write this.  It was derived from previous
versions of something I did write, but there's really no evidence of
anything I wrote left, so it should probably be From: you.

 ---
  drivers/acpi/osl.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

 diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
 index e5f416c..f6d8977 100644
 --- a/drivers/acpi/osl.c
 +++ b/drivers/acpi/osl.c
 @@ -45,6 +45,7 @@
  #include linux/list.h
  #include linux/jiffies.h
  #include linux/semaphore.h
 +#include linux/security.h

  #include asm/io.h
  #include asm/uaccess.h
 @@ -249,7 +250,7 @@ early_param(acpi_rsdp, setup_acpi_rsdp);
  acpi_physical_address __init acpi_os_get_root_pointer(void)
  {
  #ifdef CONFIG_KEXEC
 -   if (acpi_rsdp)
 +   if (acpi_rsdp  (get_securelevel = 0))

This is missing some ( ).  That means you're comparing the
get_securelevel function pointer to 0.  Pretty sure bad things will
happen.

josh
--
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 00/12] One more attempt at useful kernel lockdown

2013-09-09 Thread Josh Boyer
On Mon, Sep 9, 2013 at 4:10 PM, David Lang da...@lang.hm wrote:
 On Mon, 9 Sep 2013, Josh Boyer wrote:

 On Mon, Sep 9, 2013 at 3:41 PM, H. Peter Anvin h...@zytor.com wrote:

 On 09/09/2013 12:01 PM, valdis.kletni...@vt.edu wrote:

 On Mon, 09 Sep 2013 11:25:38 -0700, David Lang said:

 Given that we know that people want signed binaries without
 blocking kexec, you should have '1' just enforce module signing
 and '2' (or higher) implement a full lockdown including kexec.


 Or, eliminate the -1  permanently insecure option and make this a
 bitmask, if someone wants to enable every possible lockdown, have
 them set it to all 1's, define the bits only as you need them.


 This strikes me as much more workable than one big sledgehammer.


 I.e. capabilities ;)


 Circles.  All I see here are circles.


 the thing is that these are not circles. they are separate orthoginal things
 that you may or may not want to allow.

 If this was a simple set of circles, then this could be defined as a vector
 instead of bitmap, the further you go the more secure you are.

I didn't mean your recommendation of using a bitmask.  I understood
your proposal and I don't even disagree with it really.  I was
replying to something else.

josh
--
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 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

2013-09-04 Thread Josh Boyer
On Wed, Sep 4, 2013 at 6:51 AM, joeyli j...@suse.com wrote:
 於 五,2013-08-30 於 19:41 -0400,Josh Boyer 提到:
 On Fri, Aug 30, 2013 at 01:46:30PM -0700, H. Peter Anvin wrote:
  On 08/29/2013 11:37 AM, Josh Boyer wrote:
setup_efi_pci(boot_params);
   diff --git a/arch/x86/include/uapi/asm/bootparam.h 
   b/arch/x86/include/uapi/asm/bootparam.h
   index c15ddaf..d35da96 100644
   --- a/arch/x86/include/uapi/asm/bootparam.h
   +++ b/arch/x86/include/uapi/asm/bootparam.h
   @@ -131,7 +131,8 @@ struct boot_params {
__u8  eddbuf_entries;   /* 0x1e9 */
__u8  edd_mbr_sig_buf_entries;  /* 0x1ea */
__u8  kbd_status;   /* 0x1eb */
   -__u8  _pad5[3]; /* 0x1ec */
   +__u8  secure_boot;  /* 0x1ec */
   +__u8  _pad5[2]; /* 0x1ec */
/*
 * The sentinel is set to a nonzero value (0xff) in header.S.
 *
  
   You need to include the following chunk of code with this, otherwise the
   secure_boot variable gets cleared.
  
 
  Not really.
 
  There are three cases:
 
  1. Boot stub only.  Here we do the right thing with the bootparams.
  2. Boot loader bypasses the boot stub completely.  Here we MUST NOT do
 what you suggest above.
  3. Boot stub with a boot_params structure passed in.  Here we should
 run sanitize_boot_params() (an inline for a reason) in the boot
 stub, before we set the secure_boot field.  Once that is done, we
 again don't need that modification.

 OK.  If 3 works, then great.  All I know is that Fedora has been
 carrying the above hunk for months and it was missing in this patch set.
 So when I went to test it, the patches didn't do anything because the
 secure_boot field was getting cleared.

 I'm more than happy to try option 3, and I'll poke at it next week
 unless someone beats me to it.

 josh

 The secure_boot field cleaned by sanitize_boot_params() when using grub2
 linuxefi to load efi stub kernel.
 I printed the boot_params-sentinel value, confirm this value is NOT 0
 when running grub2 linuxefi path, the entry point is efi_stub_entry.

 On the other hand,
 the sentinel value is 0 when direct run efi stub kernel in UEFI shell,
 the secure_boot field can keep.

 Does that mean grub2 should clean the sentinel value? or we move the get
 secure_boot value to efi_init()?

See V3 of this patch that Matthew sent yesterday.  It calls
sanitize_boot_params in efi_main before calling get_secure_boot.  I
tested that yesterday and it worked fine.

josh
--
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:

2013-09-04 Thread Josh Boyer
On Wed, Sep 4, 2013 at 11:53 AM, Kees Cook keesc...@chromium.org wrote:
 On Tue, Sep 3, 2013 at 4:50 PM, Matthew Garrett
 matthew.garr...@nebula.com wrote:
 We have two in-kernel mechanisms for restricting module loading - disabling
 it entirely, or limiting it to the loading of modules signed with a trusted
 key. These can both be configured in such a way that even root is unable to
 relax the restrictions.

 However, right now, there's several other straightforward ways for root to
 modify running kernel code. At the most basic level these allow root to
 reset the configuration such that modules can be loaded again, rendering
 the existing restrictions useless.

 This patchset adds additional restrictions to various kernel entry points
 that would otherwise make it straightforward for root to disable enforcement
 of module loading restrictions. It also provides a patch that allows the
 kernel to be configured such that module signing will be automatically
 enabled when the system is booting via UEFI Secure Boot, allowing a stronger
 guarantee of kernel integrity.

 V3 addresses some review feedback and also locks down uswsusp.

 Looks good to me. Consider the entire series:

 Acked-by: Kees Cook keesc...@chromium.org

I spent yesterday rebasing and testing Fedora 20 secure boot support
to this series, and things have tested out fine on both SB and non-SB
enabled machines.

For the series:

Reviewed-by: Josh Boyer jwbo...@fedoraproject.org
Tested-by: Josh Boyer jwbo...@fedoraproject.org

josh
--
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 V3 08/11] kexec: Disable at runtime if the kernel enforces module loading restrictions

2013-09-04 Thread Josh Boyer
On Wed, Sep 4, 2013 at 4:09 PM,  jerry.hoem...@hp.com wrote:
 On Tue, Sep 03, 2013 at 07:50:15PM -0400, Matthew Garrett wrote:
 kexec permits the loading and execution of arbitrary code in ring 0, which
 is something that module signing enforcement is meant to prevent. It makes
 sense to disable kexec in this situation.

 Signed-off-by: Matthew Garrett matthew.garr...@nebula.com


 Matthew,

 Disabling kexec will disable kdump, correct?

Yes.

 Are there plans to enable kdump on a system where secure
 boot is enabled?

Vivek Goyal has been working on this.  I've not seen the code yet, but
I believe it should be posted somewhere relatively soon.  We're also
planning on talking about it at the Secure Boot microconference at
Linux Plumbers in two weeks.

josh
--
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 01/10] Add secure_modules() call

2013-08-29 Thread Josh Boyer
On Mon, Aug 19, 2013 at 01:26:02PM -0400, Matthew Garrett wrote:
 Provide a single call to allow kernel code to determine whether the system
 has been configured to either disable module loading entirely or to load
 only modules signed with a trusted key.
 
 Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
 ---
  include/linux/module.h | 7 +++
  kernel/module.c| 9 +
  2 files changed, 16 insertions(+)
 
 diff --git a/include/linux/module.h b/include/linux/module.h
 index 46f1ea0..0c266b2 100644
 --- a/include/linux/module.h
 +++ b/include/linux/module.h
 @@ -509,6 +509,8 @@ int unregister_module_notifier(struct notifier_block * 
 nb);
  
  extern void print_modules(void);
  
 +extern bool secure_modules(void);
 +
  #else /* !CONFIG_MODULES... */
  
  /* Given an address, look for it in the exception tables. */
 @@ -619,6 +621,11 @@ static inline int unregister_module_notifier(struct 
 notifier_block * nb)
  static inline void print_modules(void)
  {
  }
 +
 +static inline bool secure_modules(void)
 +{
 + return false;
 +}
  #endif /* CONFIG_MODULES */
  
  #ifdef CONFIG_SYSFS
 diff --git a/kernel/module.c b/kernel/module.c
 index 2069158..801021e 100644
 --- a/kernel/module.c
 +++ b/kernel/module.c
 @@ -3852,3 +3852,12 @@ void module_layout(struct module *mod,
  }
  EXPORT_SYMBOL(module_layout);
  #endif
 +
 +bool secure_modules(void)
 +{
 +#ifdef CONFIG_MODULE_SIG
 + return (sig_enforce || modules_disabled);
 +#else
 + return modules_disabled;
 +#endif
 +}

Needs an EXPORT_SYMBOL/EXPORT_SYMBOL_GPL here, or the patches to drivers
later in the series will fail to link.

josh
--
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 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

2013-08-29 Thread Josh Boyer
On Thu, Aug 29, 2013 at 12:05:47PM -0700, H. Peter Anvin wrote:
 On 08/29/2013 11:49 AM, Matthew Garrett wrote:
 
  No, you mixed and matched in a single patch...
  
  Right, but I'd fixed that in V2 (which I see I *did* send correctly, and
  you're just replying to the old one :))
  
 
 Well, I'm responding to the one that was sent 31 minutes ago.

No patches were sent 31 minutes ago.  And this is definitely in the
first series thread in my inbox.

josh
--
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


[PATCH] efi: Check EFI revision in setup_efi_vars

2013-04-24 Thread Josh Boyer
We need to check the runtime sys_table for the EFI version the firmware
specifies instead of just checking for a NULL QueryVariableInfo.  Older
implementations of EFI don't have QueryVariableInfo but the runtime is
a smaller structure, so the pointer to it may be pointing off into garbage.

This is apparently the case with several Apple firmwares that support EFI
1.10, and the current check causes them to no longer boot.  Fix based on
a suggestion from Matthew Garrett.

Signed-off-by: Josh Boyer jwbo...@redhat.com
---
 arch/x86/boot/compressed/eboot.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 8615f75..b46efbf 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -258,7 +258,9 @@ static efi_status_t setup_efi_vars(struct boot_params 
*params)
u64 store_size, remaining_size, var_size;
efi_status_t status;
 
-   if (!sys_table-runtime-query_variable_info)
+   if (sys_table-runtime-hdr.revision  EFI_2_00_SYSTEM_TABLE_REVISION)
+   return EFI_UNSUPPORTED;
+   else if(!sys_table-runtime-query_variable_info)
return EFI_UNSUPPORTED;
 
data = (struct setup_data *)(unsigned long)params-hdr.setup_data;
-- 
1.8.1.4

--
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] efi: Check EFI revision in setup_efi_vars

2013-04-24 Thread Josh Boyer
On Wed, Apr 24, 2013 at 03:44:30PM +0100, Matt Fleming wrote:
 On 24/04/13 15:37, Josh Boyer wrote:
  We need to check the runtime sys_table for the EFI version the firmware
  specifies instead of just checking for a NULL QueryVariableInfo.  Older
  implementations of EFI don't have QueryVariableInfo but the runtime is
  a smaller structure, so the pointer to it may be pointing off into garbage.
  
  This is apparently the case with several Apple firmwares that support EFI
  1.10, and the current check causes them to no longer boot.  Fix based on
  a suggestion from Matthew Garrett.
  
  Signed-off-by: Josh Boyer jwbo...@redhat.com
  ---
   arch/x86/boot/compressed/eboot.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)
  
  diff --git a/arch/x86/boot/compressed/eboot.c 
  b/arch/x86/boot/compressed/eboot.c
  index 8615f75..b46efbf 100644
  --- a/arch/x86/boot/compressed/eboot.c
  +++ b/arch/x86/boot/compressed/eboot.c
  @@ -258,7 +258,9 @@ static efi_status_t setup_efi_vars(struct boot_params 
  *params)
  u64 store_size, remaining_size, var_size;
  efi_status_t status;
   
  -   if (!sys_table-runtime-query_variable_info)
  +   if (sys_table-runtime-hdr.revision  EFI_2_00_SYSTEM_TABLE_REVISION)
  +   return EFI_UNSUPPORTED;
  +   else if(!sys_table-runtime-query_variable_info)
  return EFI_UNSUPPORTED;
   
  data = (struct setup_data *)(unsigned long)params-hdr.setup_data;
  
 
 Thanks Josh, that looks correct.
 
 It's a small point, but does the check against NULL actually make sense?
 I don't think we ever check other system table pointers against NULL.

That I'm not sure of.  I was going off of the assumption that Matthew
put it there because someone's EFI 2.0 implementation was crappy and
didn't actually implement it.  So I left that check in place for now.

josh
--
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] Remove warning in efi_enter_virtual_mode

2013-04-19 Thread Josh Boyer
On Fri, Apr 19, 2013 at 08:50:14AM +0100, Matt Fleming wrote:
 On 04/19/2013 01:18 AM, Darren Hart wrote:
  On 04/18/2013 09:19 AM, Matt Fleming wrote:
 
  Could you give it a spin on your MinnowBoard?
  
  I've removed the patch I reference above and applied your patch to my
  3.8.4 MinnowBoard dev tree. It panics with:
 
 D'oh. OK, at this point I'm inclined to apply Josh Boyer's patch on top
 of my urgent branch which will address the WARNING people are hitting on
 i386. I updated the commit message a little.
 
 Josh (Boyer), are you guys still carrying this patch and have you seen
 any fallout? I notice your SoB isn't on the patch that Darren posted, am
 I OK to add it?

Yeah, we're still carrying it.  We've had it around since August of 2011
to cover the bug mentioned.  I'm not aware of any further fallout it
might have caused, but again we don't support 32-bit EFI in Fedora.
That's not to say Fedora won't work, we just don't focus on it.

I don't actually remember authoring the patch, but it's been a while so:

Signed-off-by: Josh Boyer jwbo...@redhat.com

josh
--
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] Remove warning in efi_enter_virtual_mode

2013-04-18 Thread Josh Boyer
On Thu, Apr 18, 2013 at 12:00:26PM +0100, Matt Fleming wrote:
 On 17/04/13 23:00, Bryan O'Donoghue wrote:
  In my mind the only memory that is relevant to efi_enter_virtual_mode is
  memory that the BIOS has marked as EFI_RUNTIME_SERVICE
  
  md-attribute  0x8000_
  
  I couldn't quite understand why the code in
  
  arch/x86/platform/efi/efi.c:enter_virtual_mode() looks like this
  
  for (p = memmap.map; p  memmap.map_end; p += memmap.desc_size) {
  md = p;
  if (!(md-attribute  EFI_MEMORY_RUNTIME) 
  md-type != EFI_BOOT_SERVICES_CODE 
  md-type != EFI_BOOT_SERVICES_DATA)
  continue;
  
  While the code in
  
  arch/ia64/kernel/efi.c:enter_virtual_mode
  
  for (p = efi_map_start; p  efi_map_end; p += efi_desc_size) {
  md = p;
  if (md-attribute  EFI_MEMORY_RUNTIME) {
  
  The ia64 version is consistent with the standard - but obviously isn't
  accounting for the work-around implemented to retrieve the BGRT on ia32.
  
  Looking at the commit message associated with
  arch/x86/platform/efi/efi-bgrt.c
  
  It's pretty obvious the mapping of boot code/data was done to facilitate
  BGRT.
 
 No, that's incorrect. The patch that introduced the mapping of
 EFI_BOOT_SERVICES_{CODE,DATA} was committed before support for bgrt
 existed. git blame is a good tool to use when doing one of these
 historical digs, and in this case it shows that the above lines from
 efi_enter_virtual_mode() were introduced in the following commit,
 
 commit 916f676f8dc016103f983c7ec54c18ecdbb6e349
 Author: Matthew Garrett m...@redhat.com
 Date:   Wed May 25 09:53:13 2011 -0400
 
 x86, efi: Retain boot service code until after switching to virtual mode
 
 UEFI stands for Unified Extensible Firmware Interface, where Firmware
 is an ancient African word meaning Why do something right when you can
 do it so wrong that children will weep and brave adults will cower before
 you, and UEI is Celtic for We missed DOS so we burned it into your
 ROMs. The UEFI specification provides for runtime services (ie, another
 way for the operating system to be forced to depend on the firmware) and
 we rely on these for certain trivial tasks such as setting up the
 bootloader. But some hardware fails to work if we attempt to use these
 runtime services from physical mode, and so we have to switch into virtual
 mode. So far so dreadful.
 
 The specification makes it clear that the operating system is free to do
 whatever it wants with boot services code after ExitBootServices() has 
 been
 called. SetVirtualAddressMap() can't be called until ExitBootServices() 
 has
 been. So, obviously, a whole bunch of EFI implementations call into boot
 services code when we do that. Since we've been charmingly naive and
 trusted that the specification may be somehow relevant to the real world,
 we've already stuffed a picture of a penguin or something in that address
 space. And just to make things more entertaining, we've also marked it
 non-executable.
 
 This patch allocates the boot services regions during EFI init and makes
 sure that they're executable. Then, after SetVirtualAddressMap(), it
 discards them and everyone lives happily ever after. Except for the ones
 who have to work on EFI, who live sad lives haunted by the knowledge that
 someone's eventually going to write yet another firmware specification.
 
 [ hpa: adding this to urgent with a stable tag since it fixes 
 currently-broken
   hardware.  However, I do not know what the dependencies are and so I do
   not know which -stable versions this may be a candidate for. ]
 
 Signed-off-by: Matthew Garrett m...@redhat.com
 Link: 
 http://lkml.kernel.org/r/1306331593-28715-1-git-send-email-...@redhat.com
 Signed-off-by: H. Peter Anvin h...@linux.intel.com
 Cc: Tony Luck tony.l...@intel.com
 Cc: sta...@kernel.org
 
 Yes the bgrt code accesses the Boot Service mappings, but that isn't the
 only reason we want to map those regions.
 
  That's one solution - you'd need to make the i386::efi_ioremap() aware
  of the BGRT work-around.
  
  Presumably this work-around is also required on ia64 too.
 
 No, we've never seen an ia64 firmware implementation with the access
 EFI Boot Services Code/Data after ExitBootServices() bug, and it
 doesn't suffer from the same virtual address space limitations that i386
 does.
  
  No, no - we *don't* have a BGRT object at all.
  
  We have a completely clean memory map - but the BGRT code is causing the
  is_ram() failure.
  
 You assume that mapping of the Boot Services regions is done purely for
 the benefit of pulling out the bgrt image - it's not, see the above
 commit log - and I assumed that you had an ACPI bgrt pointer in your
 memory map, but you don't.
 
 Darren, Josh, have you ever seen an i386 machine with 

Re: [PATCH 05/12] PCI: Require CAP_COMPROMISE_KERNEL for PCI BAR access

2013-03-28 Thread Josh Boyer
On Wed, Mar 27, 2013 at 11:08 AM, Kyle McMartin kmcma...@redhat.com wrote:
 On Wed, Mar 27, 2013 at 11:03:26AM -0400, Josh Boyer wrote:
 On Mon, Mar 18, 2013 at 5:32 PM, Matthew Garrett
 matthew.garr...@nebula.com wrote:
  Any hardware that can potentially generate DMA has to be locked down from
  userspace in order to avoid it being possible for an attacker to cause
  arbitrary kernel behaviour. Default to paranoid - in future we can
  potentially relax this for sufficiently IOMMU-isolated devices.
 
  Signed-off-by: Matthew Garrett matthew.garr...@nebula.com

 As noted here:

 https://bugzilla.redhat.com/show_bug.cgi?id=90

 this breaks pci passthru with QEMU.  The suggestion in the bug is to move
 the check from read/write to open, but sysfs makes that somewhat
 difficult.  The open code is part of the core sysfs functionality shared
 with the majority of sysfs files, so adding a check there would restrict
 things that clearly don't need to be restricted.

 Kyle had the idea to add a cap field to the attribute structure, and do
 a capable check if that is set.  That would allow for a more generic
 usage of capabilities in sysfs code, at the cost of slightly increasing
 the structure size and open path.  That seems somewhat promising if we
 stick with capabilities.

 I would love to just squarely blame capabilities for causing this, but we
 can't just replace it with an efi_enabled(EFI_SECURE_BOOT) check because
 of the sysfs open case.  I'm not sure there are great answers here.


 Yeah, that was something like this (I don't even remember which Fedora
 kernel version this was against.)

Mostly an FYI for the peanut gallery, but we noticed moving the cap check
to open breaks lspci being run by an unprivileged user.  It also doesn't
fix pci passthrough because QEMU opens the PCI resource files by itself
after it's already dropped all caps.

More thinking required.

josh
--
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 05/12] PCI: Require CAP_COMPROMISE_KERNEL for PCI BAR access

2013-03-27 Thread Josh Boyer
On Mon, Mar 18, 2013 at 5:32 PM, Matthew Garrett
matthew.garr...@nebula.com wrote:
 Any hardware that can potentially generate DMA has to be locked down from
 userspace in order to avoid it being possible for an attacker to cause
 arbitrary kernel behaviour. Default to paranoid - in future we can
 potentially relax this for sufficiently IOMMU-isolated devices.

 Signed-off-by: Matthew Garrett matthew.garr...@nebula.com

As noted here:

https://bugzilla.redhat.com/show_bug.cgi?id=90

this breaks pci passthru with QEMU.  The suggestion in the bug is to move
the check from read/write to open, but sysfs makes that somewhat
difficult.  The open code is part of the core sysfs functionality shared
with the majority of sysfs files, so adding a check there would restrict
things that clearly don't need to be restricted.

Kyle had the idea to add a cap field to the attribute structure, and do
a capable check if that is set.  That would allow for a more generic
usage of capabilities in sysfs code, at the cost of slightly increasing
the structure size and open path.  That seems somewhat promising if we
stick with capabilities.

I would love to just squarely blame capabilities for causing this, but we
can't just replace it with an efi_enabled(EFI_SECURE_BOOT) check because
of the sysfs open case.  I'm not sure there are great answers here.

josh
--
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: Lock down MSR writing in secure boot

2013-02-08 Thread Josh Boyer
On Fri, Feb 8, 2013 at 4:07 PM, Matthew Garrett
matthew.garr...@nebula.com wrote:
 On Fri, 2013-02-08 at 13:02 -0800, Kees Cook wrote:

 I don't find it unreasonable to drop all caps and lose access to
 sensitive things. :) That's sort of the point, really. I think a cap
 is the best match. It seems like it should either be a cap or a
 namespace flag, but the latter seems messy.

 Yeah, I think it's an expected outcome, but it means that if (say) qemu
 drops privileges, qemu can no longer access PCI resources - even on
 non-secure boot systems. That breaks existing userspace.

Right.  We've had a few reports in Fedora of things breaking on non-SB
systems because of this.  The qemu one is the latest, but the general
problem is people think dropping all caps blindly is making their apps
safer.  Then they find they can't do things they could do before the new
cap was added.  It's messy.

I've thought of treating CAP_COMPROMISE_KERNEL as a hidden cap, where
only the kernel can grant or drop it.  Peter Jones suggested it might
work to allow it to be dropped iff it is the only cap being changed.
Either way, it's a special cap and I have no idea how acceptable
something like that would be.

Really though, the main issue is that you cannot introduce new caps to
enforce finer grained access without breaking something.

josh
--
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 RFC 0/4] Add firmware signature file check

2012-11-05 Thread Josh Boyer
On Mon, Nov 5, 2012 at 12:18 PM, Takashi Iwai ti...@suse.de wrote:
 Hi,

 this is a patch series to add the support for firmware signature
 check.  At this time, the kernel checks extra signature file (*.sig)
 for each firmware, instead of embedded signature.
 It's just a quick hack using the existing module signing mechanism,
 thus provided only as a proof of concept for now.

 To be noted, it doesn't support the firmwares via udev but only the
 direct loading, and the check for built-in firmware is missing, too.

Just to make sure I'm reading this correctly, it will sign any of the
firwmare files installed directly from the kernel tree if the option is
set.  So for the firmware in the linux-firmware tree we'd need to
either copy that into the kernel tree during build time, or duplicate the
signing bits in the linux-firmware tree itself.  However if we do the
latter, we'd probably need to use the same keys as the per-build kernel
key which means copying keys (ew) or tell the kernel to include a
separate firmware key in the extra list.

I feel like I'm rambling a bit, but I'm trying to work out how signed
firmware would look from a distro perspective.  A significant amount of
work has been done to decouple linux-firmware from the kernel tree and
if signed firmware is used it seems to couple them together one way or
another.  At the moment, using generated per-build keys to come up with
the firmware signatures seems a bit suboptimal in that regard.

josh
--
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