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

[David Gibson]

On Mon, Jul 08, 2024 at 04:59:30PM +0100, Peter Maydell wrote:
> 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.

Sure.

> 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.

From the qemu point of view, yes.  And theoretically, the fix is easy,
since libfdt provides fdt32_ld() etc. for exactly this use case.  But..

> 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."

.. this makes fdt32_ld() etc. unusable by design.

> 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).

Fair enough from the qemu point of view.  However, this unusable by
design interface was written by me as part of a library I maintain, so
it certainly worries *me*.

> 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
> 

-- 
David Gibson (he or they)       | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you, not the other way
                                | around.
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature