[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Stefan Roese]

On SoCFPGA, using "sf update" with an non-4byte aligned length leads
to a hangup (and reboot via watchdog). This is because of the unaligned
access in the cadence QSPI driver which is hard to prevent since the
data is written into a 4-byte wide FIFO. This patch fixes this problem
by changing the behavior of the last sector write (not sector aligned).

The new code is even simpler and copies the source data into the temp
buffer and now uses the temp buffer to write the complete sector. So
only one SPI sector write is used now instead of 2 in the old version.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
Cc: Holger Brunck <holger.brunck@keymile.com>
Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
 common/cmd_sf.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 5c788e9..dd82290 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -163,6 +163,8 @@ static int do_spi_flash_probe(int argc, char * const argv[])
 static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
 		size_t len, const char *buf, char *cmp_buf, size_t *skipped)
 {
+	char *ptr = (char *)buf;
+
 	debug("offset=%#x, sector_size=%#x, len=%#zx\n",
 	      offset, flash->sector_size, len);
 	/* Read the entire sector so to allow for rewriting */
@@ -178,16 +180,14 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
 	/* Erase the entire sector */
 	if (spi_flash_erase(flash, offset, flash->sector_size))
 		return "erase";
-	/* Write the initial part of the block from the source */
-	if (spi_flash_write(flash, offset, len, buf))
-		return "write";
-	/* If it's a partial sector, rewrite the existing part */
+	/* If it's a partial sector, copy the data into the temp-buffer */
 	if (len != flash->sector_size) {
-		/* Rewrite the original data to the end of the sector */
-		if (spi_flash_write(flash, offset + len,
-				    flash->sector_size - len, &cmp_buf[len]))
-			return "write";
+		memcpy(cmp_buf, buf, len);
+		ptr = cmp_buf;
 	}
+	/* Write one complete sector */
+	if (spi_flash_write(flash, offset, flash->sector_size, ptr))
+		return "write";
 
 	return NULL;
 }
-- 
2.2.1

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Wolfgang Denk]

Dear Stefan,

In message <1420810762-10712-1-git-send-email-sr@denx.de> you wrote:
>
> The new code is even simpler and copies the source data into the temp
> buffer and now uses the temp buffer to write the complete sector. So
> only one SPI sector write is used now instead of 2 in the old version.
...

>  	if (len != flash->sector_size) {
> -		/* Rewrite the original data to the end of the sector */
> -		if (spi_flash_write(flash, offset + len,
> -				    flash->sector_size - len, &cmp_buf[len]))
> -			return "write";
> +		memcpy(cmp_buf, buf, len);
> +		ptr = cmp_buf;
>  	}

Should we add a  memset(buf, 0, sizeof(buf))  before the memcpy() to
prevent information from earlier activities to leak?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I read part of it all the way through.

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Stefan Roese]

Hi Wolfgang,

On 12.01.2015 08:17, Wolfgang Denk wrote:
>> The new code is even simpler and copies the source data into the temp
>> buffer and now uses the temp buffer to write the complete sector. So
>> only one SPI sector write is used now instead of 2 in the old version.
> ...
>
>>   	if (len != flash->sector_size) {
>> -		/* Rewrite the original data to the end of the sector */
>> -		if (spi_flash_write(flash, offset + len,
>> -				    flash->sector_size - len, &cmp_buf[len]))
>> -			return "write";
>> +		memcpy(cmp_buf, buf, len);
>> +		ptr = cmp_buf;
>>   	}
>
> Should we add a  memset(buf, 0, sizeof(buf))  before the memcpy() to
> prevent information from earlier activities to leak?

"buf" points to the new data to be written into the flash. We're 
overwriting the first "len" bytes of "cmp_buf" with this data.

I don't see why we should erase anything there. Perhaps I'm missing 
something though.

Thanks,
Stefan

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Gerlando Falauto]

Hi Stefan,

thanks for the patch.
Please compare it with my v1 revision of the patch:
http://patchwork.ozlabs.org/patch/150468/
Back then, Simon suggested to use two separate SPI flash operations to 
avoid the memcpy() operation. I guess noone would have guessed about 
QSPI devices at the time.

Anyway, your patch looks even better than my v1 then, so:

Acked-by: Gerlando Falauto <gerlando.falauto@keymile.com>

Thank you,
Gerlando

On 01/09/2015 02:39 PM, Stefan Roese wrote:
> On SoCFPGA, using "sf update" with an non-4byte aligned length leads
> to a hangup (and reboot via watchdog). This is because of the unaligned
> access in the cadence QSPI driver which is hard to prevent since the
> data is written into a 4-byte wide FIFO. This patch fixes this problem
> by changing the behavior of the last sector write (not sector aligned).
>
> The new code is even simpler and copies the source data into the temp
> buffer and now uses the temp buffer to write the complete sector. So
> only one SPI sector write is used now instead of 2 in the old version.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Gerlando Falauto <gerlando.falauto@keymile.com>
> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> Cc: Holger Brunck <holger.brunck@keymile.com>
> Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
> ---
>   common/cmd_sf.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index 5c788e9..dd82290 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -163,6 +163,8 @@ static int do_spi_flash_probe(int argc, char * const argv[])
>   static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
>   		size_t len, const char *buf, char *cmp_buf, size_t *skipped)
>   {
> +	char *ptr = (char *)buf;
> +
>   	debug("offset=%#x, sector_size=%#x, len=%#zx\n",
>   	      offset, flash->sector_size, len);
>   	/* Read the entire sector so to allow for rewriting */
> @@ -178,16 +180,14 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
>   	/* Erase the entire sector */
>   	if (spi_flash_erase(flash, offset, flash->sector_size))
>   		return "erase";
> -	/* Write the initial part of the block from the source */
> -	if (spi_flash_write(flash, offset, len, buf))
> -		return "write";
> -	/* If it's a partial sector, rewrite the existing part */
> +	/* If it's a partial sector, copy the data into the temp-buffer */
>   	if (len != flash->sector_size) {
> -		/* Rewrite the original data to the end of the sector */
> -		if (spi_flash_write(flash, offset + len,
> -				    flash->sector_size - len, &cmp_buf[len]))
> -			return "write";
> +		memcpy(cmp_buf, buf, len);
> +		ptr = cmp_buf;
>   	}
> +	/* Write one complete sector */
> +	if (spi_flash_write(flash, offset, flash->sector_size, ptr))
> +		return "write";
>
>   	return NULL;
>   }
>

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Stefan Roese]

Hi Gerlando,

On 12.01.2015 08:51, Gerlando Falauto wrote:
> thanks for the patch.
> Please compare it with my v1 revision of the patch:
> http://patchwork.ozlabs.org/patch/150468/
> Back then, Simon suggested to use two separate SPI flash operations to
> avoid the memcpy() operation.

This again could generate unaligned accesses which my patch version does 
not do.

> I guess noone would have guessed about
> QSPI devices at the time.
>
> Anyway, your patch looks even better than my v1 then, so:
>
> Acked-by: Gerlando Falauto <gerlando.falauto@keymile.com>

Thanks,
Stefan

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Gerlando Falauto]

Hi Wolfgang,

On 01/12/2015 08:27 AM, Stefan Roese wrote:
> Hi Wolfgang,
>
> On 12.01.2015 08:17, Wolfgang Denk wrote:
>>> The new code is even simpler and copies the source data into the temp
>>> buffer and now uses the temp buffer to write the complete sector. So
>>> only one SPI sector write is used now instead of 2 in the old version.
>> ...
>>
>>>    	if (len != flash->sector_size) {
>>> -		/* Rewrite the original data to the end of the sector */
>>> -		if (spi_flash_write(flash, offset + len,
>>> -				    flash->sector_size - len, &cmp_buf[len]))
>>> -			return "write";
>>> +		memcpy(cmp_buf, buf, len);
>>> +		ptr = cmp_buf;
>>>    	}
>>
>> Should we add a  memset(buf, 0, sizeof(buf))  before the memcpy() to
>> prevent information from earlier activities to leak?
>
> "buf" points to the new data to be written into the flash. We're
> overwriting the first "len" bytes of "cmp_buf" with this data.
>
> I don't see why we should erase anything there. Perhaps I'm missing
> something though.

That's right, and that's the whole point: cmp_buf points to the data 
read from the flash sector before erasing it, because that's what we 
want to keep (by re-writing it).
The first part, however, we overwrite with the new data (buf).
So there's nothing to erase.

Thanks,
Gerlando

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Gerlando Falauto]

Hi Stefan,

On 01/12/2015 08:56 AM, Stefan Roese wrote:
> Hi Gerlando,
>
> On 12.01.2015 08:51, Gerlando Falauto wrote:
>> thanks for the patch.
>> Please compare it with my v1 revision of the patch:
>> http://patchwork.ozlabs.org/patch/150468/
>> Back then, Simon suggested to use two separate SPI flash operations to
>> avoid the memcpy() operation.
>
> This again could generate unaligned accesses which my patch version does
> not do.

That's right -- I was just trying to say that my v1 was actually doing 
the same thing as yours, and to explain why I changed it at a later 
point (which seemed legitimate back then). Since my new revision 
introduced unaligned accesses, we're now essentially reverting to the 
original one. That's all.

Thank you!
Gerlando

>
>> I guess noone would have guessed about
>> QSPI devices at the time.
>>
>> Anyway, your patch looks even better than my v1 then, so:
>>
>> Acked-by: Gerlando Falauto <gerlando.falauto@keymile.com>
>
> Thanks,
> Stefan
>

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Wolfgang Denk]

Dear Stefan,

In message <54B37759.7040801@denx.de> you wrote:
> 
> > Should we add a  memset(buf, 0, sizeof(buf))  before the memcpy() to
> > prevent information from earlier activities to leak?
> 
> "buf" points to the new data to be written into the flash. We're 
> overwriting the first "len" bytes of "cmp_buf" with this data.

Oh, sorry for the mixup.  Then cmp_buf should be cleared (or at elast
the remaining, unused part).

> I don't see why we should erase anything there. Perhaps I'm missing 
> something though.

You are leaking data.  This could contain "interesting" information;
see the OpenSSL ?Heartbleed? vulnerability for a (nasty) example what
information leakage can do.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Very ugly or very beautiful women should be flattered on their
understanding, and mediocre ones on their beauty.
                                       -- Philip Earl of Chesterfield

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Wolfgang Denk]

Dear Gerlando Falauto,

In message <54B380C2.4050208@keymile.com> you wrote:
> 
> That's right, and that's the whole point: cmp_buf points to the data 
> read from the flash sector before erasing it, because that's what we 
> want to keep (by re-writing it).
> The first part, however, we overwrite with the new data (buf).
> So there's nothing to erase.

Well, if we have only a partially filled buffer, then there is some
remainder which is left over from previous use.  This should not make
it to external storage.  If you write any additional bytes, these
should be known-to-be-harmless content, say all 0x00 or 0xFF bytes,
but never any data that just happen to be there.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
For every complex problem, there is a solution that is simple,  neat,
and wrong.                                           -- H. L. Mencken

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Stefan Roese]

Hi Wolfgang,

On 12.01.2015 22:10, Wolfgang Denk wrote:
>>> Should we add a  memset(buf, 0, sizeof(buf))  before the memcpy() to
>>> prevent information from earlier activities to leak?
>>
>> "buf" points to the new data to be written into the flash. We're
>> overwriting the first "len" bytes of "cmp_buf" with this data.
>
> Oh, sorry for the mixup.  Then cmp_buf should be cleared (or at elast
> the remaining, unused part).

No. cmp_buf contains the original data from the flash. And only the 
beginning of this buffer is overwritten with the new data from "buf". 
So, the result of the memcpy() is that "cmp_buf" contains the data that 
should be written into the flash. Its a combination of the "original 
data" and the "new data".

>> I don't see why we should erase anything there. Perhaps I'm missing
>> something though.
>
> You are leaking data.  This could contain "interesting" information;
> see the OpenSSL ?Heartbleed? vulnerability for a (nasty) example what
> information leakage can do.

There is nothing leaking here. When anything would be zeroed out, the 
resulting buffer would not be the one that should be used.

Viele Gr??e,
Stefan

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Jagan Teki]

On 13 January 2015 at 11:35, Stefan Roese <sr@denx.de> wrote:
> Hi Wolfgang,
>
> On 12.01.2015 22:10, Wolfgang Denk wrote:
>>>>
>>>> Should we add a  memset(buf, 0, sizeof(buf))  before the memcpy() to
>>>> prevent information from earlier activities to leak?
>>>
>>>
>>> "buf" points to the new data to be written into the flash. We're
>>> overwriting the first "len" bytes of "cmp_buf" with this data.
>>
>>
>> Oh, sorry for the mixup.  Then cmp_buf should be cleared (or at elast
>> the remaining, unused part).
>
>
> No. cmp_buf contains the original data from the flash. And only the
> beginning of this buffer is overwritten with the new data from "buf". So,
> the result of the memcpy() is that "cmp_buf" contains the data that should
> be written into the flash. Its a combination of the "original data" and the
> "new data".
>
>>> I don't see why we should erase anything there. Perhaps I'm missing
>>> something though.
>>
>>
>> You are leaking data.  This could contain "interesting" information;
>> see the OpenSSL ?Heartbleed? vulnerability for a (nasty) example what
>> information leakage can do.
>
>
> There is nothing leaking here. When anything would be zeroed out, the
> resulting buffer would not be the one that should be used.

I think this thread link got stopped any further update on this.

thanks!
-- 
Jagan.

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Stefan Roese]

On 22.04.2015 13:11, Jagan Teki wrote:
>> On 12.01.2015 22:10, Wolfgang Denk wrote:
>>>>>
>>>>> Should we add a  memset(buf, 0, sizeof(buf))  before the memcpy() to
>>>>> prevent information from earlier activities to leak?
>>>>
>>>>
>>>> "buf" points to the new data to be written into the flash. We're
>>>> overwriting the first "len" bytes of "cmp_buf" with this data.
>>>
>>>
>>> Oh, sorry for the mixup.  Then cmp_buf should be cleared (or at elast
>>> the remaining, unused part).
>>
>>
>> No. cmp_buf contains the original data from the flash. And only the
>> beginning of this buffer is overwritten with the new data from "buf". So,
>> the result of the memcpy() is that "cmp_buf" contains the data that should
>> be written into the flash. Its a combination of the "original data" and the
>> "new data".
>>
>>>> I don't see why we should erase anything there. Perhaps I'm missing
>>>> something though.
>>>
>>>
>>> You are leaking data.  This could contain "interesting" information;
>>> see the OpenSSL ?Heartbleed? vulnerability for a (nasty) example what
>>> information leakage can do.
>>
>>
>> There is nothing leaking here. When anything would be zeroed out, the
>> resulting buffer would not be the one that should be used.
>
> I think this thread link got stopped any further update on this.

I would have thought that this patch had been applied some time ago. If 
not, then please do.

Thanks,
Stefan

[U-Boot] [PATCH] cmd_sf: Fix problem with "sf update" and unaligned length

[Jagan Teki]

On 22 April 2015 at 16:45, Stefan Roese <sr@denx.de> wrote:
> On 22.04.2015 13:11, Jagan Teki wrote:
>>>
>>> On 12.01.2015 22:10, Wolfgang Denk wrote:
>>>>>>
>>>>>>
>>>>>> Should we add a  memset(buf, 0, sizeof(buf))  before the memcpy() to
>>>>>> prevent information from earlier activities to leak?
>>>>>
>>>>>
>>>>>
>>>>> "buf" points to the new data to be written into the flash. We're
>>>>> overwriting the first "len" bytes of "cmp_buf" with this data.
>>>>
>>>>
>>>>
>>>> Oh, sorry for the mixup.  Then cmp_buf should be cleared (or at elast
>>>> the remaining, unused part).
>>>
>>>
>>>
>>> No. cmp_buf contains the original data from the flash. And only the
>>> beginning of this buffer is overwritten with the new data from "buf". So,
>>> the result of the memcpy() is that "cmp_buf" contains the data that
>>> should
>>> be written into the flash. Its a combination of the "original data" and
>>> the
>>> "new data".
>>>
>>>>> I don't see why we should erase anything there. Perhaps I'm missing
>>>>> something though.
>>>>
>>>>
>>>>
>>>> You are leaking data.  This could contain "interesting" information;
>>>> see the OpenSSL ?Heartbleed? vulnerability for a (nasty) example what
>>>> information leakage can do.
>>>
>>>
>>>
>>> There is nothing leaking here. When anything would be zeroed out, the
>>> resulting buffer would not be the one that should be used.
>>
>>
>> I think this thread link got stopped any further update on this.
>
>
> I would have thought that this patch had been applied some time ago. If not,
> then please do.

Applied to u-boot-spi/master

thanks!
-- 
Jagan.