* [U-Boot] [PATCH 1/1] efi_loader: HII protocols: fix new_package_list()
@ 2019-02-28 22:20 Heinrich Schuchardt
2019-03-01 0:54 ` AKASHI Takahiro
2019-03-02 16:32 ` Alexander Graf
0 siblings, 2 replies; 5+ messages in thread
From: Heinrich Schuchardt @ 2019-02-28 22:20 UTC (permalink / raw)
To: u-boot
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);
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH 1/1] efi_loader: HII protocols: fix new_package_list()
2019-02-28 22:20 [U-Boot] [PATCH 1/1] efi_loader: HII protocols: fix new_package_list() Heinrich Schuchardt
@ 2019-03-01 0:54 ` AKASHI Takahiro
2019-03-02 21:47 ` Heinrich Schuchardt
2019-03-02 16:32 ` Alexander Graf
1 sibling, 1 reply; 5+ messages in thread
From: AKASHI Takahiro @ 2019-03-01 0:54 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH 1/1] efi_loader: HII protocols: fix new_package_list()
2019-02-28 22:20 [U-Boot] [PATCH 1/1] efi_loader: HII protocols: fix new_package_list() Heinrich Schuchardt
2019-03-01 0:54 ` AKASHI Takahiro
@ 2019-03-02 16:32 ` Alexander Graf
2019-03-02 22:08 ` Heinrich Schuchardt
1 sibling, 1 reply; 5+ messages in thread
From: Alexander Graf @ 2019-03-02 16:32 UTC (permalink / raw)
To: u-boot
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);
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH 1/1] efi_loader: HII protocols: fix new_package_list()
2019-03-01 0:54 ` AKASHI Takahiro
@ 2019-03-02 21:47 ` Heinrich Schuchardt
0 siblings, 0 replies; 5+ messages in thread
From: Heinrich Schuchardt @ 2019-03-02 21:47 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH 1/1] efi_loader: HII protocols: fix new_package_list()
2019-03-02 16:32 ` Alexander Graf
@ 2019-03-02 22:08 ` Heinrich Schuchardt
0 siblings, 0 replies; 5+ messages in thread
From: Heinrich Schuchardt @ 2019-03-02 22:08 UTC (permalink / raw)
To: u-boot
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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-02 22:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-28 22:20 [U-Boot] [PATCH 1/1] efi_loader: HII protocols: fix new_package_list() Heinrich Schuchardt
2019-03-01 0:54 ` AKASHI Takahiro
2019-03-02 21:47 ` Heinrich Schuchardt
2019-03-02 16:32 ` Alexander Graf
2019-03-02 22:08 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.