sys/abi_compat.h: fix UB

[kib]

Summary

compat32: provide a type and a macro for (u)int64_t handling on non-x86 arches
 
 uint64_t is 4-byte aligned on i386, but is 8-bytes aligned on all other
 32bit arches FreeBSD supports.  Provide the freebsd32_uint64_t type and
 the FU64_CP() macro, which are intended to be used where 32bit ABI uses
 (u)int64_t type, and do proper layout and copying for the aggregate type.
 

 sys/abi_compat.h: fix UB for bintime32 handling
 
 Do not cast and then access potentially unaligned uint64_t in the BT_CP()
 macro.  Use freebsd32_uint64_t type and FU64_CP() for the frac member.
 
 Noted by:       des

[des]

sys/sys/abi_compat.h

88

Wouldn't it be better to have a separate macro for the 64-bit copy? And perhaps also a type? Something like:

#if _BYTE_ORDER == _LITTLE_ENDIAN
typedef struct { uint32_t lo, hi; } uint64_32;
#else
typedef struct { uint32_t hi, lo; } uint64_32;
#endif
#define CP64(src, dst) do { \
    (dst).lo = (uint32_t)(uint64_t)(src); \
    (dst).hi = (uint32_t)((uint64_t)(src) >> 32); \
}

[kib]

sys/sys/abi_compat.h

88

This type has the wrong alignment exactly on the architectures that need it, i.e. everything except i386. To correct it, we need either explicit __align(), or use union with real int64_t. IMO existing time32_t approach is cleaner.

[des]

sys/sys/abi_compat.h

88

This is the existing approach, just with a struct instead of an array. It has exactly the same alignment. My point is that it is needed for more than just struct bintime32, so we should have a separate macro for it, which BT_CP() can then use.

[kib]

sys/sys/abi_compat.h

88

Ok, this is reasonable. In fact, the existing split of int64_t into two int32_t is only correct for x86, since other arches require 8-bytes alignment.

I rewrote the patch, see the next update.

[kib]

freebsd32_uint64_t

[kib]

After we agree on the approach and this change lands, I plan to do the pass over uses of val1/val2 in compat32, where int64_t val is used by the host ABI, converting them to freebsd32_uint64_t.

[brooks]

In D54663#1250766, @kib wrote:

After we agree on the approach and this change lands, I plan to do the pass over uses of val1/val2 in compat32, where int64_t val is used by the host ABI, converting them to freebsd32_uint64_t.

Please consider limiting this sweep to cases where the value will be mis-aligned relative to the struct.

[kib]

In D54663#1250781, @brooks wrote:

In D54663#1250766, @kib wrote:

After we agree on the approach and this change lands, I plan to do the pass over uses of val1/val2 in compat32, where int64_t val is used by the host ABI, converting them to freebsd32_uint64_t.

Please consider limiting this sweep to cases where the value will be mis-aligned relative to the struct.

Can you elaborate please, why? I understand that it formally changes nothing.

[brooks]

In D54663#1250782, @kib wrote:

In D54663#1250781, @brooks wrote:

In D54663#1250766, @kib wrote:

After we agree on the approach and this change lands, I plan to do the pass over uses of val1/val2 in compat32, where int64_t val is used by the host ABI, converting them to freebsd32_uint64_t.

Please consider limiting this sweep to cases where the value will be mis-aligned relative to the struct.

Can you elaborate please, why? I understand that it formally changes nothing.

I'd rather we only have messy MD things where they are needed. That being said, the benefit of doing it everywhere is you do it once and don't have to think about it again so perhaps you are right that we should do it everywhere.

sys/sys/abi_compat.h

71

I must admit I'm tempted to suggest this be memcpy(&(dst).fld, &(src).fld, sizeof((dst).fld));

[kib]

Switch to memcpy

[kib]

In D54663#1250789, @brooks wrote:

In D54663#1250782, @kib wrote:

In D54663#1250781, @brooks wrote:

In D54663#1250766, @kib wrote:

After we agree on the approach and this change lands, I plan to do the pass over uses of val1/val2 in compat32, where int64_t val is used by the host ABI, converting them to freebsd32_uint64_t.

Please consider limiting this sweep to cases where the value will be mis-aligned relative to the struct.

Can you elaborate please, why? I understand that it formally changes nothing.

I'd rather we only have messy MD things where they are needed. That being said, the benefit of doing it everywhere is you do it once and don't have to think about it again so perhaps you are right that we should do it everywhere.

With this change, the fix is MI: all places for uint64_t are replaced by freebsd32_uint64_t and FU64_CP (any better name btw?). Then we do not need to have any MD specifics around it. While val1/val2 bogusly exposes MD details by being i386 specific, and wrong for other arches.

[des]

sys/compat/freebsd32/freebsd32.h

44–45

Since you're using memcpy(), this can simply be an array now like it was in bintime32_t. My initial proposal used two separate members because they had descriptive names (lo and hi).

[kib]

sys/compat/freebsd32/freebsd32.h

44–45

I do not see a difference between the array and two valX members. but ok.

[kib]

Use array member for freebsd32_uint64_t on amd64.