unix/*: rewrite unp_internalize() cmsg parsing cycle

[glebius]

Summary

Make it a complex, but a single for(;;) statement. The previous cycle
with some loop logic in the beginning and some loop logic at the end
was confusing. Both me and markj@ were misleaded to a conclusion that
some checks are unnecessary, while they actually were necessary.

[glebius]

tiny fixup

[markj]

What does "unix/*:" mean in the commit title? Why not just "unix:"?

sys/kern/uipc_usrreq.c

2640

2642

Hmm, I think there is a sneaky bug here, also present in the old version.

Note that on LP64 platforms, CMSG_SPACE(0) > sizeof(struct cmsghdr), since message data is aligned to 8 bytes and sizeof(struct cmsghdr) == 12. So it's possible to have data - (char *)cm > cm->cmsg_len. That means that the calculation of datalen can underflow.

For example, set msg_controllen = CMSG_SPACE(0) and msg_control = &(struct cmsghdr){.cmsg_len = sizeof(struct cmsghdr), .cmsg_level = SOL_SOCKET, .cmsg_type = SCM_foo}.

I think in this case it's mostly harmless because the test CMSG_SPACE(newlen) > MCLBYTES below will fail, and for non SCM_RIGHTS messages, CMSG_SPACE(datalen) will equal CMSG_SPACE(0) due to 32-bit integer truncation, so the loop will break (though we do not signal an error). But we should definitely fix this.

[glebius]

• Handle incorrectly filled cmsg_len
• Add test case for this
• Rebase it independent from my unix-dgram branch

[markj]

Thanks! Looks ok to me, my commits are minor.

sys/kern/uipc_usrreq.c

2642

Can't it be part of the for loop condition?

tests/sys/kern/unix_passfd_test.c

748 ↗

(On Diff #106548)

This comment is too specific now, probably the second sentence can just be deleted.

775 ↗

(On Diff #106548)

CMSG_LEN(0) is exactly equal to sizeof(struct cmsghdr).

783 ↗

(On Diff #106548)

Rather than having an ifdef LP64 (I believe this is discouraged in new code since at least on CHERI, sizeof(long) != sizeof(void *)), we could use the predicate CMSG_SPACE(0) != CMSG_LEN(0) or CMSG_SPACE(0) != sizeof(struct cmsghdr).

[glebius]

tests/sys/kern/unix_passfd_test.c

775 ↗

(On Diff #106548)

Nope:

(kgdb) p sizeof(struct cmsghdr)
$1 = 12
(kgdb) p (((__uintptr_t)(sizeof(struct cmsghdr)) + 7) & ~7)
$2 = 16

[glebius]

Will update on the other comments today. Thanks

[markj]

tests/sys/kern/unix_passfd_test.c

775 ↗

(On Diff #106548)

Oops, sorry, I forgot that CMSG_LEN includes alignment for the header.

[glebius]

Suggestions from Mark

[glebius]

tests/sys/kern/unix_passfd_test.c

783 ↗

(On Diff #106548)

Unfortunately we can't use sizeof() in preprocessor. Do you want me to code the check in C?

[markj]

tests/sys/kern/unix_passfd_test.c

783 ↗

(On Diff #106548)

Yeah, I think checking in C is fine...

[glebius]

tests/sys/kern/unix_passfd_test.c

783 ↗

(On Diff #106548)

Then, I think, this is final version.

[markj]

Thanks!