| Message ID | 1411534992-27443-14-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Sep 24, 2014 at 2:07 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 24.09.14 at 07:03, <roy.franz@linaro.org> wrote: >> The only valid response from the LocateHandle() call is EFI_BUFFER_TOO_SMALL, >> so exit if we get anything else. We pass a 0 size/NULL pointer buffer, so the >> only other returns we will get is an error. Return right away as there is >> nothing to do. Also return if there is an error allocating the buffer, as the >> previous code path also allowed for an undefined pointer to be freed. >> >> Signed-off-by: Roy Franz <roy.franz@linaro.org> > > Thanks, but I restructured the patch (see below). Additionally such > bug fixes would better be placed at the start of a series to ease > backporting. > > Jan > > x86/EFI: fix freeing of uninitialized pointer > > The only valid response from the LocateHandle() call is EFI_BUFFER_TOO_SMALL, > so exit if we get anything else. We pass a 0 size/NULL pointer buffer, so the > only other returns we will get is an error. Return right away as there is > nothing to do. Also return if there is an error allocating the buffer, as the > previous code path also allowed for an undefined pointer to be freed. > > Signed-off-by: Roy Franz <roy.franz@linaro.org> > > Re-structure the change. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/efi/boot.c > +++ b/xen/arch/x86/efi/boot.c > @@ -595,11 +595,12 @@ static void __init setup_efi_pci(void) > struct efi_pci_rom *last = NULL; > > status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, NULL); > - if ( status == EFI_BUFFER_TOO_SMALL ) > - status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles); > - if ( !EFI_ERROR(status) ) > - status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, > - handles); > + if ( status != EFI_BUFFER_TOO_SMALL ) > + return; > + status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles); > + if ( EFI_ERROR(status) ) > + return; > + status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, handles); > if ( EFI_ERROR(status) ) > size = 0; > > > > OK, I'll use your version, and move it to the start of the patch series. Roy
>>> On 24.09.14 at 21:22, <roy.franz@linaro.org> wrote: > On Wed, Sep 24, 2014 at 2:07 AM, Jan Beulich <JBeulich@suse.com> wrote: >> --- a/xen/arch/x86/efi/boot.c >> +++ b/xen/arch/x86/efi/boot.c >> @@ -595,11 +595,12 @@ static void __init setup_efi_pci(void) >> struct efi_pci_rom *last = NULL; >> >> status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, NULL); >> - if ( status == EFI_BUFFER_TOO_SMALL ) >> - status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles); >> - if ( !EFI_ERROR(status) ) >> - status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, >> - handles); >> + if ( status != EFI_BUFFER_TOO_SMALL ) >> + return; >> + status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles); >> + if ( EFI_ERROR(status) ) >> + return; >> + status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, handles); >> if ( EFI_ERROR(status) ) >> size = 0; >> > OK, I'll use your version, and move it to the start of the patch series. Did you overlook that I committed it already? Jan
On Thu, Sep 25, 2014 at 1:13 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 24.09.14 at 21:22, <roy.franz@linaro.org> wrote: >> On Wed, Sep 24, 2014 at 2:07 AM, Jan Beulich <JBeulich@suse.com> wrote: >>> --- a/xen/arch/x86/efi/boot.c >>> +++ b/xen/arch/x86/efi/boot.c >>> @@ -595,11 +595,12 @@ static void __init setup_efi_pci(void) >>> struct efi_pci_rom *last = NULL; >>> >>> status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, NULL); >>> - if ( status == EFI_BUFFER_TOO_SMALL ) >>> - status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles); >>> - if ( !EFI_ERROR(status) ) >>> - status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, >>> - handles); >>> + if ( status != EFI_BUFFER_TOO_SMALL ) >>> + return; >>> + status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles); >>> + if ( EFI_ERROR(status) ) >>> + return; >>> + status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, handles); >>> if ( EFI_ERROR(status) ) >>> size = 0; >>> >> OK, I'll use your version, and move it to the start of the patch series. > > Did you overlook that I committed it already? > > Jan > I sure did..
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index d5c9355..54db5c9 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -582,9 +582,15 @@ static void __init setup_efi_pci(void) status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, NULL); if ( status == EFI_BUFFER_TOO_SMALL ) status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles); + else + return; + if ( !EFI_ERROR(status) ) status = efi_bs->LocateHandle(ByProtocol, &pci_guid, NULL, &size, handles); + else + return; + if ( EFI_ERROR(status) ) size = 0;
The only valid response from the LocateHandle() call is EFI_BUFFER_TOO_SMALL, so exit if we get anything else. We pass a 0 size/NULL pointer buffer, so the only other returns we will get is an error. Return right away as there is nothing to do. Also return if there is an error allocating the buffer, as the previous code path also allowed for an undefined pointer to be freed. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- xen/common/efi/boot.c | 6 ++++++ 1 file changed, 6 insertions(+)