Re: [PATCH v2 01/15] x86, kaslr: Use init_size instead of run_size
On Fri, Mar 6, 2015 at 11:56 AM, Kees Cook keesc...@chromium.org wrote:
On Fri, Mar 6, 2015 at 11:28 AM, Yinghai Lu ying...@kernel.org wrote:
Okay, I've proven this to myself now. :) I think it would be valuable
to call out that brk and bss are included in the _end calculation. For
others:
...
So, _end - _text does equal _text + bss offset + bss size + brk size
Thanks! It'll be nice to lose the run_size hack. Adding some
documentation to the code here would help others in the future trying
to find this value, I think. :)
in arch/x86/kernel/vmlinux.lds.S, we have
/* BSS */
. = ALIGN(PAGE_SIZE);
.bss : AT(ADDR(.bss) - LOAD_OFFSET) {
__bss_start = .;
*(.bss..page_aligned)
*(.bss)
. = ALIGN(PAGE_SIZE);
__bss_stop = .;
}
. = ALIGN(PAGE_SIZE);
.brk : AT(ADDR(.brk) - LOAD_OFFSET) {
__brk_base = .;
. += 64 * 1024; /* 64k alignment slop space */
*(.brk_reservation) /* areas brk users have reserved */
__brk_limit = .;
}
_end = .;
so _end already cover bss and brk.
--
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 v2] efi/arm64: use UEFI for system reset and poweroff
If UEFI Runtime Services are available, they are preferred over direct
PSCI calls or other methods to reset the system.
For the reset case, we need to hook into machine_restart(), as the
arm_pm_restart function pointer may be overwritten by modules.
Tested-by: Mark Rutland mark.rutl...@arm.com
Reviewed-by: Mark Rutland mark.rutl...@arm.com
Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
---
arch/arm64/kernel/efi.c | 9 +
arch/arm64/kernel/process.c | 8
2 files changed, 17 insertions(+)
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index b42c7b480e1e..2b8d70164428 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -354,3 +354,12 @@ void efi_virtmap_unload(void)
efi_set_pgd(current-active_mm);
preempt_enable();
}
+
+/*
+ * UpdateCapsule() depends on the system being shutdown via
+ * ResetSystem().
+ */
+bool efi_poweroff_required(void)
+{
+ return efi_enabled(EFI_RUNTIME_SERVICES);
+}
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index fde9923af859..c6b1f3b96f45 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -21,6 +21,7 @@
#include stdarg.h
#include linux/compat.h
+#include linux/efi.h
#include linux/export.h
#include linux/sched.h
#include linux/kernel.h
@@ -150,6 +151,13 @@ void machine_restart(char *cmd)
local_irq_disable();
smp_send_stop();
+ /*
+* UpdateCapsule() depends on the system being reset via
+* ResetSystem().
+*/
+ if (efi_enabled(EFI_RUNTIME_SERVICES))
+ efi_reboot(reboot_mode, NULL);
+
/* Now call the architecture specific reboot code. */
if (arm_pm_restart)
arm_pm_restart(reboot_mode, cmd);
--
1.8.3.2
--
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: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, Mar 06, 2015 at 11:41:57AM +, Kweh, Hock Leong wrote: # cat /any/path/capsule.bin /sys/devices/platform/efi_capsule/capsule_load This is straight-forward and clean. or doing: # echo /any/path/capsule.bin /sys/devices/platform/efi_capsule/capsule_load This is strange and clumsy. So do the first one please. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 04, 2015 at 01:32:53PM -0800, Yinghai Lu wrote:
On Wed, Mar 4, 2015 at 12:00 PM, Ingo Molnar mi...@kernel.org wrote:
It is totally unacceptable that you don't do proper analysis of the
patches you submit, and that you don't bother writing proper, readable
changelogs.
Sorry, please check it again:
Subject: [PATCH v4] x86, kaslr: Get kaslr_enabled back correctly
Subject: x86/kaslr: Access the correct kaslr_enabled variable
commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation)
is using address as value for kaslr_enabled.
commit ... started passing KASLR status to kernel proper.
That will get wrong value back for kaslr_enabled in kernel stage.
1. When kaslr is not enabled at boot/choose_kernel_location, if kaslr_enabled
get set wrongly in setup.c, late in module.c::get_module_load_offset
will return not wanted random module load offset.
That change behavior when HIBERNATION is defined or nokaslr is passed.
2. When kaslr is enabled at boot/choose_kernel_location, if kaslr_enabled
get cleared wrongly in setup.c, late in module.c::get_module_load_offset
will not return wanted random module load offset.
This patch changes the code to use early_memmap and access the value,
and will keep boot and kernel consistent with kaslr.
Replace all that with:
However, the setup_data linked list and thus the element which contains
kaslr_enabled is chained together using physical addresses. At the
time when we access it in the kernel proper, we're already running
with paging enabled and therefore must access it through its virtual
address.
That's it, now how hard was to explain it that way?
-v3: add checking return from early_memmap according to bp.
I guess with bp you mean me? You can call me Boris.
Fixes: f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation)
Cc: Matt Fleming matt.flem...@intel.com
Cc: Borislav Petkov b...@suse.de
Cc: Kees Cook keesc...@chromium.org
Cc: Jiri Kosina jkos...@suse.cz
Acked-by: Jiri Kosina jkos...@suse.cz
Signed-off-by: Yinghai Lu ying...@kernel.org
---
arch/x86/kernel/setup.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
Index: linux-2.6/arch/x86/kernel/setup.c
===
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -429,7 +429,18 @@ static void __init reserve_initrd(void)
static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
{
-kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
+/* kaslr_setup_data is defined in aslr.c */
+unsigned char *data;
+unsigned long offset = sizeof(struct setup_data);
+
+data = early_memremap(pa_data, offset + 1);
+if (!data) {
+kaslr_enabled = true;
+return;
+}
+
+kaslr_enabled = *(data + offset);
+early_memunmap(data, offset + 1);
}
static void __init parse_setup_data(void)
Please use checkpatch before submitting patches:
WARNING: please, no spaces at the start of a line
#71: FILE: arch/x86/kernel/setup.c:433:
+unsigned char *data;$
WARNING: please, no spaces at the start of a line
#72: FILE: arch/x86/kernel/setup.c:434:
+unsigned long offset = sizeof(struct setup_data);$
WARNING: please, no spaces at the start of a line
#74: FILE: arch/x86/kernel/setup.c:436:
+data = early_memremap(pa_data, offset + 1);$
WARNING: please, no spaces at the start of a line
#75: FILE: arch/x86/kernel/setup.c:437:
+if (!data) {$
ERROR: code indent should use tabs where possible
#76: FILE: arch/x86/kernel/setup.c:438:
+kaslr_enabled = true;$
WARNING: please, no spaces at the start of a line
#76: FILE: arch/x86/kernel/setup.c:438:
+kaslr_enabled = true;$
ERROR: code indent should use tabs where possible
#77: FILE: arch/x86/kernel/setup.c:439:
+return;$
WARNING: please, no spaces at the start of a line
#77: FILE: arch/x86/kernel/setup.c:439:
+return;$
WARNING: please, no spaces at the start of a line
#78: FILE: arch/x86/kernel/setup.c:440:
+}$
WARNING: please, no spaces at the start of a line
#80: FILE: arch/x86/kernel/setup.c:442:
+kaslr_enabled = *(data + offset);$
WARNING: please, no spaces at the start of a line
#81: FILE: arch/x86/kernel/setup.c:443:
+early_memunmap(data, offset + 1);$
total: 2 errors, 9 warnings, 19 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
Your patch has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
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] arm64/efi: use UEFI ResetSystem() Runtime Service for system reset
On 5 March 2015 at 15:22, Mark Rutland mark.rutl...@arm.com wrote:
Hi Ard,
On Thu, Mar 05, 2015 at 12:51:11PM +, Ard Biesheuvel wrote:
If UEFI Runtime Services are available, the ResetSystem() service should
be preferred over direct PSCI calls or other methods to reset the system.
The reason is that the UpdateCapsule() UEFI Runtime Service, which is used
to perform firmware updates, relies on this.
Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
---
I sent roughly the same patch ~6 months ago, but at the time, we were still
waiting for the restart notifier call chain patches to land. Since that code
got rejected, I am proposing this again. Note that efi_enabled(x) always
evaluates to 'false' on !CONFIG_EFI.
Also, efi_reboot is a static inline for !CONFIG_EFI, so I can't see any
possibility of a build failure.
This fixes reboot on my Seattle [although it doesn't make it any faster :-)]
arch/arm64/kernel/process.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index fde9923af859..a52bc0c316a8 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -21,6 +21,7 @@
#include stdarg.h
#include linux/compat.h
+#include linux/efi.h
#include linux/export.h
#include linux/sched.h
#include linux/kernel.h
@@ -150,6 +151,14 @@ void machine_restart(char *cmd)
local_irq_disable();
smp_send_stop();
+ /*
+ * Prefer reboot via EFI if available, so that capsule updates [which
+ * rely on UEFI's ResetSystem() being called with the return value of
+ * UpdateCapsule()] have a chance of working as expected.
+ */
+ if (efi_enabled(EFI_RUNTIME_SERVICES))
+ efi_reboot(reboot_mode, NULL);
I expect that the particulars of the UpdateCapsule() will be handled
within efi_reboot and won't require any additions here. So the comment
could just be trimmed to say that UpdateCapsule() depends on the system
being reset with ResetSystem().
OK.
Also, we need to make sure we call efi_poweroff to make UpdateCapsule()
work when shutting the machine down (behind the scenes efi_poweroff
calls ResetSystem(EfiResetShutdown, ...)).
For that I think adding the following to arch/arm64/kernel/efi.c is
sufficient:
/*
* UpdateCapsule() depends on the system being shutdown via
* ResetSystem().
*/
bool efi_poweroff_required(void)
{
return efi_enabled(EFI_RUNTIME_SERVICES);
}
Yes. I used to have this as a separate patch as well ~6 months ago,
but I did not realise at the time that UpdateCapsule() depends on
ResetSystem() for poweroff as well.
I've given the patch a spin (with and without that addition) on Juno and
Seattle. So with that folded in:
Tested-by: Mark Rutland mark.rutl...@arm.com
Reviewed-by: Mark Rutland mark.rutl...@arm.com
Thanks. I will fold it in and resend.
--
Ard.
--
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: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Friday, March 06, 2015 7:09 AM On Mar 5, 2015 1:19 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: This really is not a big deal. User should cope with it. No, it's a big deal, and the user should not cope. The user *should not* be required to have write access to anything in /lib to install a UEFI capsule that they download from their motherboard vendor's website. /lib belongs to the distro, and UEFI capsules do not belong to the distro. In this regard, UEFI capsules are completely unlike your wireless card firmware, your cpu microcode, etc. Imagine systems using NFS root, Atomic-style systems (e.g. ostree), systems that boot off squashfs, etc. They should still be able to load capsules. The basic user interface that should work is: # uefi-load-capsule /path/to/capsule or: # uefi-load-capsule - /path/to/capsule I don't really care how uefi-load-capsule is implemented, as long as it's straightforward, because people will screw it up if it isn't straightforward. Why is it so hard to have a file in sysfs that you write the capsule to using *cat* (not echo) and that will return an error code if cat fails? Is it because you don't know where the end of the capsule is? if so, ioctl is designed for exactly this purpose. TBH, I find this thread kind of ridiculous. The problem that you're trying to solve is extremely simple, the functionality that userspace needs is trivial, and all of these complex proposals for how it should work are an artifact of the fact that the kernel-internal interfaces you're using for it are not well suited to the problem at hand. --Andy Sorry, I may not catch your point correctly. Are you trying to tell that a normal user can perform efi capsule update. But a normal user does not have the right to install or copy the capsule binary into /lib/firmware/. So, there is a need to make this capsule module to allow uploading the capsule binary at any path or location other than /lib/firmware/. Is this what you mean? No. Only root should be able to load capsules, but even root may not be able to write to /lib. --Andy Okay, I accept that only root user can perform the load capsule. It is new to me that root user may not have the access right to /lib/firmware. But I remember you do mention that CPU micro code and wifi firmware they are different and able to write in /lib/firmware. I am curious why efi capsule binary have such a restriction. What is preventing it being write to that location? I even went to check out the FHS: http://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard I do not find any restriction calling out for that. Would you mind to educate me on that? Thanks. Regards, Wilson
Re: [PATCH v2 01/15] x86, kaslr: Use init_size instead of run_size
On Wed, Mar 04, 2015 at 12:00:34AM -0800, Yinghai Lu wrote: commit e6023367d779 (x86, kaslr: Prevent .bss from overlaping initrd) introduced one run_size for kaslr. We do not need to have home grown run_size. We should use real runtime size (include copy/decompress) aka init_size Why? I can see why but you need to explain it here. Also, look at the e6023367d779 commit message. Now this is how you do a commit message! Especially if it is early boot code, you explain stuff. Junjie even went the distance and did a nice graphic. So please redo your commit message. And saying it is trivial and obvious will not get you anywhere, as you've noticed already. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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 02/15] x86, boot: move ZO to end of buffer
On Wed, Mar 04, 2015 at 12:00:35AM -0800, Yinghai Lu wrote: bp found data from boot stage can not be used kernel stage. Actually those data area is overlapped with VO kernel bss stage, and clear_bss() VO kernel bss stage? I'm sure you can think of a better explanation. Right now I'm thinking of Video Only or Virtually Omnipresent or ... I can go all day. Let's be more precise here please. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message-
From: Borislav Petkov [mailto:b...@alien8.de]
Sent: Friday, March 06, 2015 4:14 PM
On Thu, Mar 05, 2015 at 03:08:42PM -0800, Andy Lutomirski wrote:
No. Only root should be able to load capsules, but even root may not
be able to write to /lib.
So basically what we want to do is:
# cat /any/path/to/efi/capsule/accessible/to/root/efi_capsule.img
/sys/firmware/efi/update
Now it can't get any simpler than that and you get error codes too by
failing the cat if the update fails.
Mind you, I'm using '#' and not '$' as a shell prompt :-)
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
I believe you guys are more paying attention to the PATH and
whether doing:
# cat /any/path/capsule.bin /sys/devices/platform/efi_capsule/capsule_load
or doing:
# echo /any/path/capsule.bin /sys/devices/platform/efi_capsule/capsule_load
is not a big issue right?
Regards,
Wilson
N�r��yb�X��ǧv�^�){.n�+{�y^n�r���z���h��� �G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�
Re: [PATCH v2 01/15] x86, kaslr: Use init_size instead of run_size
On Fri, Mar 6, 2015 at 5:55 AM, Borislav Petkov b...@alien8.de wrote: On Wed, Mar 04, 2015 at 12:00:34AM -0800, Yinghai Lu wrote: commit e6023367d779 (x86, kaslr: Prevent .bss from overlaping initrd) introduced one run_size for kaslr. We do not need to have home grown run_size. We should use real runtime size (include copy/decompress) aka init_size Why? New change log: Subject: [PATCH] x86, kaslr: Use init_size instead of run_size commit e6023367d779 (x86, kaslr: Prevent .bss from overlaping initrd) introduced one run_size for kaslr. We should use real runtime size (include copy/decompress) aka init_size. run_size is size of VO (vmlinux). init_size is the size needed for decompress and it is bigger than run_size when decompress need more buff. According to arch/x86/boot/header.S: | #define ZO_INIT_SIZE(ZO__end - ZO_startup_32 + ZO_z_extract_offset) | #define VO_INIT_SIZE(VO__end - VO__text) | #if ZO_INIT_SIZE VO_INIT_SIZE | #define INIT_SIZE ZO_INIT_SIZE | #else | #define INIT_SIZE VO_INIT_SIZE | #endif | init_size: .long INIT_SIZE # kernel initialization size Bootloader allocate buffer according to init_size in hdr, and load the ZO (arch/x86/boot/compressed/vmlinux) from start of that buffer. During running of ZO, ZO move itself to the middle of buffer at z_extract_offset to make sure that decompressor would not have output overwrite input data before input data get consumed. But z_extract_offset calculating is based on size of VO (vmlinux) and size of compressed VO only at first. So need to make [z_extra_offset, init_size) will fit ZO, that means init_size need to be adjusted according to ZO size. That make init_size is always = run_size. During aslr buffer searching, we need to make sure the buffer is bigger enough for decompress at first. So use init_size instead, and kill not needed run_size related code. -- 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/15] x86, kaslr: Use init_size instead of run_size
On Fri, Mar 6, 2015 at 10:44 AM, Yinghai Lu ying...@kernel.org wrote: On Fri, Mar 6, 2015 at 5:55 AM, Borislav Petkov b...@alien8.de wrote: On Wed, Mar 04, 2015 at 12:00:34AM -0800, Yinghai Lu wrote: commit e6023367d779 (x86, kaslr: Prevent .bss from overlaping initrd) introduced one run_size for kaslr. We do not need to have home grown run_size. We should use real runtime size (include copy/decompress) aka init_size Why? New change log: Subject: [PATCH] x86, kaslr: Use init_size instead of run_size commit e6023367d779 (x86, kaslr: Prevent .bss from overlaping initrd) introduced one run_size for kaslr. We should use real runtime size (include copy/decompress) aka init_size. run_size is size of VO (vmlinux). init_size is the size needed for decompress and it is bigger than run_size when decompress need more buff. According to arch/x86/boot/header.S: | #define ZO_INIT_SIZE(ZO__end - ZO_startup_32 + ZO_z_extract_offset) | #define VO_INIT_SIZE(VO__end - VO__text) | #if ZO_INIT_SIZE VO_INIT_SIZE | #define INIT_SIZE ZO_INIT_SIZE | #else | #define INIT_SIZE VO_INIT_SIZE | #endif | init_size: .long INIT_SIZE # kernel initialization size Bootloader allocate buffer according to init_size in hdr, and load the ZO (arch/x86/boot/compressed/vmlinux) from start of that buffer. During running of ZO, ZO move itself to the middle of buffer at z_extract_offset to make sure that decompressor would not have output overwrite input data before input data get consumed. But z_extract_offset calculating is based on size of VO (vmlinux) and size of compressed VO only at first. So need to make [z_extra_offset, init_size) will fit ZO, that means init_size need to be adjusted according to ZO size. That make init_size is always = run_size. During aslr buffer searching, we need to make sure the buffer is bigger enough for decompress at first. So use init_size instead, and kill not needed run_size related code. I don't see how bss and brk are related to these sizes. Can you explain how bss, brk, and initrd factor into these sizes? Those were what run_size was created to represent. I don't want to accidentally start stomping on bss and brk again. :) -Kees -- Kees Cook Chrome OS Security -- 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 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov b...@suse.de wrote:
Please use checkpatch before submitting patches:
WARNING: please, no spaces at the start of a line
#71: FILE: arch/x86/kernel/setup.c:433:
+unsigned char *data;$
WARNING: please, no spaces at the start of a line
#72: FILE: arch/x86/kernel/setup.c:434:
+unsigned long offset = sizeof(struct setup_data);$
WARNING: please, no spaces at the start of a line
#74: FILE: arch/x86/kernel/setup.c:436:
+data = early_memremap(pa_data, offset + 1);$
WARNING: please, no spaces at the start of a line
#75: FILE: arch/x86/kernel/setup.c:437:
+if (!data) {$
ERROR: code indent should use tabs where possible
#76: FILE: arch/x86/kernel/setup.c:438:
+kaslr_enabled = true;$
WARNING: please, no spaces at the start of a line
#76: FILE: arch/x86/kernel/setup.c:438:
+kaslr_enabled = true;$
ERROR: code indent should use tabs where possible
#77: FILE: arch/x86/kernel/setup.c:439:
+return;$
WARNING: please, no spaces at the start of a line
#77: FILE: arch/x86/kernel/setup.c:439:
+return;$
WARNING: please, no spaces at the start of a line
#78: FILE: arch/x86/kernel/setup.c:440:
+}$
WARNING: please, no spaces at the start of a line
#80: FILE: arch/x86/kernel/setup.c:442:
+kaslr_enabled = *(data + offset);$
WARNING: please, no spaces at the start of a line
#81: FILE: arch/x86/kernel/setup.c:443:
+early_memunmap(data, offset + 1);$
total: 2 errors, 9 warnings, 19 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
Your patch has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
That is copy and paste instead of attachment for easy review.
but gmail web client convert tab to spaces.
--
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: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mar 6, 2015 4:20 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: -Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Friday, March 06, 2015 7:09 AM On Mar 5, 2015 1:19 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: This really is not a big deal. User should cope with it. No, it's a big deal, and the user should not cope. The user *should not* be required to have write access to anything in /lib to install a UEFI capsule that they download from their motherboard vendor's website. /lib belongs to the distro, and UEFI capsules do not belong to the distro. In this regard, UEFI capsules are completely unlike your wireless card firmware, your cpu microcode, etc. Imagine systems using NFS root, Atomic-style systems (e.g. ostree), systems that boot off squashfs, etc. They should still be able to load capsules. The basic user interface that should work is: # uefi-load-capsule /path/to/capsule or: # uefi-load-capsule - /path/to/capsule I don't really care how uefi-load-capsule is implemented, as long as it's straightforward, because people will screw it up if it isn't straightforward. Why is it so hard to have a file in sysfs that you write the capsule to using *cat* (not echo) and that will return an error code if cat fails? Is it because you don't know where the end of the capsule is? if so, ioctl is designed for exactly this purpose. TBH, I find this thread kind of ridiculous. The problem that you're trying to solve is extremely simple, the functionality that userspace needs is trivial, and all of these complex proposals for how it should work are an artifact of the fact that the kernel-internal interfaces you're using for it are not well suited to the problem at hand. --Andy Sorry, I may not catch your point correctly. Are you trying to tell that a normal user can perform efi capsule update. But a normal user does not have the right to install or copy the capsule binary into /lib/firmware/. So, there is a need to make this capsule module to allow uploading the capsule binary at any path or location other than /lib/firmware/. Is this what you mean? No. Only root should be able to load capsules, but even root may not be able to write to /lib. --Andy Okay, I accept that only root user can perform the load capsule. It is new to me that root user may not have the access right to /lib/firmware. But I remember you do mention that CPU micro code and wifi firmware they are different and able to write in /lib/firmware. I am curious why efi capsule binary have such a restriction. What is preventing it being write to that location? It has to do with where the firmware comes from. When someone prepares Linux fs image, they put user code, a kernel (usually), all modules that might be needed, and all firmware that's likely to be needed into /. This might come from the distro or a company-wide image. If a normal firmware firmware file needs an update, it's just like updating a driver or a library -- / will be updated by whatever mechanism is in use. Nonvolatile firmware is different. The update isn't needed on subsequent boots, and it could be much less generic than a driver. For example, a capsule could contain a boot splash screen image that says this is computer #27. Updating the system image to do this makes little sense. Instead it'll be a one-time thing done by root. --Andy I even went to check out the FHS: http://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard I do not find any restriction calling out for that. Would you mind to educate me on that? Thanks. Regards, Wilson -- 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/15] x86, kaslr: Use init_size instead of run_size
On Fri, Mar 6, 2015 at 10:55 AM, Kees Cook keesc...@chromium.org wrote: On Fri, Mar 6, 2015 at 10:44 AM, Yinghai Lu ying...@kernel.org wrote: I don't see how bss and brk are related to these sizes. Can you explain how bss, brk, and initrd factor into these sizes? Those were what run_size was created to represent. I don't want to accidentally start stomping on bss and brk again. :) VO (vlinux) init size aka VO_INIT_SIZE already inlude that. Please check update version. commit e6023367d779 (x86, kaslr: Prevent .bss from overlaping initrd) introduced one run_size for kaslr. We should use real runtime size (include copy/decompress) aka init_size. run_size is VO (vmlinux) init size include bss and brk. init_size is the size needed for decompress and it is bigger than run_size when decompress need more buff. According to arch/x86/boot/header.S: | #define ZO_INIT_SIZE(ZO__end - ZO_startup_32 + ZO_z_extract_offset) | #define VO_INIT_SIZE(VO__end - VO__text) | #if ZO_INIT_SIZE VO_INIT_SIZE | #define INIT_SIZE ZO_INIT_SIZE | #else | #define INIT_SIZE VO_INIT_SIZE | #endif | init_size: .long INIT_SIZE # kernel initialization size Bootloader allocate buffer according to init_size in hdr, and load the ZO (arch/x86/boot/compressed/vmlinux) from start of that buffer. init_size first should come from VO (vmlinux) init size. That VO init size is from VO _end to VO _end and include VO bss and brk area. During running of ZO, ZO move itself to the middle of buffer at z_extract_offset to make sure that decompressor would not have output overwrite input data before input data get consumed. But z_extract_offset calculating is based on size of VO (vmlinux) and size of compressed VO only at first. So need to make sure [z_extra_offset, init_size) will fit ZO, that means init_size need to be adjusted according to ZO size. That make init_size is always = run_size. During aslr buffer searching, we need to make sure the buffer is bigger enough for decompress at first. So use init_size instead, and kill not needed run_size related code. -- 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 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 6, 2015 at 11:50 AM, Yinghai Lu ying...@kernel.org wrote: On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov b...@suse.de wrote: However, the setup_data linked list and thus the element which contains kaslr_enabled is chained together using physical addresses. At the time when we access it in the kernel proper, we're already running with paging enabled and therefore must access it through its virtual address. That's it, now how hard was to explain it that way? No, I don't think your change log is right. Actually the old code is using address as value. if the old code would be like: kaslr_enabled = (bool)(*(unsigned char *)(pa_data + sizeof(struct setup_data))); then your change log would be good, but the old code is kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data)); Please check if you are ok with this: Subject: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation) started passing KASLR status to kernel proper, but it uses address as the vaule. That will get wrong value back for kaslr_enabled in kernel stage. 1. When kaslr is not enabled at boot/choose_kernel_location, if kaslr_enabled get set wrongly in setup.c, late in module.c::get_module_load_offset will return not wanted random module load offset. That change behavior when HIBERNATION is defined or nokaslr is passed. 2. When kaslr is enabled at boot/choose_kernel_location, if kaslr_enabled get cleared wrongly in setup.c, late in module.c::get_module_load_offset will not return wanted random module load offset. The setup_data linked list and thus the element which contains kaslr_enabled is chained together using physical addresses. At the time when we access it in the kernel proper, we're already running with paging enabled and therefore must access it through its virtual address. This patch changes the code to use early_memmap and access the value, and will keep boot and kernel consistent with kaslr. -v3: add checking return from early_memmap according to Boris. -- 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 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov b...@suse.de wrote: However, the setup_data linked list and thus the element which contains kaslr_enabled is chained together using physical addresses. At the time when we access it in the kernel proper, we're already running with paging enabled and therefore must access it through its virtual address. That's it, now how hard was to explain it that way? No, I don't think your change log is right. Actually the old code is using address as value. if the old code would be like: kaslr_enabled = (bool)(*(unsigned char *)(pa_data + sizeof(struct setup_data))); then your change log would be good, but the old code is kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data)); -- 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: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: Hi All, After some internal discussion and re-design prototyping testing on this efi capsule interface kernel module, I would like to start a discussion here on the new idea and wish to get input for the implementation and eventually upstream. So do we actually *need* a kernel interface to UpdateCapsule? We've already got an implementation in progress where we use my ESRT patch to figure out what firmwares we can update, and we schedule them and do the update during boot up before the normal bootloader is run. That means never having to call UpdateCapsule() after ExitBootServices() or SetVirtualAddressMap() have been called, and only doing it across reboots that UEFI is performing itself. It also means never having to do it with multiple CPUs running. I've posted several revisions of the ESRT patch to linux-efi , and right now we're just waiting on the UEFI 2.5 spec to be finalized before I send a final copy to Matt. The command line tool and the code to apply the firmwares during boot are at https://github.com/rhinstaller/fwupdate and there's a GNOME-based UI in progress at https://github.com/hughsie/fwupd (yes we apparently stink at naming things.) I'm not sure how making this go through the kernel will make things better - it introduces a lot more things that can go wrong, and the only benefit is one reboot where BootNext is set - which actually is relatively fast even slow-POSTing machines. After that the UpdateCapsule+reboot to apply is *extremely* fast, because applying capsule updates have to happen very very early in the boot-up sequence. (That early processing is /effectively/ a requirement, since it has to happen before the block write locking on the SPI part is done.) So even on the machine that takes quite some time to POST, the time that would be saved saved is less than 1% or so of the total update time. An early version of my code worked to update a machine I got from your employer, FWIW - right now we're adding API and fixing up implementation bits I made specifically to that one proof-of-concept scenario, and there's some API parts that are in UEFI 2.5 draft releases that we're not quite handling yet. However, there's code there that's already been checked in which has successfully applied system firmware updates without having a kernel interface to UpdateCapsule(). So again: do we really need or want to do this? -- Peter -- 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: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, Mar 6, 2015 at 1:39 PM, Peter Jones pjo...@redhat.com wrote: On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: Hi All, After some internal discussion and re-design prototyping testing on this efi capsule interface kernel module, I would like to start a discussion here on the new idea and wish to get input for the implementation and eventually upstream. So do we actually *need* a kernel interface to UpdateCapsule? We've already got an implementation in progress where we use my ESRT patch to figure out what firmwares we can update, and we schedule them and do the update during boot up before the normal bootloader is run. That means never having to call UpdateCapsule() after ExitBootServices() or SetVirtualAddressMap() have been called, and only doing it across reboots that UEFI is performing itself. It also means never having to do it with multiple CPUs running. So this does it by writing the capsules to the EFI system partition, and having them processed from there during the next boot? How would this work on diskless systems? (or boot-disk-less systems) What are the use cases for capsules that don't require a reboot? I'm not really clear uses for those, but the kernel interface could be better for those, as it would allow processing without a reboot. Roy I've posted several revisions of the ESRT patch to linux-efi , and right now we're just waiting on the UEFI 2.5 spec to be finalized before I send a final copy to Matt. The command line tool and the code to apply the firmwares during boot are at https://github.com/rhinstaller/fwupdate and there's a GNOME-based UI in progress at https://github.com/hughsie/fwupd (yes we apparently stink at naming things.) I'm not sure how making this go through the kernel will make things better - it introduces a lot more things that can go wrong, and the only benefit is one reboot where BootNext is set - which actually is relatively fast even slow-POSTing machines. After that the UpdateCapsule+reboot to apply is *extremely* fast, because applying capsule updates have to happen very very early in the boot-up sequence. (That early processing is /effectively/ a requirement, since it has to happen before the block write locking on the SPI part is done.) So even on the machine that takes quite some time to POST, the time that would be saved saved is less than 1% or so of the total update time. An early version of my code worked to update a machine I got from your employer, FWIW - right now we're adding API and fixing up implementation bits I made specifically to that one proof-of-concept scenario, and there's some API parts that are in UEFI 2.5 draft releases that we're not quite handling yet. However, there's code there that's already been checked in which has successfully applied system firmware updates without having a kernel interface to UpdateCapsule(). So again: do we really need or want to do this? -- Peter -- 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 -- 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: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, Mar 06, 2015 at 01:49:20PM -0800, Roy Franz wrote: On Fri, Mar 6, 2015 at 1:39 PM, Peter Jones pjo...@redhat.com wrote: On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: Hi All, After some internal discussion and re-design prototyping testing on this efi capsule interface kernel module, I would like to start a discussion here on the new idea and wish to get input for the implementation and eventually upstream. So do we actually *need* a kernel interface to UpdateCapsule? We've already got an implementation in progress where we use my ESRT patch to figure out what firmwares we can update, and we schedule them and do the update during boot up before the normal bootloader is run. That means never having to call UpdateCapsule() after ExitBootServices() or SetVirtualAddressMap() have been called, and only doing it across reboots that UEFI is performing itself. It also means never having to do it with multiple CPUs running. So this does it by writing the capsules to the EFI system partition, and having them processed from there during the next boot? How would this work on diskless systems? (or boot-disk-less systems) One of the things I'm currently writing is making the file we load be specified correctly by UEFI device paths - and that'll include the ability to get it from devices presented by the network drivers. On currently extant test machines that includes tftp support, just like netbooting. On UEFI 2.5 machines, where ESRT is introduced so that we actually know something about the capsules we can apply updates for, it also includes http support. Admittedly that means when you're doing deployment you'll have to have some process for putting the images someplace, but it can be the same device you're net-booting from. Just one example. If we're talking about systems that are booting entirely out of their own firmware volume, there's definitely some legitimate argument that doing this without an ESP could be valuable - so yeah, a kernel API in that case might be worthwhile. That said, the extra complexity combined with the fact that it's running after ExitBootServices() and SetVirtualAddressMap() means I'll probably avoid using it from the userland code except on machines where there are no other options. My faith that we're going to see a lot of machines where that's well tested without our address maps looking exactly like Windows's isn't very high, and we've repeatedly run into a lot of pain with that in the past. That's not the only thing that has to be exactly right, either - for instance there's no guarantee it'll work if you use the ACPI reset vector instead of the EFI runtime services ResetSystem(EfiResetWarm) call. Right now we use the ACPI method preferentially because of SetVirtualAddressMap() not working as documented on so *many* platforms. That means what happens upon reboot when we do UpdateCapsule() is undefined behavior. Of course I'm likely not the only person who will ever implement tools in userland to orchestrate firmware updates, either :) What are the use cases for capsules that don't require a reboot? I'm not really clear uses for those, but the kernel interface could be better for those, as it would allow processing without a reboot. I'm really not sure if we know the answer to that yet. Things like USB devices most often won't ever be registered with firmware's FMP anyway, so they won't have capsules exposed, and they'll use more traditional USB commands to do firmware updates - stuff like hughsie's ColorHug device are in that category, and he's already supporting that with fwupd. Things like peripheral card option ROMs are potentially in that category, though I'm a little skeptical of the idea that I actually want to update the firmware on my video or network card with the kernel already running, and that when I do I'm not going to want to reboot immediately to make sure it worked right anyway. There are almost certainly lots of cases I haven't thought of, though. If we want this interface for those cases, I think we should also be enumerating the cases we think it's supporting, as well, even if only in broad strokes. I don't think we need to say support this specific device's updates, but keeping track of the basic model of the update we're supporting would go a long way to establishing the value of supporting all the complexity. -- Peter -- 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: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Thu, Mar 05, 2015 at 03:08:42PM -0800, Andy Lutomirski wrote: No. Only root should be able to load capsules, but even root may not be able to write to /lib. So basically what we want to do is: # cat /any/path/to/efi/capsule/accessible/to/root/efi_capsule.img /sys/firmware/efi/update Now it can't get any simpler than that and you get error codes too by failing the cat if the update fails. Mind you, I'm using '#' and not '$' as a shell prompt :-) -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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
