[U-Boot] Issue with memcpy when booting a fitImage on SPEAr600

[Simon Glass]

Hi,

On Thu, 6 Dec 2018 at 03:37, Thomas Petazzoni
<thomas.petazzoni at bootlin.com> wrote:
>
> Hello Stefan,
>
> On Wed, 5 Dec 2018 12:30:57 +0100, Stefan Roese wrote:
>
> > > It's weird to me that it's happening with this SoC because it's based on
> > > ARM926ejs which is widely used I assume. Shouldn't have anyone already
> > > encountered the bug? Or is nobody actually booting a fitImage and had the
> > > luck to never call memcpy with src == dest anywhere else in the code?
> >
> > I did some work on the SPEAr600 some years ago and I'm pretty sure that
> > I never used FIT image at that time. Sorry, but I can't remember any
> > similar issues like these.
>
> Well, the issue is in generic ARM code, so whether it's SPEAr600 or any
> other ARM SoC should not really matter here.
>
> > FWIW, I would not oppose to having at least this "src == dest" check
> > in the code again, since it doesn't really make sense to overwrite
> > an area with the same value - other than for testing purposes.
>
> The thing is that the memcpy() that gets called does have this src ==
> dest check, as its code starts with:
>
> ENTRY(memcpy)
>                 cmp     r0, r1
>                 bxeq    lr
>
> which, if my assembly-fu is not bad, means: if first argument == second
> argument, then return. But even with this src == dest check within
> memcpy() itself, Quentin is seeing memmove() hang when src == dest.
>
> Another thing is that the memmove() code looks like this:
>
> {
>         char *tmp, *s;
>
>         if (dest <= src) {
>                 memcpy(dest, src, count);
>
> However, having dest <= src does not guarantee that the destination and
> source areas are non-overlapping. And the normal semantic for memcpy()
> is that it doesn't work for overlapping areas. From memcpy(3):
>
>   The memcpy() function copies n bytes from memory area src to memory
>   area dest.  The memory areas must not overlap.  Use memmove(3) if the
>   memory areas do overlap.
>
> Of course, this man page documents the memcpy() implementation from the
> C library, and perhaps the semantic of U-Boot memcpy is different.

Yes I suppose it is different.

>
> So is it correct to use memcpy() to implement memmove() when the areas
> are overlapping ?

Strictly speaking, no. I think perhaps my patch should have been more careful.

>
> Perhaps Simon Glass, who did the change to use memcpy() in
> cb0eae8cf8aaca76910dee4c7eb536d0814d1bd2 could comment on that ?
>

That said, I cannot see why the memcpy() fails. How do you explain
that? If you revert my change, does it work?

> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Regards,
Simon