diff mbox series

common/board_f: Respect original FDT size while relocating

Message ID 20200619082218.656-1-andr2000@gmail.com
State New
Headers show
Series common/board_f: Respect original FDT size while relocating | expand

Commit Message

Oleksandr Andrushchenko June 19, 2020, 8:22 a.m. UTC 
From: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>

While relocating FDT we reserve some memory for the new FDT and
set the size of the FDT with that respect. But FDT may be placed
at the end of the RAM leading to memory access beyond it.
Fix this by copying exact FDT size bytes, not the reserved size.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
---
 common/board_f.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Tom Rini June 19, 2020, 1:53 p.m. UTC | #1 
On Fri, Jun 19, 2020 at 11:22:18AM +0300, Oleksandr Andrushchenko wrote:

> From: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
> 
> While relocating FDT we reserve some memory for the new FDT and
> set the size of the FDT with that respect. But FDT may be placed
> at the end of the RAM leading to memory access beyond it.
> Fix this by copying exact FDT size bytes, not the reserved size.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
> ---
>  common/board_f.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/board_f.c b/common/board_f.c
> index 01194eaa0e4d..aa1285e94999 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -670,7 +670,7 @@ static int reloc_fdt(void)
>  	if (gd->flags & GD_FLG_SKIP_RELOC)
>  		return 0;
>  	if (gd->new_fdt) {
> -		memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
> +		memcpy(gd->new_fdt, gd->fdt_blob, fdt_totalsize(gd->fdt_blob));
>  		gd->fdt_blob = gd->new_fdt;
>  	}
>  #endif

So, I think the problem is placing the fdt so close to the end of memory
and we need to fix that.  With the above change, we won't copy past the
end of memory but gd->fdt_blob + gd->fdt_size will still point past it,
yes?  Thanks!
Oleksandr Andrushchenko June 19, 2020, 3:19 p.m. UTC | #2 
On 6/19/20 4:53 PM, Tom Rini wrote:
> On Fri, Jun 19, 2020 at 11:22:18AM +0300, Oleksandr Andrushchenko wrote:
>
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
>>
>> While relocating FDT we reserve some memory for the new FDT and
>> set the size of the FDT with that respect. But FDT may be placed
>> at the end of the RAM leading to memory access beyond it.
>> Fix this by copying exact FDT size bytes, not the reserved size.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
>> ---
>>   common/board_f.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/board_f.c b/common/board_f.c
>> index 01194eaa0e4d..aa1285e94999 100644
>> --- a/common/board_f.c
>> +++ b/common/board_f.c
>> @@ -670,7 +670,7 @@ static int reloc_fdt(void)
>>   	if (gd->flags & GD_FLG_SKIP_RELOC)
>>   		return 0;
>>   	if (gd->new_fdt) {
>> -		memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
>> +		memcpy(gd->new_fdt, gd->fdt_blob, fdt_totalsize(gd->fdt_blob));
>>   		gd->fdt_blob = gd->new_fdt;
>>   	}
>>   #endif
> So, I think the problem is placing the fdt so close to the end of memory
> and we need to fix that.
Exactly
>    With the above change, we won't copy past the
> end of memory
yes
>   but gd->fdt_blob + gd->fdt_size will still point past it,
> yes?

Not really as the next op after the memcpy will set fdt_blob to the new location

and it is safe to access the memory in [gd->fdt_blob; gd->fdt_blob + gd->fdt_size) range.

We only ensure we are copying the fdt itself, not also the reserved memory which

doesn't exist past the original fdt addresses

>   Thanks!
>
Tom Rini June 19, 2020, 5:51 p.m. UTC | #3 
On Fri, Jun 19, 2020 at 03:19:21PM +0000, Oleksandr Andrushchenko wrote:
> On 6/19/20 4:53 PM, Tom Rini wrote:
> > On Fri, Jun 19, 2020 at 11:22:18AM +0300, Oleksandr Andrushchenko wrote:
> >
> >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
> >>
> >> While relocating FDT we reserve some memory for the new FDT and
> >> set the size of the FDT with that respect. But FDT may be placed
> >> at the end of the RAM leading to memory access beyond it.
> >> Fix this by copying exact FDT size bytes, not the reserved size.
> >>
> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
> >> ---
> >>   common/board_f.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/common/board_f.c b/common/board_f.c
> >> index 01194eaa0e4d..aa1285e94999 100644
> >> --- a/common/board_f.c
> >> +++ b/common/board_f.c
> >> @@ -670,7 +670,7 @@ static int reloc_fdt(void)
> >>   	if (gd->flags & GD_FLG_SKIP_RELOC)
> >>   		return 0;
> >>   	if (gd->new_fdt) {
> >> -		memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
> >> +		memcpy(gd->new_fdt, gd->fdt_blob, fdt_totalsize(gd->fdt_blob));
> >>   		gd->fdt_blob = gd->new_fdt;
> >>   	}
> >>   #endif
> > So, I think the problem is placing the fdt so close to the end of memory
> > and we need to fix that.
> Exactly
> >    With the above change, we won't copy past the
> > end of memory
> yes
> >   but gd->fdt_blob + gd->fdt_size will still point past it,
> > yes?
> 
> Not really as the next op after the memcpy will set fdt_blob to the new location
> 
> and it is safe to access the memory in [gd->fdt_blob; gd->fdt_blob + gd->fdt_size) range.
> 
> We only ensure we are copying the fdt itself, not also the reserved memory which
> 
> doesn't exist past the original fdt addresses

Ah, so I had something backwards then.  We're fine because gd->new_fdt +
gd->fdt_size is fine.  Thanks!
Oleksandr Andrushchenko June 19, 2020, 6:21 p.m. UTC | #4 
On 6/19/20 8:51 PM, Tom Rini wrote:
> On Fri, Jun 19, 2020 at 03:19:21PM +0000, Oleksandr Andrushchenko wrote:
>> On 6/19/20 4:53 PM, Tom Rini wrote:
>>> On Fri, Jun 19, 2020 at 11:22:18AM +0300, Oleksandr Andrushchenko wrote:
>>>
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
>>>>
>>>> While relocating FDT we reserve some memory for the new FDT and
>>>> set the size of the FDT with that respect. But FDT may be placed
>>>> at the end of the RAM leading to memory access beyond it.
>>>> Fix this by copying exact FDT size bytes, not the reserved size.
>>>>
>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
>>>> ---
>>>>    common/board_f.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/common/board_f.c b/common/board_f.c
>>>> index 01194eaa0e4d..aa1285e94999 100644
>>>> --- a/common/board_f.c
>>>> +++ b/common/board_f.c
>>>> @@ -670,7 +670,7 @@ static int reloc_fdt(void)
>>>>    	if (gd->flags & GD_FLG_SKIP_RELOC)
>>>>    		return 0;
>>>>    	if (gd->new_fdt) {
>>>> -		memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
>>>> +		memcpy(gd->new_fdt, gd->fdt_blob, fdt_totalsize(gd->fdt_blob));
>>>>    		gd->fdt_blob = gd->new_fdt;
>>>>    	}
>>>>    #endif
>>> So, I think the problem is placing the fdt so close to the end of memory
>>> and we need to fix that.
>> Exactly
>>>     With the above change, we won't copy past the
>>> end of memory
>> yes
>>>    but gd->fdt_blob + gd->fdt_size will still point past it,
>>> yes?
>> Not really as the next op after the memcpy will set fdt_blob to the new location
>>
>> and it is safe to access the memory in [gd->fdt_blob; gd->fdt_blob + gd->fdt_size) range.
>>
>> We only ensure we are copying the fdt itself, not also the reserved memory which
>>
>> doesn't exist past the original fdt addresses
> Ah, so I had something backwards then.  We're fine because gd->new_fdt +
> gd->fdt_size is fine.  Thanks!
>
Yes, that's right
Simon Glass June 26, 2020, 1:43 a.m. UTC | #5 
On Fri, 19 Jun 2020 at 02:22, Oleksandr Andrushchenko
<andr2000 at gmail.com> wrote:
>
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
>
> While relocating FDT we reserve some memory for the new FDT and
> set the size of the FDT with that respect. But FDT may be placed
> at the end of the RAM leading to memory access beyond it.
> Fix this by copying exact FDT size bytes, not the reserved size.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com>
> ---
>  common/board_f.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg at chromium.org>
diff mbox series

Patch

diff --git a/common/board_f.c b/common/board_f.c
index 01194eaa0e4d..aa1285e94999 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -670,7 +670,7 @@  static int reloc_fdt(void)
 	if (gd->flags & GD_FLG_SKIP_RELOC)
 		return 0;
 	if (gd->new_fdt) {
-		memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
+		memcpy(gd->new_fdt, gd->fdt_blob, fdt_totalsize(gd->fdt_blob));
 		gd->fdt_blob = gd->new_fdt;
 	}
 #endif