MAC: syscalls: mac_label_copyin(): 32-bit compatibility

[olce]

Summary

A step on the way to support 32-bit compatibility for MAC-related system
calls. Needed by the upcoming setcred() system call.

[olce]

sys/security/mac/mac_syscalls.c

154–159

Well, sometimes it's enough to just upload some diff to get a problem back... Bracketing test for COMPAT_FREEBSD32 is missing here, will add.

[brooks]

sys/security/mac/mac_syscalls.c

98

Why _int? Maybe _impl?

sys/security/mac/mac_syscalls.h

27

This must be named struct mac32 to support freebsd32 system call stub generation.

I'd also be tempted to forward declare it here and keep the real definition in mac_syscalls.c.

[olce]

Apply brooks@'s request and suggestion.

[olce]

sys/security/mac/mac_syscalls.c

98

_int for "internal". Renamed it to _impl.

[jhb]

Blech, this conflicted quite a bit with CheriBSD downstream which has COMPAT_FREEBSD64 and used SV_CURPROC_FLAG to determine the ABI instead of a bool (that needs to not be a bool). At some point I will probably just have to upstream reworking this to remove all the bool stuff as you don't need it since you can just check the ABI here and do the fixup as needed. In CheriBSD we also fix the copying in of struct mac for exec which also needs to honor the ABI.

[olce]

In D47878#1226543, @jhb wrote:

Blech, this conflicted quite a bit with CheriBSD downstream which has COMPAT_FREEBSD64 and used SV_CURPROC_FLAG to determine the ABI instead of a bool (that needs to not be a bool). At some point I will probably just have to upstream reworking this to remove all the bool stuff as you don't need it since you can just check the ABI here and do the fixup as needed. In CheriBSD we also fix the copying in of struct mac for exec which also needs to honor the ABI.

Yes, more or less expected, didn't look at what CheriBSD was doing back then. We perhaps could have used SV_ILP32 in advance here. So you have COMPAT_FREEBSD64, which basically means you can compile it out?

[jhb]

In D47878#1226550, @olce wrote:

In D47878#1226543, @jhb wrote:

Blech, this conflicted quite a bit with CheriBSD downstream which has COMPAT_FREEBSD64 and used SV_CURPROC_FLAG to determine the ABI instead of a bool (that needs to not be a bool). At some point I will probably just have to upstream reworking this to remove all the bool stuff as you don't need it since you can just check the ABI here and do the fixup as needed. In CheriBSD we also fix the copying in of struct mac for exec which also needs to honor the ABI.

Yes, more or less expected, didn't look at what CheriBSD was doing back then. We perhaps could have used SV_ILP32 in advance here. So you have COMPAT_FREEBSD64, which basically means you can compile it out?

Actually, I found a bug in the compat32 setcred() as well. Yes, we have COMPAT_FREEBSD64 downstream. I will work on some patches though I'll try to post for review today/tomorrow.

[olce]

In D47878#1226923, @jhb wrote:

Actually, I found a bug in the compat32 setcred() as well. (...) I will work on some patches though I'll try to post for review today/tomorrow.

I've quickly reviewed again that code and fail to see any practical problem (there's a memcpy() that conceptually should operate on &wcred32.setcred32_copy_start instead of &wcred32, but that makes no difference in practice).

If you're unable to produce quickly a separate patch for this, could you please point me at the problem you've seen? I'd like to quickly assess if it's very easily fixable and, if that's the case, produce a minimal fix and then ask re@/cperciva@ whether they are OK to include it in 15.0.

[jhb]

In D47878#1227305, @olce wrote:

In D47878#1226923, @jhb wrote:

Actually, I found a bug in the compat32 setcred() as well. (...) I will work on some patches though I'll try to post for review today/tomorrow.

I've quickly reviewed again that code and fail to see any practical problem (there's a memcpy() that conceptually should operate on &wcred32.setcred32_copy_start instead of &wcred32, but that makes no difference in practice).

If you're unable to produce quickly a separate patch for this, could you please point me at the problem you've seen? I'd like to quickly assess if it's very easily fixable and, if that's the case, produce a minimal fix and then ask re@/cperciva@ whether they are OK to include it in 15.0.

The memcpy size is wrong. Subtracting two int *'s is not the same as subtracting two char *'s. My fix is to avoid the (IMO overly clever) memcpy and just use plain CP assignments. Compilers are smart enough to optimize this, and this is more typical style. I can upload my small series of fixes for now. I can't test them because kernels on HEAD are broken (my VM locks up and panics in make buildkernel now and I need to bisect to where that broke).

[olce]

In D47878#1227393, @jhb wrote:

The memcpy size is wrong.

Indeed. I should have cast the pointers to uintptr_t or ptrdiff_t to avoid that. I don't think the memcpy() is particularly clever, but these structures are not supposed to change often, so explicitly listing all fields another time is not horrible (avoiding this is the real point for using memcpy(), not performance optimization). The really "clever" fix would be to use X macros, but given I'd like to ask for inclusion of a fix in 15.0, let's forget about it for the time being.

So, yes, please upload your fix, and I'll test it on an earlier HEAD and ask for inclusion in 15.0.