[U-Boot] [PATCH 1/1] efi_loader: HII protocols: fix new_package_list()
In new_package_list() we call new_packagelist() to create a new package list. Next we try to add the packages which fails for form packages. Due to this error we call free_packagelist(). Now in free_packagelist() list_del() is called for an uninitialized field hii->link. This leads to changing random memory addresses.
To solve the problem move the initialization of hii->link to new_packagelist().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- @Takahiro: Please, review the patch. --- lib/efi_loader/efi_hii.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c index d63d2d84184..0ed4b196333 100644 --- a/lib/efi_loader/efi_hii.c +++ b/lib/efi_loader/efi_hii.c @@ -343,6 +343,7 @@ static struct efi_hii_packagelist *new_packagelist(void) struct efi_hii_packagelist *hii;
hii = malloc(sizeof(*hii)); + list_add_tail(&hii->link, &efi_package_lists); hii->max_string_id = 0; INIT_LIST_HEAD(&hii->string_tables); INIT_LIST_HEAD(&hii->guid_list); @@ -465,7 +466,6 @@ new_package_list(const struct efi_hii_database_protocol *this, }
hii->driver_handle = driver_handle; - list_add_tail(&hii->link, &efi_package_lists); *handle = hii;
return EFI_EXIT(EFI_SUCCESS);
On Thu, Feb 28, 2019 at 11:20:34PM +0100, Heinrich Schuchardt wrote:
In new_package_list() we call new_packagelist() to create a new package list. Next we try to add the packages which fails for form packages. Due to this error we call free_packagelist(). Now in free_packagelist() list_del() is called for an uninitialized field hii->link. This leads to changing random memory addresses.
To solve the problem move the initialization of hii->link to new_packagelist().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
@Takahiro: Please, review the patch.
Good catch, thank you.
Reviewed-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_hii.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c index d63d2d84184..0ed4b196333 100644 --- a/lib/efi_loader/efi_hii.c +++ b/lib/efi_loader/efi_hii.c @@ -343,6 +343,7 @@ static struct efi_hii_packagelist *new_packagelist(void) struct efi_hii_packagelist *hii;
hii = malloc(sizeof(*hii));
- list_add_tail(&hii->link, &efi_package_lists); hii->max_string_id = 0; INIT_LIST_HEAD(&hii->string_tables); INIT_LIST_HEAD(&hii->guid_list);
@@ -465,7 +466,6 @@ new_package_list(const struct efi_hii_database_protocol *this, }
hii->driver_handle = driver_handle;
list_add_tail(&hii->link, &efi_package_lists); *handle = hii;
return EFI_EXIT(EFI_SUCCESS);
-- 2.20.1
On 3/1/19 1:54 AM, AKASHI Takahiro wrote:
On Thu, Feb 28, 2019 at 11:20:34PM +0100, Heinrich Schuchardt wrote:
In new_package_list() we call new_packagelist() to create a new package list. Next we try to add the packages which fails for form packages. Due to this error we call free_packagelist(). Now in free_packagelist() list_del() is called for an uninitialized field hii->link. This leads to changing random memory addresses.
To solve the problem move the initialization of hii->link to new_packagelist().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
@Takahiro: Please, review the patch.
Good catch, thank you.
Reviewed-by: AKASHI Takahiro takahiro.akashi@linaro.org
With this patch I no longer experience an error when booting my Odroid C2 via iPXE from an iSCSI disk. So I think in v2019.07 we will can enable the HII protocols by default.
Best regards
Heinrich
On 28.02.19 23:20, Heinrich Schuchardt wrote:
In new_package_list() we call new_packagelist() to create a new package list. Next we try to add the packages which fails for form packages. Due to this error we call free_packagelist(). Now in free_packagelist() list_del() is called for an uninitialized field hii->link. This leads to changing random memory addresses.
To solve the problem move the initialization of hii->link to new_packagelist().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
@Takahiro: Please, review the patch.
lib/efi_loader/efi_hii.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c index d63d2d84184..0ed4b196333 100644 --- a/lib/efi_loader/efi_hii.c +++ b/lib/efi_loader/efi_hii.c @@ -343,6 +343,7 @@ static struct efi_hii_packagelist *new_packagelist(void) struct efi_hii_packagelist *hii;
hii = malloc(sizeof(*hii));
- list_add_tail(&hii->link, &efi_package_lists);
Why in new_packagelist() and not in new_package_list()?
Alex
hii->max_string_id = 0; INIT_LIST_HEAD(&hii->string_tables); INIT_LIST_HEAD(&hii->guid_list); @@ -465,7 +466,6 @@ new_package_list(const struct efi_hii_database_protocol *this, }
hii->driver_handle = driver_handle;
list_add_tail(&hii->link, &efi_package_lists); *handle = hii;
return EFI_EXIT(EFI_SUCCESS);
On 3/2/19 5:32 PM, Alexander Graf wrote:
On 28.02.19 23:20, Heinrich Schuchardt wrote:
In new_package_list() we call new_packagelist() to create a new package list. Next we try to add the packages which fails for form packages. Due to this error we call free_packagelist(). Now in free_packagelist() list_del() is called for an uninitialized field hii->link. This leads to changing random memory addresses.
To solve the problem move the initialization of hii->link to new_packagelist().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
@Takahiro: Please, review the patch.
lib/efi_loader/efi_hii.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c index d63d2d84184..0ed4b196333 100644 --- a/lib/efi_loader/efi_hii.c +++ b/lib/efi_loader/efi_hii.c @@ -343,6 +343,7 @@ static struct efi_hii_packagelist *new_packagelist(void) struct efi_hii_packagelist *hii;
hii = malloc(sizeof(*hii));
- list_add_tail(&hii->link, &efi_package_lists);
Why in new_packagelist() and not in new_package_list()?
The important thing is that we do not call list_del() before list_add_tail().
new_packagelist() is only called by new_package_list(). So you could move any line of new_packagelist() to new_package_list().
But I see new_packagelist as the counterpart of free_packagelist(). So it seems natural to put list_add_tail() into new_packagelist() to match the list_del() in free_packagelist().
Anything specific you dislike about it?
Best regards
Heinrich
Alex
hii->max_string_id = 0; INIT_LIST_HEAD(&hii->string_tables); INIT_LIST_HEAD(&hii->guid_list); @@ -465,7 +466,6 @@ new_package_list(const struct efi_hii_database_protocol *this, }
hii->driver_handle = driver_handle;
list_add_tail(&hii->link, &efi_package_lists); *handle = hii;
return EFI_EXIT(EFI_SUCCESS);
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
participants (4)
-
AKASHI Takahiro -
Alexander Graf -
Heinrich Schuchardt -
Heinrich Schuchardt