Patchwork [U-Boot] disk: part_efi: fix regression due to incorrect buffer usage

login
register
mail settings
Submitter Stephen Warren
Date Oct. 28, 2011, 7:21 p.m.
Message ID <1319829706-6459-1-git-send-email-swarren@nvidia.com>
Download mbox | patch
Permalink /patch/122485/
State Accepted
Commit 4715a81136049167160230efd28eb9d48eeded1f
Delegated to: Stefano Babic
Headers show

Comments

Stephen Warren - Oct. 28, 2011, 7:21 p.m.
Commit deb5ca80275e8cfa74d5680b41204e08a095eca5 "disk: part_efi: fix
**pgpt_pte == NULL" modified the code to pass "&gpt_head" to
is_gpt_valid() rather than the previous "gpt_head". However, gpt_head
is a pointer to the buffer, not the actual buffer, since it was allocated
using ALLOC_CACHE_ALIGN_BUFFER. This caused is_gpt_valid() to read the
disk block onto the stack rather than into the buffer, causing the
code to fail.

This change reverts that portion of the commit mentioned above.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 disk/part_efi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Doug Anderson - Oct. 28, 2011, 7:43 p.m.
Acked-by: Doug Anderson <dianders@chromium.org>

...to be fair though, the regression appears to be caused by a mid-air
collision of Anton's change (f75dd584cdfe29dfdcfd424bb237b9238cfb8fe4) with
my change.  His patch was committed on Oct 25th (though authored earlier).
 When I submitted my patch on the Oct 19th against denx/master, I'm pretty
sure it was correct.

Thank you for finding / fixing!  :)

-Doug

---

On Fri, Oct 28, 2011 at 12:21 PM, Stephen Warren <swarren@nvidia.com> wrote:

> Commit deb5ca80275e8cfa74d5680b41204e08a095eca5 "disk: part_efi: fix
> **pgpt_pte == NULL" modified the code to pass "&gpt_head" to
> is_gpt_valid() rather than the previous "gpt_head". However, gpt_head
> is a pointer to the buffer, not the actual buffer, since it was allocated
> using ALLOC_CACHE_ALIGN_BUFFER. This caused is_gpt_valid() to read the
> disk block onto the stack rather than into the buffer, causing the
> code to fail.
>
> This change reverts that portion of the commit mentioned above.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  disk/part_efi.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index e7f2714..ddf80a7 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -130,7 +130,7 @@ void print_part_efi(block_dev_desc_t * dev_desc)
>        }
>        /* This function validates AND fills in the GPT header and PTE */
>        if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
> -                        &(gpt_head), &gpt_pte) != 1) {
> +                        gpt_head, &gpt_pte) != 1) {
>                printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>                return;
>        }
> @@ -169,7 +169,7 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc,
> int part,
>
>        /* This function validates AND fills in the GPT header and PTE */
>        if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
> -                       &(gpt_head), &gpt_pte) != 1) {
> +                       gpt_head, &gpt_pte) != 1) {
>                printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>                return -1;
>        }
> --
> 1.7.0.4
>
>
Anton staaf - Oct. 28, 2011, 10:47 p.m.
Thanks Stephen, sorry Doug.  :)

-Anton

On Fri, Oct 28, 2011 at 12:43 PM, Doug Anderson <dianders@chromium.org> wrote:
> Acked-by: Doug Anderson <dianders@chromium.org>
>
> ...to be fair though, the regression appears to be caused by a mid-air
> collision of Anton's change (f75dd584cdfe29dfdcfd424bb237b9238cfb8fe4) with
> my change.  His patch was committed on Oct 25th (though authored earlier).
>  When I submitted my patch on the Oct 19th against denx/master, I'm pretty
> sure it was correct.
>
> Thank you for finding / fixing!  :)
>
> -Doug
>
> ---
>
> On Fri, Oct 28, 2011 at 12:21 PM, Stephen Warren <swarren@nvidia.com> wrote:
>
>> Commit deb5ca80275e8cfa74d5680b41204e08a095eca5 "disk: part_efi: fix
>> **pgpt_pte == NULL" modified the code to pass "&gpt_head" to
>> is_gpt_valid() rather than the previous "gpt_head". However, gpt_head
>> is a pointer to the buffer, not the actual buffer, since it was allocated
>> using ALLOC_CACHE_ALIGN_BUFFER. This caused is_gpt_valid() to read the
>> disk block onto the stack rather than into the buffer, causing the
>> code to fail.
>>
>> This change reverts that portion of the commit mentioned above.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>>  disk/part_efi.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>> index e7f2714..ddf80a7 100644
>> --- a/disk/part_efi.c
>> +++ b/disk/part_efi.c
>> @@ -130,7 +130,7 @@ void print_part_efi(block_dev_desc_t * dev_desc)
>>        }
>>        /* This function validates AND fills in the GPT header and PTE */
>>        if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
>> -                        &(gpt_head), &gpt_pte) != 1) {
>> +                        gpt_head, &gpt_pte) != 1) {
>>                printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>>                return;
>>        }
>> @@ -169,7 +169,7 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc,
>> int part,
>>
>>        /* This function validates AND fills in the GPT header and PTE */
>>        if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
>> -                       &(gpt_head), &gpt_pte) != 1) {
>> +                       gpt_head, &gpt_pte) != 1) {
>>                printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>>                return -1;
>>        }
>> --
>> 1.7.0.4
>>
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>
Simon Glass - Oct. 29, 2011, 12:04 a.m.
On Fri, Oct 28, 2011 at 3:47 PM, Anton Staaf <robotboy@chromium.org> wrote:
> Thanks Stephen, sorry Doug.  :)
>
> -Anton
>
> On Fri, Oct 28, 2011 at 12:43 PM, Doug Anderson <dianders@chromium.org> wrote:
>> Acked-by: Doug Anderson <dianders@chromium.org>

Tested-by: Simon Glass <sjg@chromium.org>

>>
>> ...to be fair though, the regression appears to be caused by a mid-air
>> collision of Anton's change (f75dd584cdfe29dfdcfd424bb237b9238cfb8fe4) with
>> my change.  His patch was committed on Oct 25th (though authored earlier).
>>  When I submitted my patch on the Oct 19th against denx/master, I'm pretty
>> sure it was correct.
>>
>> Thank you for finding / fixing!  :)
>>
>> -Doug
>>
>> ---
>>
>> On Fri, Oct 28, 2011 at 12:21 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>
>>> Commit deb5ca80275e8cfa74d5680b41204e08a095eca5 "disk: part_efi: fix
>>> **pgpt_pte == NULL" modified the code to pass "&gpt_head" to
>>> is_gpt_valid() rather than the previous "gpt_head". However, gpt_head
>>> is a pointer to the buffer, not the actual buffer, since it was allocated
>>> using ALLOC_CACHE_ALIGN_BUFFER. This caused is_gpt_valid() to read the
>>> disk block onto the stack rather than into the buffer, causing the
>>> code to fail.
>>>
>>> This change reverts that portion of the commit mentioned above.
>>>
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>> ---
>>>  disk/part_efi.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>>> index e7f2714..ddf80a7 100644
>>> --- a/disk/part_efi.c
>>> +++ b/disk/part_efi.c
>>> @@ -130,7 +130,7 @@ void print_part_efi(block_dev_desc_t * dev_desc)
>>>        }
>>>        /* This function validates AND fills in the GPT header and PTE */
>>>        if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
>>> -                        &(gpt_head), &gpt_pte) != 1) {
>>> +                        gpt_head, &gpt_pte) != 1) {
>>>                printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>>>                return;
>>>        }
>>> @@ -169,7 +169,7 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc,
>>> int part,
>>>
>>>        /* This function validates AND fills in the GPT header and PTE */
>>>        if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
>>> -                       &(gpt_head), &gpt_pte) != 1) {
>>> +                       gpt_head, &gpt_pte) != 1) {
>>>                printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>>>                return -1;
>>>        }
>>> --
>>> 1.7.0.4
>>>
>>>
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Mike Frysinger - Oct. 30, 2011, 3:44 p.m.
Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
Anton staaf - Nov. 15, 2011, 6:25 p.m.
Tested-by: Anton Staaf <robotboy@chromium.org>
Mike, do you think you could pull this into staging as per Wolfgang's
recent request for help?

Thanks,
    Anton

On Sun, Oct 30, 2011 at 8:44 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> Acked-by: Mike Frysinger <vapier@gentoo.org>
> -mike
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>
Simon Glass - Nov. 22, 2011, 10:10 p.m.
On Tue, Nov 15, 2011 at 10:25 AM, Anton Staaf <robotboy@chromium.org> wrote:
> Tested-by: Anton Staaf <robotboy@chromium.org>
> Mike, do you think you could pull this into staging as per Wolfgang's
> recent request for help?
>
> Thanks,
>    Anton
>

Please can one of the maintainers grab this and put it in staging?

Regards,
Simon

> On Sun, Oct 30, 2011 at 8:44 AM, Mike Frysinger <vapier@gentoo.org> wrote:
>> Acked-by: Mike Frysinger <vapier@gentoo.org>
>> -mike
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Anatolij Gustschin - Nov. 22, 2011, 10:38 p.m.
Hi Simon,

On Tue, 22 Nov 2011 14:10:12 -0800
Simon Glass <sjg@chromium.org> wrote:

> On Tue, Nov 15, 2011 at 10:25 AM, Anton Staaf <robotboy@chromium.org> wrote:
> > Tested-by: Anton Staaf <robotboy@chromium.org>
> > Mike, do you think you could pull this into staging as per Wolfgang's
> > recent request for help?
> >
> > Thanks,
> >    Anton
> >
> 
> Please can one of the maintainers grab this and put it in staging?

I've seen similar patch, it is already in arm tree:

http://git.denx.de/?p=u-boot/u-boot-arm.git;a=commit;h=81432275812e7d9b3b5eec9c54c6c58dfe506759

Anatolij
Simon Glass - Nov. 22, 2011, 10:42 p.m.
On Tue, Nov 22, 2011 at 2:38 PM, Anatolij Gustschin <agust@denx.de> wrote:
> Hi Simon,
>
> On Tue, 22 Nov 2011 14:10:12 -0800
> Simon Glass <sjg@chromium.org> wrote:
>
>> On Tue, Nov 15, 2011 at 10:25 AM, Anton Staaf <robotboy@chromium.org> wrote:
>> > Tested-by: Anton Staaf <robotboy@chromium.org>
>> > Mike, do you think you could pull this into staging as per Wolfgang's
>> > recent request for help?
>> >
>> > Thanks,
>> >    Anton
>> >
>>
>> Please can one of the maintainers grab this and put it in staging?
>
> I've seen similar patch, it is already in arm tree:
>
> http://git.denx.de/?p=u-boot/u-boot-arm.git;a=commit;h=81432275812e7d9b3b5eec9c54c6c58dfe506759

OK, that's a funny place for it, but OK. Thanks for pointing that out.

Regards,
Simon

>
> Anatolij
>
Stefano Babic - Nov. 23, 2011, 8:30 a.m.
On 22/11/2011 23:10, Simon Glass wrote:
> On Tue, Nov 15, 2011 at 10:25 AM, Anton Staaf <robotboy@chromium.org> wrote:
>> Tested-by: Anton Staaf <robotboy@chromium.org>
>> Mike, do you think you could pull this into staging as per Wolfgang's
>> recent request for help?
>>
>> Thanks,
>>    Anton
>>
> 
> Please can one of the maintainers grab this and put it in staging?

Hi Simon,

I grab it in my TODO list in patchwork, I will take a look at it.

Stefano
Simon Glass - Nov. 23, 2011, 7:18 p.m.
On Wed, Nov 23, 2011 at 12:30 AM, Stefano Babic <sbabic@denx.de> wrote:
> On 22/11/2011 23:10, Simon Glass wrote:
>> On Tue, Nov 15, 2011 at 10:25 AM, Anton Staaf <robotboy@chromium.org> wrote:
>>> Tested-by: Anton Staaf <robotboy@chromium.org>
>>> Mike, do you think you could pull this into staging as per Wolfgang's
>>> recent request for help?
>>>
>>> Thanks,
>>>    Anton
>>>
>>
>> Please can one of the maintainers grab this and put it in staging?
>
> Hi Simon,
>
> I grab it in my TODO list in patchwork, I will take a look at it.

Thanks Stefano - please also see Anatolij's email on this thread. It
seems that Albert may have picked up a similar patch.

Regards,
Simon

>
> Stefano
>
> --
> =====================================================================
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
> =====================================================================
>
Stefano Babic - Nov. 23, 2011, 8:57 p.m.
Am 23/11/2011 20:18, schrieb Simon Glass:

> 
> Thanks Stefano - please also see Anatolij's email on this thread. It
> seems that Albert may have picked up a similar patch.

In fact the patch "Fix errors noticed after enabling
CONFIG_EFI_PARTITION for the OMAP3 EVM board" has already fixed this
issue. There is no need to merge this patch.

Best regards.
Stefano Babic

Patch

diff --git a/disk/part_efi.c b/disk/part_efi.c
index e7f2714..ddf80a7 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -130,7 +130,7 @@  void print_part_efi(block_dev_desc_t * dev_desc)
 	}
 	/* This function validates AND fills in the GPT header and PTE */
 	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
-			 &(gpt_head), &gpt_pte) != 1) {
+			 gpt_head, &gpt_pte) != 1) {
 		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
 		return;
 	}
@@ -169,7 +169,7 @@  int get_partition_info_efi(block_dev_desc_t * dev_desc, int part,
 
 	/* This function validates AND fills in the GPT header and PTE */
 	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
-			&(gpt_head), &gpt_pte) != 1) {
+			gpt_head, &gpt_pte) != 1) {
 		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
 		return -1;
 	}