CHERI: add mem{cpy,move}_data

[brooks]

Summary

Add kernel-only provenance-discarding memcpy_data and
memmove_data APIs intended to copy raw data which does not contain
pointers (e.g., buffers on their way to or from network or storage
devices). On CHERI architectures, they will explicitly remove tags
from capabilities, removing any provenance. This reduces the risk of
accidental spread of pointers on CHERI system.

This includes a simple C implementation for CHERI targets.

Effort: CHERI upstreaming
Sponsored by: DARPA, AFRL, Innovate UK

[ziaee]

share/man/man9/memcpy.9

93

typo

[brooks]

Fix typo

[emaste]

sys/sys/systm.h

295

How widely used is bcopy_data in CHERI? Is it reasonable to move those to memmove_data (possibly moving to memmove first in FreeBSD)? It seems slightly unfortunate to make a new version of a legacy/obsolete interface.

[brooks]

sys/sys/systm.h

295

None of these are widely used. memmove_data is currently completely unused except to implement bcopy_data. I actually think most (or even all) could be memcpy_data (I don't think any of these overlap).

The one slightly perverse argument for bcopy_data is that it's safely outside the reserved mem* namespace.

[brooks]

sys/sys/systm.h

295

I'll make the s/bcopy/memcpy changes and do some consolidation along the way.

[brooks]

Rebase after removing references to strings in base manpages

[brooks]

Drop bcopy_data

[adrian]

share/man/man9/memcpy.9

63

where is "pointer provenance" defined?

[brooks]

Link manpages to the description of pointer provenacne in arch(7)

[brooks]

share/man/man9/memcpy.9

63

Now linked to a discussion in arch(7).

[markj]

share/man/man9/memmove.9

36–37

59

sys/libkern/bcopy.c

66 ↗

(On Diff #180517)

Do you not get a warning about keeptags being unused in non-CHERI kernels?

160 ↗

(On Diff #180517)

169 ↗

(On Diff #180517)

[emaste]

share/man/man9/memcpy.9

53–55

63

Given our explicit decision of making this the non-default case, do we want to make this read a bit more in an active sense -- something along the lines of "discards" rather than "does not preserve"?

"Discards pointer provenance" is not quite right though. In a CHERI sense we could maybe use "discards capability validity." Anyway, a suggestion if you can find a good way to express this, otherwise we can leave it until CHERI-specific content starts getting added.

share/man/man9/memmove.9

60

Same comment as above

[brooks]

Address feedback, switch references to memory_model.7

[brooks]

share/man/man9/memcpy.9

63

I've added some more active CHERI-specific content. In the non-CHERI case it's probably best to be somewhat passive here since it's literally a no-op today.

sys/libkern/bcopy.c

66 ↗

(On Diff #180517)

I've added a __maybe_unused.

[emaste]

share/man/man9/memcpy.9

66

[brooks]

fix typo

[emaste]

man page lgtm

[emaste]

s/CHERI system/CHERI systems/ in the commit message

share/man/man9/memcpy.9

73–74

Not part of your change but this is a stray .Pp

[kib]

share/man/man9/memcpy.9

54

Which pointer?
I suspect the pointers contained in the copied object?

[brooks]

Simplify C implementation of mem{cpy,move}.

Rather than clearing the tag on each capability sized and aligned block
of data, remove the permission allowing capabilites to be loaded from
the source capability. On all CHERI architectures, this allows data
to be loaded into registers, but does not propagate tags. This change
reduces diffs to FreeBSD and removes a branch on each uintptr_t of
data. We can't easily do this in CheriBSD because memmove doesn't take
capability arguments in a hybrid kernel.

[brooks]

Clarify which pointers provenance is preserved

[ziaee]

Nothing from my end!

share/man/man9/bcopy.9

51

I always thought we could not do this, but looking at mandoc -T{tree,html,pdf} and groff output, I think we can.

[brooks]

share/man/man9/bcopy.9

51

It looks like the key is macros that take a fixed number of arguments.

[kib]

sys/libkern/bcopy.c

86 ↗

(On Diff #180923)

Why these casts are not changed to ptraddr_t?

146 ↗

(On Diff #180923)

Can we have a dedicated file. e.g. subr_cheri.c, where the cheri utility functions are collected, instead of spreading MD #ifdefs into the common sources?

[jrtc27]

sys/libkern/bcopy.c

146 ↗

(On Diff #180923)

That gets CHERI out of the way for people who want to pretend it doesn't exist, but then would mean you have a total mishmash of random functions grouped together. Also, sys/kern/subr_cheri.c would not be in sys/libkern where it belongs, like memmove, and it would be quite surprising for memcpy/memmove variants to *not* live in bcopy.c where memcpy and memmove live.

[kib]

sys/libkern/bcopy.c

146 ↗

(On Diff #180923)

subr_cheri.c was mostly a placeholder name. It can be split into as many files as feels appropriate, and put into the right subdirectories. My point is that cheri is specific architecture, and accordingly its code should be put into the MD subdirectory, instead of spreading through the MI parts of the kernel.

[jrtc27]

sys/libkern/bcopy.c

146 ↗

(On Diff #180923)

CHERI is not a specific architecture. It is an architectural feature, like virtual memory. Multiple architectures exist that provide that model. Therefore it is definitively not MD. It is as "MI" as, say, "64-bit", which is a feature of multiple, but not all, architectures supported by FreeBSD.

[kib]

sys/libkern/bcopy.c

146 ↗

(On Diff #180923)

CHERI is not a specific architecture. It is an architectural feature

Of specific architectures

, like virtual memory

Of course no. Virtual memory is the requirement for the FreeBSD kernel to be ported to an architecture, at all.
Non-cheri arches happily exists, as is shown by the whole history of the FreeBSD.

Multiple architectures exist that provide that model. Therefore it is definitively not MD. It is as "MI" as, say, "64-bit", which is a feature of multiple, but not all, architectures supported by FreeBSD.

And we try hard to have #ifdef _LP64 (or whatever tries to replace this symbol now) as little as possible in the MI parts.

[jrtc27]

sys/libkern/bcopy.c

146 ↗

(On Diff #180923)

Sure, the comparison to virtual memory was perhaps not the right one to make in this context. And yes, we do minimise "if 64-bit" throughout the kernel, just as we will try to minimise "if CHERI". But when we do "if 64-bit" we don't have a subr_64bit.c for those functions, we just put them next to their counterparts.

[brooks]

I'm going to further decompose this review to pull the cherification of memmove into it's own commit.

sys/libkern/bcopy.c

86 ↗

(On Diff #180923)

Why these casts are not changed to ptraddr_t?

IIRC I left them alone because this cast was fine in practice (unlike the uintptr_t's below) and I wanted to keep diffs down. I'll change them.

146 ↗

(On Diff #180923)

In this case, I think a sys/libkern/memmove_data.c is reasonable as it will keep the history cleaner, won't impose excessive overhead, and will work with a future CHERI-aware MD implementation of memmove so I'll do it.

In places like the pmap code I think putting functions like pagecopy_cleartags and pmap_copy_page_tags in a separate file would not make sense and would result in missed optimizations.

[brooks]

Move _data implementation to libkern/memcpy_data. Move cherification of memmove to D57965.

[kib]

sys/libkern/bcopy.c

146 ↗

(On Diff #180923)

I agree with both points, thanks.

In fact, pmap.c would benefit from splitting on all arches, but this is not related to the cheri additions. pmaps are too bloated to keep it in single file, but the split is not by relatively small cheri additions.

[brooks]

I ended up pulling the implementations out of the commit I merged because their dependencies (kernel option for sys/conf/files and cheri headers aren't available yet.)