On Tue, Dec 17, 2019 at 09:00:08PM +0100, Arnd Bergmann wrote: > On Tue, Dec 17, 2019 at 3:36 PM Rich Felker <dalias@xxxxxxxx> wrote: > > > > It came to my attention while reviewing possible breakage with move to > > 64-bit time_t that some applications are dereferencing data in socket > > control messages (particularly SCM_TIMESTAMP*) in-place as the message > > type, rather than memcpy'ing it to appropriate storage. This > > necessarily does not work and is not supportable if the message > > contains data with greater alignment requirement than the header. In > > particular, on 32-bit archs, cmsghdr has size 12 and alignment 4, but > > struct timeval and timespec may have alignment requirement 8. > > > > I found at least ptpd, socat, and ssmping doing this via Debian Code > > Search: > > > > https://sources.debian.org/src/ptpd/2.3.1-debian1-4/src/dep/net.c/?hl=1578#L1578 > > https://sources.debian.org/src/socat/1.7.3.3-2/xio-socket.c/?hl=1839#L1839 > > https://sources.debian.org/src/ssmping/0.9.1-3/ssmpngcl.c/?hl=307#L307 > > > > and I suspect there are a good deal more out there. On most archs they > > won't break, or will visibly break with SIGBUS, but in theory it's > > possible that they silently read wrong data and this might happen on > > some older and more tiny-embedded-oriented archs. > > Good find. I suppose this is going to be particularly annoying for > architectures that are affected because all systems that are in > widespread use are not affected: > > - x86, riscv, ppc and s390 always allow unaligned loads > - ARMv6+ mostly allows unaligned loads. Some instructions such as ldrd > require alignment of four bytes, which is ok, and ARMv5 requires natural > alignment up to 32 bits, so this is also ok Seems correct. x > - On MIPS I think that o32 is fine since there are no 64-bit loads, but > n64 would likely be affected, if there are still users remaining (musl > supports it, so I assume there are some users). I think you mean n32. n64 is the full LP64 ABI. Indeed it seems like n32 is likely affected unless the kernel traps and fixes up misaligned accesses. > - m68k only requires 16-bit alignment > - For the other 32-bit architectures that musl supports (microblaze, sh, ^^^^^^^^^^^^^^^^^^ FWIW this isn't specific to musl; glibc is also affected, and uclibc would be too if they ever implement time64. > openrisc), none advertise unaligned-access capability to the kernel, > but I also don't think any of them have a native 64-bit load instruction. > armv5, microblaze, sh and nds32 fix up unaligned accesses in an > exception handler; openrisc and csky require aligned accesses in user > space. This sounds correct. Presently J2 (open source SH2 ISA implementation) has no unaligned trap; it just loads/stores the wrong value. But there are no 64-bit load/store insns anyway and 32-bit alignment is met. > > I think it's clear to someone who understands alignment and who's > > thought about it that applications just can't do this, but it doesn't > > seem to be documented, and an example in cmsg(3) even shows access to > > int payload via *(int *)CMSG_DATA(cmsg) (of course int is safe because > > its alignment is <= header alignment, but this is not mentioned). > > > > Could we add text, and perhaps change the example, to indicate that in > > general memcpy needs to be used to copy the payload to/from a suitable > > object? > > Yes, I think that would be a good idea. How about adding to: * CMSG_DATA() returns a pointer to the data portion of a cmsghdr. "The pointer returned cannot be assumed to be suitably aligned for accessing arbitrary payload data types. Applications should not cast it to a pointer type matching the payload, but should use memcpy to copy data to or from a suitably declared object." and doing this in the examples? Are there other places it should be mentioned to to make sure readers see it? Rich
