This removes mac_label_copyin32() as mac_label_copyin() can now handle
both native and 32-bit struct mac objects.
Obtained from: CheriBSD
Sponsored by: AFRL, DARPA
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 68635 Build 65518: arc lint + arc unit
Event Timeline
| sys/security/mac/mac_syscalls.c | ||
|---|---|---|
| 99 | I would keep the original type for u_mac, as it is not a pointer to a struct mac for 32-bit processes, which could lead to programming errors. In general, I think userland pointers in the kernel should always be of type void * to avoid inadvertent direct uses. The argument name (and herald comments) should be enough of a documentation for what kind of thing these pointers point to. | |
| sys/security/mac/mac_syscalls.c | ||
|---|---|---|
| 99 | That’s the opposite of what makesyscalls.lua generates. Typed pointers are the style. | |
| sys/security/mac/mac_syscalls.c | ||
|---|---|---|
| 99 | makesyscalls.lua does not generate sub-routines to copy in sub-structures, and particularly sub-routines that can adapt to the ABI (thus supporting multiple types in input), so relating its output to the style of mac_label_copyin() is dubious. makesyscalls.lua generates one prototype per ABI, with indeed the proper type for arguments there but they are encapsulated in a per-syscall-specific structure. The point here is precisely that there is a stronger reason than questionable consistency to actually *not* have a readily usable type: Avoid errors where userland pointers are directly used in the kernel. And I don't see any real drawback in doing that. Do you see some? Are there Cheri-specific ones? The situation of syscall prototypes is slightly different because arguments appear as fields of a syscall-specific structure, so at least you have to use an indirection to get access to them, which is already a (more or less) visible indication that you're fetching a userland pointer. But it's only an advisory one; in this case also, the compiler won't prevent you to use a pointer argument directly, that's the link I would instead make between makesyscalls.lua and the situation here. So I think we should actually either convert all of the pointer arguments to void * for kernel prototypes, or use a more clever scheme, ideally tagging such arguments with some attributes making it a compile error to dereference the pointer, but I don't know if that is readily possible (I seem to recall seeing something like __userland in Linux a while ago? I'll look that up). Note that this would concern all syscall entry points, e.g., the sys_*() functions (tagging the fields of their associated structure), but not necessarily the kern_*() or user_*() ones. But those last ones would anyway need to take a void * pointer or, better, some pointer to a union, in case they have to handle "polymorphic" pointers. | |
Comment Actions
Well, this isn't a generated stub so using void * could be ok, OTOH, it is now always passed something like wcred->cr_label in which case it is typed (though user_setcred intentionally discards that type) and it might be nice to keep what type checking you do get there (so that for internal APIs a compat user pointer is always treated as the native user pointer). Most places where we deal with user pointers though they are just one type and not polymorphic as in this case. Globally changing all user pointers would definitely be a net negative, but using void * here is probably fine.
Comment Actions
user_setcred() passes mac_label_copyin() wcred->cr_label, yes, but the latter is only used as a temporary pointer storage until after mac_label_copyin(), and thus one cannot assume that wcred->sc_label is of type struct mac * before mac_label_copyin(). I re-used wcred->sc_label for semantics (grouping information in a single struct setcred), at the expense of making the code not really "type clean" before mac_label_copyin(). That could be changed if you think it would be clearer in the 32-bit case to copy the label pointer in umac directly and have wcred->sc_label filled only by mac_label_copyin().
In any case, the umac argument to mac_label_copyin() really can be a pointer to struct mac or struct mac32, so a type of struct mac * is deceptive (it's an input parameter, not an output one, and I don't see a reason to temporarily derogate to the type here, contrary to what is described in the previous paragraph). Having a pointer to union here would probably be overkill, but would be the exact expression of what umac really is. void * is already a compromise, with the benefit that you can't dereference umac by error.
Most places where we deal with user pointers though they are just one type and not polymorphic as in this case. Globally changing all user pointers would definitely be a net negative, but using void * here is probably fine.
I'd probably have a slightly different take here, but that doesn't matter much anyway since I think we can have the best of both worlds (keep the real type, but enforce the pointer is not dereferenced), which I'll try to hack as soon as I have more time. It certainly can wait though if you think this is going to interfere badly with Cheri upstreaming.
Comment Actions
Please don't add Linux-style __user annotations. Every single one will be a conflict in CheriBSD because in hybrid C mode (not being upstreamed to FreeBSD) we add a qualifier to the *pointer* saying that it is a capability. __user annotations apply to the thing being pointed to, not the pointer itself so we can not use them as they are in the wrong position.
Comment Actions
Please add Effort: CHERI upstreaming to the commit message so we count this as part of that effort.
Comment Actions
Admittedly I haven't played with attributes recently. I think the quite cumbersome GCC documentation on Attribute Syntax is nonetheless clear about the fact that an attribute placed before the semi-colon applies to the object being declared, so would apply to the pointer itself, by contrast with putting an attribute in the specifiers/qualifiers list of a declaration. Does your experience suggest otherwise?
A couple of other points:
- I intend this modification to apply only to generated prototypes and syscall-specific structures, which can only work if the compiler accepts an annotated function declaration together with a non-annotated definition (that remains to be checked). That would mean no interference at all with hand-written code and manually-added qualifiers for capabilities.
- For now, only clang supports noderef, so the annotation macro would just be empty for GCC.
- clang may support C23 attributes correctly even when compiling with an older standard, so we might want to leverage that instead of GCC-style attributes.