Page MenuHomeFreeBSD


Authored by kib on Feb 16 2019, 1:36 PM.


Group Reviewers
rS344355: pkru(3) man page.

Build glue is not added for a reason.

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Feb 16 2019, 1:36 PM
pho added inline comments.Feb 16 2019, 2:16 PM
60 ↗(On Diff #53987)


jilles added a subscriber: jilles.Feb 16 2019, 2:57 PM
jilles added inline comments.
62 ↗(On Diff #53987)

an associated 4-bit protection key

63 ↗(On Diff #53987)

A new per-thread PKRU hardware register determines

64 ↗(On Diff #53987)

for each protection key, whether user-mode addresses with that protection key

72–75 ↗(On Diff #53987)

Perhaps it should be mentioned that this EFAULT is not always a "proper" error in the sense that a failed system call may have been partially successful. For example, if sigaction(2) could not write to *oact, it will return the EFAULT error code even if the signal disposition was updated.

It is best for applications to pass pointers that may cause protection faults only for data buffers for read, write and the like, where the error can be properly reported (either as immediate EFAULT or as an incomplete transfer).

If the EFAULT is used as a trigger to abort the process, the above is not a problem.

90 ↗(On Diff #53987)

The system provides

kib marked 5 inline comments as done.Feb 16 2019, 3:44 PM
kib added inline comments.
72–75 ↗(On Diff #53987)

I am not sure what to do about this note and even how to interpret it. If taken literally, I have to note that sigaction(2) would fail in the same manner if oact is invalid but non-NULL pointer. So really the behavior of the syscalls is same as if passed pointer was invalid (but not NULL) or valid but readonly. In the corner case, it might be even a race where initially usable pointer now points to unusable (e.g. not writable) page.

So I do not think that this additional permission check changes potential syscalls outcome.

What it changes is that it makes easier to make useracc(9) useless, but this falls under the already acceptable behavior.

kib updated this revision to Diff 53989.Feb 16 2019, 3:45 PM


markj added inline comments.Feb 16 2019, 6:48 PM
55 ↗(On Diff #53989)

The dash here isn't needed.

61 ↗(On Diff #53989)

Enabled by what?

64 ↗(On Diff #53989)

No need for a comma after "key".

73 ↗(On Diff #53989)

"... it returns the EFAULT..."

77 ↗(On Diff #53989)

"that the system"

78 ↗(On Diff #53989)

"only available on amd64 systems"?

79 ↗(On Diff #53989)

Usually written "64-bit" and "32-bit".

83 ↗(On Diff #53989)

"The key indexes"

86 ↗(On Diff #53989)

"managed using the user-mode instructions"

94 ↗(On Diff #53989)

"The x86_pkru_protect_range function assigns..."

97 ↗(On Diff #53989)

"assigns key keyidx to..."

99 ↗(On Diff #53989)

"having length len."

102 ↗(On Diff #53989)

It should probably be explained that only one key may apply to a given range at a time. If _EXCL is not specified, x86_pkru_protect_range() will replace any existing key.

109 ↗(On Diff #53989)

"the zero key"

110 ↗(On Diff #53989)

"You must first remove any existing key with x86_pkru_unprotect_range() in order for this request to succeed.

117 ↗(On Diff #53989)

"You must use a x86_pkru_unprotect_range call"

124 ↗(On Diff #53989)

Missing "function" after the function name.

139 ↗(On Diff #53989)

I think you can s/at least the //.

140 ↗(On Diff #53989)

"the variable"

142 ↗(On Diff #53989)

"indicates that write access"

166 ↗(On Diff #53989)

"completion of the operation," or "the operation's completion"

Is it still possible for these calls to fail with ENOMEM?

181 ↗(On Diff #53989)

This expands to nothing, did you mean .Nm?

jilles added inline comments.Feb 16 2019, 6:58 PM
72–75 ↗(On Diff #53987)

My point is that while you can fix up userspace accesses by catching SIGSEGV, remapping the faulting address and returning from the signal handler, there is no way to do something similar for system calls in the general case. Reading the standards strictly, it is undefined behaviour to pass a pointer to a system function that does not permit the required access (where NULL is also valid if documented for the function). PKRU does not change this, but makes it more likely to trigger.

Perhaps a sentence "Note that some side effects may have occurred if this error is reported." could be added.

0mp added a subscriber: 0mp.Feb 16 2019, 7:28 PM
0mp added inline comments.
2 ↗(On Diff #53989)

All rights reserved is usually omitted in recent commits.

100 ↗(On Diff #53989)

.Fa len . (missing space)

kib updated this revision to Diff 53996.Feb 16 2019, 8:13 PM
kib marked 24 inline comments as done.


kib added inline comments.Feb 16 2019, 8:14 PM
61 ↗(On Diff #53989)

By user. More precisely, 'not disabled'. I removed the 'enabled' part.

78 ↗(On Diff #53989)

I mean that user might run i386 on the same machine.

166 ↗(On Diff #53989)

I want to document the interface. Not returning ENOMEM is actually an impl detail, e.g. we can start enforcing the limit (and probably should).

markj accepted this revision.Feb 17 2019, 5:07 PM
markj added inline comments.
166 ↗(On Diff #53989)


83 ↗(On Diff #53996)

Should be "Developer's"

92 ↗(On Diff #53996)

"for both"

136 ↗(On Diff #53996)


This revision is now accepted and ready to land.Feb 17 2019, 5:07 PM
kib updated this revision to Diff 54018.Feb 17 2019, 5:31 PM

More Mark' notes.

This revision now requires review to proceed.Feb 17 2019, 5:31 PM
alc added a comment.Feb 17 2019, 5:52 PM

I question if there shouldn't be an explicit statement somewhere in this man page that this feature can be used by an application to implement memory "safety" mechanisms, but not "security". Because the RDPKRU and WRPKRU instructions are not privileged, a compromised application could always change the access rights. This is different from the protection key mechanisms on other architectures such as 32-bit ARM.

35 ↗(On Diff #53996)

In this place, the 's' on the end of 'Keys' makes my ears hurt. :-) "Protection Key Rights" would be better.

79 ↗(On Diff #53996)

I think that 'AKA' is too informal for man pages.

151 ↗(On Diff #53996)

"Conversely, ..."

kib updated this revision to Diff 54020.Feb 17 2019, 6:26 PM
kib marked 7 inline comments as done.

Alan' notes.

jilles accepted this revision as: jilles.Feb 17 2019, 9:15 PM
This revision is now accepted and ready to land.Feb 17 2019, 9:15 PM
alc added inline comments.Feb 18 2019, 3:56 AM
74–75 ↗(On Diff #54020)

I wanted to highlight this sentence. My actual question is in my next comment.

117–118 ↗(On Diff #54020)

The earlier sentence that I highlighted could be interpreted as any address range has a initial, default key value of zero. But AMD64_PKRU_EXCL fails even for a zero key, so such an operation could never succeed.

I assume that this is not the intended interpretation, i.e., that there can be address ranges with no associated key. Yes?

kib added inline comments.Feb 18 2019, 8:17 AM
117–118 ↗(On Diff #54020)

When PKRU is enabled, hardware requires that any valid pte has a key assigned. By default, the key zero is installed.

There is a separate, software managed key, that can be in two states: assigned for a range, or not. If assigned, it can be removed, and then the state would be not assigned (in ptes, zero key is installed then). For _EXCL to succeed, there must be no sw key assigned to any page in the range.

alc added inline comments.Feb 18 2019, 4:40 PM
117–118 ↗(On Diff #54020)

This distinction between the hardware keys and the software managed keys doesn't really appear in the text, at least explicitly. Can you please add some text?

kib updated this revision to Diff 54043.Feb 18 2019, 5:04 PM

Consistently use the term 'assigned' to describe keys.
Explain assignment state changes and mention that hardware key zero is not a substitute for assigning key zero (which probably no useful app would do anyway).

This revision now requires review to proceed.Feb 18 2019, 5:04 PM
alc accepted this revision.Feb 19 2019, 3:45 AM
alc added inline comments.
112 ↗(On Diff #54043)

"After a successful call, ..."

113 ↗(On Diff #54043)

"changed" -> "change"

This revision is now accepted and ready to land.Feb 19 2019, 3:45 AM
markj accepted this revision.Feb 19 2019, 3:56 AM
This revision was automatically updated to reflect the committed changes.