* [PATCH] x86/efi: initialize status to ensure garbage is not returned on small size @ 2016-07-20 10:11 Colin King 2016-07-27 14:38 ` Matt Fleming 0 siblings, 1 reply; 4+ messages in thread From: Colin King @ 2016-07-20 10:11 UTC (permalink / raw) To: Matt Fleming, H . Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-efi Cc: linux-kernel From: Colin Ian King <colin.king@canonical.com> Although very unlikey, if size is too small or zero, then we end up with status not being set and returning garbage. Instead, initializing status to EFI_INVALID_PARAMETER to indicate that size is invalid in the calls to setup_uga32 and setup_uga64. Signed-off-by: Colin Ian King <colin.king@canonical.com> --- arch/x86/boot/compressed/eboot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index ff574da..ec6d2ef 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -578,7 +578,7 @@ setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height) efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID; unsigned long nr_ugas; u32 *handles = (u32 *)uga_handle;; - efi_status_t status; + efi_status_t status = EFI_INVALID_PARAMETER; int i; first_uga = NULL; @@ -623,7 +623,7 @@ setup_uga64(void **uga_handle, unsigned long size, u32 *width, u32 *height) efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID; unsigned long nr_ugas; u64 *handles = (u64 *)uga_handle;; - efi_status_t status; + efi_status_t status = EFI_INVALID_PARAMETER; int i; first_uga = NULL; -- 2.8.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/efi: initialize status to ensure garbage is not returned on small size 2016-07-20 10:11 [PATCH] x86/efi: initialize status to ensure garbage is not returned on small size Colin King @ 2016-07-27 14:38 ` Matt Fleming 2016-07-27 14:50 ` Colin Ian King 0 siblings, 1 reply; 4+ messages in thread From: Matt Fleming @ 2016-07-27 14:38 UTC (permalink / raw) To: Colin King Cc: H . Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-efi, linux-kernel On Wed, 20 Jul, at 11:11:06AM, Colin Ian King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Although very unlikey, if size is too small or zero, then we end up with > status not being set and returning garbage. Instead, initializing status to > EFI_INVALID_PARAMETER to indicate that size is invalid in the calls to > setup_uga32 and setup_uga64. > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > arch/x86/boot/compressed/eboot.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index ff574da..ec6d2ef 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -578,7 +578,7 @@ setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height) > efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID; > unsigned long nr_ugas; > u32 *handles = (u32 *)uga_handle;; > - efi_status_t status; > + efi_status_t status = EFI_INVALID_PARAMETER; > int i; > > first_uga = NULL; > @@ -623,7 +623,7 @@ setup_uga64(void **uga_handle, unsigned long size, u32 *width, u32 *height) > efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID; > unsigned long nr_ugas; > u64 *handles = (u64 *)uga_handle;; > - efi_status_t status; > + efi_status_t status = EFI_INVALID_PARAMETER; > int i; > > first_uga = NULL; Can this ever happen in practice? This would imply that locate_protocol() found EFI_UGA_PROTOCOL_GUID but that the size returned is utterly bogus? If so, I have no problem applying the patch but want to make sure we're not tricking ourselves into thinking we're being protected from something when we're not. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/efi: initialize status to ensure garbage is not returned on small size 2016-07-27 14:38 ` Matt Fleming @ 2016-07-27 14:50 ` Colin Ian King 2016-08-01 11:50 ` Matt Fleming 0 siblings, 1 reply; 4+ messages in thread From: Colin Ian King @ 2016-07-27 14:50 UTC (permalink / raw) To: Matt Fleming Cc: H . Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-efi, linux-kernel On 27/07/16 15:38, Matt Fleming wrote: > On Wed, 20 Jul, at 11:11:06AM, Colin Ian King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> Although very unlikey, if size is too small or zero, then we end up with >> status not being set and returning garbage. Instead, initializing status to >> EFI_INVALID_PARAMETER to indicate that size is invalid in the calls to >> setup_uga32 and setup_uga64. >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> arch/x86/boot/compressed/eboot.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c >> index ff574da..ec6d2ef 100644 >> --- a/arch/x86/boot/compressed/eboot.c >> +++ b/arch/x86/boot/compressed/eboot.c >> @@ -578,7 +578,7 @@ setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height) >> efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID; >> unsigned long nr_ugas; >> u32 *handles = (u32 *)uga_handle;; >> - efi_status_t status; >> + efi_status_t status = EFI_INVALID_PARAMETER; >> int i; >> >> first_uga = NULL; >> @@ -623,7 +623,7 @@ setup_uga64(void **uga_handle, unsigned long size, u32 *width, u32 *height) >> efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID; >> unsigned long nr_ugas; >> u64 *handles = (u64 *)uga_handle;; >> - efi_status_t status; >> + efi_status_t status = EFI_INVALID_PARAMETER; >> int i; >> >> first_uga = NULL; > > Can this ever happen in practice? This would imply that > locate_protocol() found EFI_UGA_PROTOCOL_GUID but that the size > returned is utterly bogus? I just wanted to guard against the call efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL, &uga_proto, NULL, &size, uga_handle) returning a bogus size of less than a u32/u64 (depending on the setup_uga32/setup_uga64), which I was not 100% sure if this could happen or not, so I always assume efi_call_early calls can go wrong when you least expect them. > > If so, I have no problem applying the patch but want to make sure > we're not tricking ourselves into thinking we're being protected from > something when we're not. > I'd rather put extra guarding in rather than getting some potential garbage return from the stack, but I am playing it rather conservatively here. Colin ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/efi: initialize status to ensure garbage is not returned on small size 2016-07-27 14:50 ` Colin Ian King @ 2016-08-01 11:50 ` Matt Fleming 0 siblings, 0 replies; 4+ messages in thread From: Matt Fleming @ 2016-08-01 11:50 UTC (permalink / raw) To: Colin Ian King Cc: H . Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-efi, linux-kernel On Wed, 27 Jul, at 03:50:16PM, Colin Ian King wrote: > > I'd rather put extra guarding in rather than getting some potential > garbage return from the stack, but I am playing it rather conservatively > here. I think that's sensible, I just wanted to confirm your rationale. Thanks, applied. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-08-01 11:50 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-20 10:11 [PATCH] x86/efi: initialize status to ensure garbage is not returned on small size Colin King 2016-07-27 14:38 ` Matt Fleming 2016-07-27 14:50 ` Colin Ian King 2016-08-01 11:50 ` Matt Fleming
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox