Path: csiph.com!news.mixmin.net!aioe.org!bofh.it!news.nic.it!robomod From: Colin Ian King Newsgroups: linux.kernel Subject: Re: [PATCH] x86/efi: initialize status to ensure garbage is not returned on small size Date: Wed, 27 Jul 2016 17:00:01 +0200 Message-ID: References: X-Original-To: Matt Fleming User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: robomod@news.nic.it List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Approved: robomod@news.nic.it Lines: 59 Organization: linux.* mail to news gateway X-Original-Cc: "H . Peter Anvin" , Thomas Gleixner , Ingo Molnar , x86@kernel.org, linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org X-Original-Date: Wed, 27 Jul 2016 15:50:16 +0100 X-Original-Message-ID: X-Original-References: <1469009466-19980-1-git-send-email-colin.king@canonical.com> <20160727143813.GG31759@codeblueprint.co.uk> X-Original-Sender: linux-kernel-owner@vger.kernel.org Xref: csiph.com linux.kernel:1451322 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 >> >> 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 >> --- >> 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