Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Peter Maydell]

On Mon, 8 Jul 2024 at 08:49, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Sun Jul 7, 2024 at 9:46 AM AEST, David Gibson wrote:
> > On Sat, Jul 06, 2024 at 11:37:08AM +0100, Peter Maydell wrote:
> > > On Fri, 5 Jul 2024 at 06:13, David Gibson <david@gibson.dropbear.id.au> 
> > > wrote:
> > > > Huh.. well I'm getting different impressions of what the problem
> > > > actually is from what I initially read versus Peter Maydell's
> > > > comments, so I don't really know what to think.
> > > >
> > > > If it's just the load then fdt32_ld() etc. already exist.  Or is it
> > > > really such a hot path that unconditionally handling unaligned
> > > > accesses isn't tenable?
> > >
> > > The specific problem here is that the code as written tries to
> > > cast a not-aligned-enough pointer to uint64_t* to do the load,
> > > which is UB.
> >
> > Ah... and I'm assuming it's the cast itself which triggers the UB, not
> > just dereferencing it.
>
> Oh it's just the cast itself that is UB? Looks like that's true.
> Interesting gcc and clang don't flag it, I guess they care about
> warning on practical breakage first.

Er, I was speaking a bit vaguely there, don't take my word for
it without going and looking at the text of the C standard.

What I *meant* was that the practical problem here is that we
really do dereference a pointer for a 64-bit load when the
pointer isn't necessarily 64-bit-aligned.

As it happens, C99 says that it is the cast that is UB:
section 6.3.2.3 para 7 says:
 "A pointer to an object or incomplete type may be converted to
  a pointer to a different object or incomplete type. If the
  resulting pointer is not correctly aligned for the pointed-to
  type, the behavior is undefined. Otherwise, when converted back
  again, the result shall compare equal to the original pointer."

Presumably this is envisaging the possibility of a pointer cast
being a destructive operation somehow, such that e.g. a uint64_t*
can only represent 64-bit-aligned values. But I bet QEMU does
a lot of casting pointers around that might fall foul of this
rule, so I'm not particularly worried about trying to clean up
that kind of thing (until/unless analysers start warning about
it, in which case we have a specific set of things to clean up).

What I care about from the point of view of this patch
is that we fix the actually-broken-on-some-real-hardware problem
of doing the load as a misaligned access. My vote would be for
"take Akihiko's patch as-is, rather than gating fixing the bug
on deciding on an improvement/change to the fdt API or our
wrappers of it".

thanks
-- PMM