| Message ID | 147977469914.6360.17194649697208113702.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 22, 2016 at 12:31:39AM +0000, David Howells wrote: > Provide the ability to perform mixed-mode runtime service calls for x86 in > the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187 provides Small nit, checkpatch usually complains that this should be written as 12-character SHA-1 followed by the commit subject, i.e. 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services") Other than that LGTM. Same for patch 2 of this series. Thanks, Lukas > the ability to invoke arbitrary boot services. > > Suggested-by: Lukas Wunner <lukas@wunner.de> > Signed-off-by: David Howells <dhowells@redhat.com> > --- > > arch/x86/boot/compressed/eboot.c | 1 + > arch/x86/boot/compressed/head_32.S | 6 +++--- > arch/x86/boot/compressed/head_64.S | 8 ++++---- > arch/x86/include/asm/efi.h | 5 +++++ > 4 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index ff01c8fc76f7..c8c32ebcdfdb 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -32,6 +32,7 @@ static void setup_boot_services##bits(struct efi_config *c) \ > \ > table = (typeof(table))sys_table; \ > \ > + c->runtime_services = table->runtime; \ > c->boot_services = table->boottime; \ > c->text_output = table->con_out; \ > } > diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S > index fd0b6a272dd5..d85b9625e836 100644 > --- a/arch/x86/boot/compressed/head_32.S > +++ b/arch/x86/boot/compressed/head_32.S > @@ -82,7 +82,7 @@ ENTRY(efi_pe_entry) > > /* Relocate efi_config->call() */ > leal efi32_config(%esi), %eax > - add %esi, 32(%eax) > + add %esi, 40(%eax) > pushl %eax > > call make_boot_params > @@ -108,7 +108,7 @@ ENTRY(efi32_stub_entry) > > /* Relocate efi_config->call() */ > leal efi32_config(%esi), %eax > - add %esi, 32(%eax) > + add %esi, 40(%eax) > pushl %eax > 2: > call efi_main > @@ -264,7 +264,7 @@ relocated: > #ifdef CONFIG_EFI_STUB > .data > efi32_config: > - .fill 4,8,0 > + .fill 5,8,0 > .long efi_call_phys > .long 0 > .byte 0 > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S > index efdfba21a5b2..beab8322f72a 100644 > --- a/arch/x86/boot/compressed/head_64.S > +++ b/arch/x86/boot/compressed/head_64.S > @@ -265,7 +265,7 @@ ENTRY(efi_pe_entry) > /* > * Relocate efi_config->call(). > */ > - addq %rbp, efi64_config+32(%rip) > + addq %rbp, efi64_config+40(%rip) > > movq %rax, %rdi > call make_boot_params > @@ -285,7 +285,7 @@ handover_entry: > * Relocate efi_config->call(). > */ > movq efi_config(%rip), %rax > - addq %rbp, 32(%rax) > + addq %rbp, 40(%rax) > 2: > movq efi_config(%rip), %rdi > call efi_main > @@ -457,14 +457,14 @@ efi_config: > #ifdef CONFIG_EFI_MIXED > .global efi32_config > efi32_config: > - .fill 4,8,0 > + .fill 5,8,0 > .quad efi64_thunk > .byte 0 > #endif > > .global efi64_config > efi64_config: > - .fill 4,8,0 > + .fill 5,8,0 > .quad efi_call > .byte 1 > #endif /* CONFIG_EFI_STUB */ > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h > index e99675b9c861..2f77bcefe6b4 100644 > --- a/arch/x86/include/asm/efi.h > +++ b/arch/x86/include/asm/efi.h > @@ -191,6 +191,7 @@ static inline efi_status_t efi_thunk_set_virtual_address_map( > struct efi_config { > u64 image_handle; > u64 table; > + u64 runtime_services; > u64 boot_services; > u64 text_output; > efi_status_t (*call)(unsigned long, ...); > @@ -226,6 +227,10 @@ static inline bool efi_is_64bit(void) > #define __efi_call_early(f, ...) \ > __efi_early()->call((unsigned long)f, __VA_ARGS__); > > +#define efi_call_runtime(f, ...) \ > + __efi_early()->call(efi_table_attr(efi_runtime_services, f, \ > + __efi_early()->runtime_services), __VA_ARGS__) > + > extern bool efi_reboot_required(void); > > #else > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Lukas Wunner <lukas@wunner.de> wrote: > Small nit, checkpatch usually complains that this should be written as > 12-character SHA-1 followed by the commit subject, i.e. > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services") In this case, checkpatch is wrong. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2016-11-22 at 14:17 +0000, David Howells wrote: > Lukas Wunner <lukas@wunner.de> wrote: > > > Small nit, checkpatch usually complains that this should be written as > > 12-character SHA-1 followed by the commit subject, i.e. > > > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services") > > In this case, checkpatch is wrong. Why do you think so? -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Joe Perches <joe@perches.com> wrote: > > > Small nit, checkpatch usually complains that this should be written as > > > 12-character SHA-1 followed by the commit subject, i.e. > > > > > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services") > > > > In this case, checkpatch is wrong. > > Why do you think so? Actually, checkpatch doesn't complain about embedded commit IDs anymore, so in that case, it's just about acceptable. Apart from that, I think we should put in the full SHA-1 commit. The probability of a collision in a 12-digit hex number for the >5,000,000 commits just in Linus's tree is currently at ~4.5% and gradually increasing. Add in all the commits in not-yet-upstreamed trees - which might be another million commits, say - then we're over 6%.. Oh, yes, and speaking of checkpatch, can you make it so that if it sees: commit 12345... Author: foo <foo@bar> Date: blah Subject line Description lines ... ... ... ... Signed-off-by-and-suchline-lines diff ... with the all description indented by 4 spaces, then assume that it's the output of git show and not give the warnings about signed-off-by and other things being indented? David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2016-11-22 at 15:52 +0000, David Howells wrote: > Joe Perches <joe@perches.com> wrote: > > > > > Small nit, checkpatch usually complains that this should be written as > > > > 12-character SHA-1 followed by the commit subject, i.e. > > > > > > > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services") > > > > > > In this case, checkpatch is wrong. > > > > Why do you think so? > > Actually, checkpatch doesn't complain about embedded commit IDs anymore, so in > that case, it's just about acceptable. checkpatch still emits warnings about the format of commmit IDs. What version of checkpatch are yuu using? > Apart from that, I think we should put in the full SHA-1 commit. The > probability of a collision in a 12-digit hex number for the >5,000,000 commits > just in Linus's tree is currently at ~4.5% and gradually increasing. Add in > all the commits in not-yet-upstreamed trees - which might be another million > commits, say - then we're over 6%.. Umm, no, that's not correct. SHA-1 lengths of 12 are unique for quite awhile yet. https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/ Using Linus' tree today, from commit 3b404a519815 the current output of the git-uniq-abbrev script is: $ git-uniq-abbrev 5048673 objects 4: 5048673 / 65536 5: 5007413 / 998721 6: 1312496 / 623343 7: 94487 / 47089 8: 6163 / 3081 9: 416 / 208 10: 28 / 14 11: 4 / 2 12: 0 / 0 d597639e2036f04f0226761e2d818b31f2db7820 d597639e203a100156501df8a0756fd09573e2de ef91b6e893a00d903400f8e1303efc4d52b710af ef91b6e893afc4c4ca488453ea9f19ced5fa5861 > Oh, yes, and speaking of checkpatch, can you make it so that if it sees: > > commit 12345... > Author: foo <foo@bar> > Date: blah > > Subject line > > Description lines > ... > ... > ... > ... > > Signed-off-by-and-suchline-lines > > diff ... > > with the all description indented by 4 spaces, then assume that it's the > output of git show and not give the warnings about signed-off-by and other > things being indented? No. Use --format=email as appropriate instead. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Joe Perches <joe@perches.com> wrote: > Umm, no, that's not correct. > SHA-1 lengths of 12 are unique for quite awhile yet. > > https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/ The article says: 1.9% at 12 which is for 3253824 objects (I get 1.86%). However, that was three years ago, and we now have over five million objects, so the collision possibility is 4.5% now. If we add another 2 million over the next three years, then the probability will be over 8% then. I've attached my spreadsheet for you to have a look at. > No. Use --format=email as appropriate instead. Fix checkpatch. This is an entirely reasonable supposition. David
On Tue, 2016-11-22 at 16:40 +0000, David Howells wrote: > Joe Perches <joe@perches.com> wrote: > > > Umm, no, that's not correct. > > SHA-1 lengths of 12 are unique for quite awhile yet. > > > > https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/ > > The article says: > > 1.9% at 12 > > which is for 3253824 objects (I get 1.86%). > > However, that was three years ago, and we now have over five million objects, > so the collision possibility is 4.5% now. > > If we add another 2 million over the next three years, then the probability > will be over 8% then. > > I've attached my spreadsheet for you to have a look at. > > > No. Use --format=email as appropriate instead. > > Fix checkpatch. This is an entirely reasonable supposition. No. There's nothing to fix there IMO. Of course you are welcome to submit patches. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index ff01c8fc76f7..c8c32ebcdfdb 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -32,6 +32,7 @@ static void setup_boot_services##bits(struct efi_config *c) \ \ table = (typeof(table))sys_table; \ \ + c->runtime_services = table->runtime; \ c->boot_services = table->boottime; \ c->text_output = table->con_out; \ } diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S index fd0b6a272dd5..d85b9625e836 100644 --- a/arch/x86/boot/compressed/head_32.S +++ b/arch/x86/boot/compressed/head_32.S @@ -82,7 +82,7 @@ ENTRY(efi_pe_entry) /* Relocate efi_config->call() */ leal efi32_config(%esi), %eax - add %esi, 32(%eax) + add %esi, 40(%eax) pushl %eax call make_boot_params @@ -108,7 +108,7 @@ ENTRY(efi32_stub_entry) /* Relocate efi_config->call() */ leal efi32_config(%esi), %eax - add %esi, 32(%eax) + add %esi, 40(%eax) pushl %eax 2: call efi_main @@ -264,7 +264,7 @@ relocated: #ifdef CONFIG_EFI_STUB .data efi32_config: - .fill 4,8,0 + .fill 5,8,0 .long efi_call_phys .long 0 .byte 0 diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index efdfba21a5b2..beab8322f72a 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -265,7 +265,7 @@ ENTRY(efi_pe_entry) /* * Relocate efi_config->call(). */ - addq %rbp, efi64_config+32(%rip) + addq %rbp, efi64_config+40(%rip) movq %rax, %rdi call make_boot_params @@ -285,7 +285,7 @@ handover_entry: * Relocate efi_config->call(). */ movq efi_config(%rip), %rax - addq %rbp, 32(%rax) + addq %rbp, 40(%rax) 2: movq efi_config(%rip), %rdi call efi_main @@ -457,14 +457,14 @@ efi_config: #ifdef CONFIG_EFI_MIXED .global efi32_config efi32_config: - .fill 4,8,0 + .fill 5,8,0 .quad efi64_thunk .byte 0 #endif .global efi64_config efi64_config: - .fill 4,8,0 + .fill 5,8,0 .quad efi_call .byte 1 #endif /* CONFIG_EFI_STUB */ diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index e99675b9c861..2f77bcefe6b4 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -191,6 +191,7 @@ static inline efi_status_t efi_thunk_set_virtual_address_map( struct efi_config { u64 image_handle; u64 table; + u64 runtime_services; u64 boot_services; u64 text_output; efi_status_t (*call)(unsigned long, ...); @@ -226,6 +227,10 @@ static inline bool efi_is_64bit(void) #define __efi_call_early(f, ...) \ __efi_early()->call((unsigned long)f, __VA_ARGS__); +#define efi_call_runtime(f, ...) \ + __efi_early()->call(efi_table_attr(efi_runtime_services, f, \ + __efi_early()->runtime_services), __VA_ARGS__) + extern bool efi_reboot_required(void); #else
Provide the ability to perform mixed-mode runtime service calls for x86 in the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187 provides the ability to invoke arbitrary boot services. Suggested-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: David Howells <dhowells@redhat.com> --- arch/x86/boot/compressed/eboot.c | 1 + arch/x86/boot/compressed/head_32.S | 6 +++--- arch/x86/boot/compressed/head_64.S | 8 ++++---- arch/x86/include/asm/efi.h | 5 +++++ 4 files changed, 13 insertions(+), 7 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html