Jail and capability mode for shm_rename; add audit support for shm_rename

[matthew.bryan_isilon.com]

Summary

Co-mingling two things here:

1. Addressing some feedback from Konstantin and Kyle re: jail, capability mode, and a few other things
2. Adding audit support as promised.

The audit support change includes a partial refresh of OpenBSM from upstream, where my change to add shm_rename has already been accepted. I won't be working on refreshing anything else to support audit for those new event types.

Test Plan

Running the posixshm kyua tests, checking audit results manually. Example audit event:
header,96,11,shm_rename(2),0,Tue Oct 8 07:31:50 2019, + 580 msec
path,/t1
path,/t2
argument,2,0x1,flags
subject,root,root,0,root,0,778,755,0,0.0.0.0
return,success,0
trailer,96

[kib]

Please do not add generated files into the review. Also I think it is worth splitting the patch into fixes, and separate review for adding audit.

sys/kern/uipc_shm.c

433

No point in the assignment.

951

error != 0

1005

I do not see why this chunk is needed.

[matthew.bryan_isilon.com]

sys/kern/uipc_shm.c

1005

As I understand it: capability mode generally means that we would disallow messing with any global namespaces. Renaming an shm other than SHM_ANON would be messing with the SHM namespace. So: we quit early with ECAPMODE.

I'm sure you know better than I though. So: what is your though more specifically?

[kib]

sys/kern/uipc_shm.c

1005

I already answered this in my previous reply. The shm_rename(2) syscall is not listed in capabilities.conf, which means that in capability mode it is rejected right away by the infra code without ever entering into the syscall implementation routine. So it is simply impossible to have the test at line 1005 to ever return true.

[kevans]

sys/kern/uipc_shm.c

1005

shm_rename is also, unfortunately, not suitable for renames involving SHM_ANON mapping as either source or destination anyways, since SHM_ANON is a value that should not be suitable for copyinstr(9).

sys/sys/mman.h

328

The SHM_RENAME_* flags further up (outside of given context) should also be moved into the __BSD_VISIBLE namespace.

[matthew.bryan_isilon.com]

sys/kern/uipc_shm.c

1005

Yep; SHM_ANON is not allowed at all. There are even tests to make sure of it!

1005

Well...I was trying to plan for the future when somebody did add it but neglected to change this file :-p Engineering for quality. But I don't mind removing it.

sys/sys/mman.h

328

Good call; done.

[matthew.bryan_isilon.com]

CR fixes. Removed generated files. Leaving audit and other changes together (sorry).

[kib]

sys/kern/uipc_shm.c

450

It is too late to check for '/' at the start of name for jailed case. I understand that previous code did the same, but this is not a reason to keep it.

454

Extra blank line.

460

Extra blank line.

812

error != 0

1023

I see no point in use of strncmp.

1107–1109

No point in the assingment.

1111

Extra blank line.

1113

No need to check for NULL (I think I already noted that).

1115

Same.

[matthew.bryan_isilon.com]

sys/kern/uipc_shm.c

450

Could you tell me what you are suggesting? In the jail case this is pointing to the char after the jail name to make sure there is a '/' there. Do we also need to check the first character? I don't know if pr_path always has a leading '/'. So are you suggesting this?

if (path[pr_pathlen] != '/' || path[0] != '/')

Or do you mean we shouldn't do this check at all in the jail case? Or only check the first character?

454

Here and elsewhere: I'm leaving the blank lines for readability.

812

Good catch. Fixed.

1023

I think you mean vs strcmp. I just learned that copyinstr makes sure these are null terminated. Thanks. :)

1107–1109

It is needed for the ENOENT case above.

[matthew.bryan_isilon.com]

CR fixes

[kib]

Only stylistic or minor code organization issues are left in the patch, I do not see the need for you to wait for somebody to re-read this after fixing, so I am approving the change.

sys/kern/uipc_shm.c

437

We usually put the trailing binary (or ternary in this case) operator on the previous line. IOW, style prefers to keep ':' on the line 437.

450

I do not remember the previous version of your patch, but the current code looks fine. I mean that the user-supplied path must start with '/'.

Interesting but unrelated question is should we collapse multiple slashes into one.

454

Unneeded blank lines eat vertical space in editor.

Style only suggests empty lines in very specific cases, like split between local vars and code block, or before/after multiline-commented block.

1005

Hmm, it is not completely true. If mmap at zero is allowed (it could be), then (char *)1 is a valid pointer. I think we should check for SHM_ANON explicitly before calling copyinstr(9).

1107–1109

You mean, around line 1067. I suggest to reset error at that location, then.

[matthew.bryan_isilon.com]

sys/kern/uipc_shm.c

437

Good to know. I'll put the ternary line on a single line, breaking after '=='.

1005

Great point! I never thought of that case. Explicit is better too. I'll check for path in or path out being SHM_ANON.

1107–1109

Great idea. Done.